Skip to content

Conversation

@axlon
Copy link
Contributor

@axlon axlon commented Jul 10, 2025

Passport refers to users through their auth identifier, similarly to guards and session handlers found in the framework itself. Normally this is also the user's primary key, but if a developer opts to use a different column as the auth identifier Passport will break, because while it stores the user's auth identifier it tries to look up tokens through the user's primary key.

This PR fixes this by making the tokens() relation use the auth identifier.

Passport already uses the auth identifier in most places, but it seems it isn't doing it consistently:

This should not be a breaking change because the auth identifier is set to the primary key by default. If the developer was already using a different key Passport would already not work.

Related PR:

@hafezdivandari
Copy link
Contributor

hafezdivandari commented Jul 10, 2025

The change on tokens relation seems correct as it is based on auth identifier, actually user_id is not proper name here, it actually refers to resource owner and depends on the auth provider (i.e. client's provider)

But the change on oauthApps relation seems incorrect, you may also check the revers relation, as it is based on the model key and has nothing to do with the auth ID?

Also the change on the clients relation, I don't recommend as it may be a breaking change, this relation was wrong from the beginning, that's why it is deprecated in favor of the new oauthApps relation.

@axlon axlon marked this pull request as draft July 10, 2025 11:16
@axlon axlon force-pushed the auth-identifier branch from 1365142 to 5d35277 Compare July 10, 2025 11:41
@axlon
Copy link
Contributor Author

axlon commented Jul 10, 2025

@hafezdivandari good catch! I had some local changes where I was explicitly passing the auth identifier into these related models, which led to the mistake

@axlon axlon marked this pull request as ready for review July 10, 2025 11:43
@hafezdivandari
Copy link
Contributor

But the changes are still there, you may consider reverting those changes on clients and oauthApps?

@axlon axlon force-pushed the auth-identifier branch from 5d35277 to 649bd9f Compare July 10, 2025 13:39
@axlon
Copy link
Contributor Author

axlon commented Jul 10, 2025

But the changes are still there, you may consider reverting those changes on clients and oauthApps?

🤦‍♂️ I thought I reverted them before, should be fixed now!

@axlon axlon changed the title [13.x] Fix broken relations when auth identifier is not PK [13.x] Fix broken token relation when auth identifier is not PK Jul 10, 2025
@taylorotwell taylorotwell merged commit c5bf6c5 into laravel:13.x Jul 10, 2025
8 checks passed
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