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 25 commits into
base: 13.x
Choose a base branch
from

Conversation

hafezdivandari
Copy link
Contributor

@hafezdivandari hafezdivandari commented Oct 31, 2024

Before Release

  • Merge branch 12.x into 13.x

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
  • Fix some types and PHPDoc types.
  • Enhance commands' styles by using components.
  • Use Date facade instead of Carbon.
  • Use UUID v7 for new clients as discussed here.
  • 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.
    • Remove lcobucci/jwt package. Not needed anymore.
  • Use registered exp and jti claims instead of non-standard expiry and csrf claims on JWT cookie.
  • Cleanup.
  • Add more tests / assertions.
  • Improve tests by fixing risky, warnings and notices.

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.

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