Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[13.x] Release Passport 13.x #1797

Draft
wants to merge 55 commits into
base: 13.x
Choose a base branch
from

Conversation

hafezdivandari
Copy link
Contributor

@hafezdivandari hafezdivandari commented Oct 31, 2024

Before Release

After Release

This PR does final touches and improvements that makes Passport ready for release:

  • Add tests for PHP 8.4 Add PHP 8.4 support thephpleague/oauth2-server#1454
  • Add support for Laravel 12.x.
  • Fix many types and PHPDoc types:
    • Add new OAuthenticatable interface for better typing. Implemented by HasApiTokens trait.
    • Add new ScopeAuthorizable interface for better typing. Implemented by AccessToken and TransientToken classes.
  • Fix the relation between the HasApiTokens and the Client models:
    • This is a backward compatible change. Old projects have the user_id column on the oauth_clients table and can continue using HasApiTokens::clients() and Client::user() relation methods.
    • New projects have owner_id and owner_type columns on the oauth_clients table and will use new HasApiTokens::oauthApps() and Client::owner() relation methods.
    • Why?
      • The HasApiTokens trait can be used on any Authenticatable model, but the old relation method were guessing the related models based on the user provider which is wrong. For example, the owner of the client can be an admin but the client's provider is a regular user. New relations are morphable that fixes this bug.
      • The clients relation method on the HasApiTokens trait that typically will be used on User model, has a very general name! It's very likely that developers use this relation name on their User model, which causes conflict. The new oauthApps and owner names are much clearer for this relationship: The owner has many registered OAuth apps and each OAuth app has an owner.
    • Deprecate HasApiTokens::clients() method, in favor of the new oauthApps() relation method.
    • Deprecate Client::user() method, in favor of the new owner() relation method.
  • Enhance commands' styles by using components.
  • Use Date facade instead of Carbon.
  • Use HasUuid trait on the Client model.
  • Enable PSR-17 auto-discovery:
  • Add support for legacy "client_credentials" tokens on EnsureClientIsResourceOwner middleware, where oauth_user_id was null prior to version 9.0 of league/oauth2-server.
  • Add $user->tokenCant() method to determine a missing scope. (Sanctum has the same method).
  • Improve issuing PATs:
    • Access to token's attributes on result, including token ID and expire time.
    • Improve performance by setting the token's name on grant instead of re-parsing the JWT on the factory.
    • Dispatch an event after access token is issued, just like other grant types.
    • Remove lcobucci/jwt package. Not needed anymore.
  • Use registered exp instead of non-standard expiry claim on JWT cookie.
    • Fix a bug where checking expiry time was mistakenly ignored when Passport::$ignoreCsrfToken was set to true.
  • Pass guard name to AuthenticationException when thrown on AuthorizationController.
  • Better naming for 2 recently added methods before release:
    • Newly added User::getProvider() method has been renamed to getProviderName().
    • Newly added ResolvesInheritedScopes::scopeExists() method has been renamed to scopeExistsIn().
  • Add many more tests / assertions.
  • Improve tests by fixing deprecations, risky, warnings and notices.
  • Cleanup.

Copy link

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

@jonerickson
Copy link
Contributor

jonerickson commented Nov 16, 2024

@hafezdivandari With the switching of Passport to a headless approach, have you taken into account Inertia external redirects yet? Specifically in ApproveAuthorizationController if using an Inertia component to render the approve screen. Currently the authorization server will return a standard 302 when approving/denying an auth request - Inertia expects a 409 status to perform the correct external redirect. Might need to create and bind a new ApproveAuthorizationResponse to return the correct Inertia::location response. I have tested this in the standard auth code grant flow, I'm sure it happens for implicit as well.

@hafezdivandari
Copy link
Contributor Author

hafezdivandari commented Nov 17, 2024

@jonerickson currently external redirect happens when utilizing "implicit" and "auth code" grants on different places / scenarios:

  • On AuthorizationController:
    • In case of invalide_scope, login_required, and consent_required exceptions.
    • Implicit approval (consent skipped).
  • On ApproveAuthorizationController: explicit approval.
  • On DenyAuthorizationController: access_denied exception.

So it's not easily possible to just bind a response for each case to handle Inertia redirect. But 2 solutions come to mind:

  1. Having a config method (Passport::$responseHandler) to be used on Passport\Http\Controllers\ConvertsPsrResponses (I don't like this ATM).
  2. Having a middleware on the app itself (not Passport) to catch 302 responses and use Inertial::location() method (like this)?

@jonerickson
Copy link
Contributor

@hafezdivandari Right - I understand the various locations of all external redirects - was just giving you an exact example of where this can happen.

I am not a fan of the $responseHandler approach either. I have see other places where support for Inertia is baked right into the source rather than having the app developer responsible for the implementation. Taking that into account, and using how I am currently handling the edge case, is to define a middleware on the routes that can return redirects and handle the status code changes. For example:

$response = parent::handle($request, $next);

if ($response->isRedirection() && $request->inertia() && $response->headers->has('location')) {
    return Response::make('', 409, [Header::LOCATION => $response->headers->get('location')]);
}

return $response;

This removes the responsibility from the app developer into the framework, hopefully reducing the potential for implementation errors. Whatcha think?

@hafezdivandari
Copy link
Contributor Author

@jonerickson As you know, Passport is headless and not dependent on Inertia, so it is not the right place to handle this edge case. You may refer to laravel/jetstream#1521 and laravel/breeze#398 for more.

@crynobone crynobone mentioned this pull request Jan 13, 2025
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants