-
Notifications
You must be signed in to change notification settings - Fork 786
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
base: 13.x
Are you sure you want to change the base?
Conversation
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. |
@hafezdivandari With the switching of Passport to a headless approach, have you taken into account Inertia external redirects yet? Specifically in |
@jonerickson currently external redirect happens when utilizing "implicit" and "auth code" grants on different places / scenarios:
So it's not easily possible to just bind a response for each case to handle Inertia redirect. But 2 solutions come to mind:
|
@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
This removes the responsibility from the app developer into the framework, hopefully reducing the potential for implementation errors. Whatcha think? |
@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. |
Before Release
After Release
This PR does final touches and improvements that makes Passport ready for release:
OAuthenticatable
interface for better typing. Implemented byHasApiTokens
trait.ScopeAuthorizable
interface for better typing. Implemented byAccessToken
andTransientToken
classes.HasApiTokens
and theClient
models:user_id
column on theoauth_clients
table and can continue usingHasApiTokens::clients()
andClient::user()
relation methods.owner_id
andowner_type
columns on theoauth_clients
table and will use newHasApiTokens::oauthApps()
andClient::owner()
relation methods.HasApiTokens
trait can be used on anyAuthenticatable
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.clients
relation method on theHasApiTokens
trait that typically will be used onUser
model, has a very general name! It's very likely that developers use this relation name on theirUser
model, which causes conflict. The newoauthApps
andowner
names are much clearer for this relationship: The owner has many registered OAuth apps and each OAuth app has an owner.HasApiTokens::clients()
method, in favor of the newoauthApps()
relation method.Client::user()
method, in favor of the newowner()
relation method.components
.Date
facade instead of Carbon.HasUuid
trait on theClient
model.nyholm/psr7
package. Not needed anymore.EnsureClientIsResourceOwner
middleware, whereoauth_user_id
wasnull
prior to version 9.0 ofleague/oauth2-server
.$user->tokenCant()
method to determine a missing scope. (Sanctum has the same method).lcobucci/jwt
package. Not needed anymore.exp
instead of non-standardexpiry
claim on JWT cookie.Passport::$ignoreCsrfToken
was set to true.AuthenticationException
when thrown onAuthorizationController
.User::getProvider()
method has been renamed togetProviderName()
.ResolvesInheritedScopes::scopeExists()
method has been renamed toscopeExistsIn()
.