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

Cast client ids to strings before comparing them #1054

Closed
wants to merge 1 commit into from

Conversation

strongholdmedia
Copy link

Currently a request variable is compared with a stored string.
With strict comparison, this leads to issues when the provided client ID is e.g. an integer (by type).
This is a completely backwards-compatible change that relieves users from having to explicitly quote numerical client IDs.

Currently a request variable is compared with a stored string.
With strict comparison, this leads to issues when the provided client ID is e.g. an integer (by type).
This is a completely backwards-compatible change that relieves users from having to explicitly quote numerical client IDs.
@strongholdmedia
Copy link
Author

There must have been some recent change, given that our long-lived tokens once expired, then clients could not request any newer ones; they received "invalid_request" instead, with a hint to check the code.

Perhaps the auth code-based flow is not quite widespread nowadays, I have no idea.

@Sephster
Copy link
Member

Thanks for your pull request @strongholdmedia. I'm not sure this change is required. As per the docblocks, the client from the DB should be returned as a string. I think the client ID coming from the request will always be a string as well.

Is your client returning an int from the DB?

@strongholdmedia
Copy link
Author

strongholdmedia commented Sep 27, 2019

Is your client returning an int from the DB?

I am not sure.
What I do know that this solved an issue, where I dumped the values, and one was an int, and the other definitely a string.
I am, however, uncertain if it was the database one that was string.
Please bear with me while I try to find the issue. As a last resort, I will check the logs that lead to this fix, but I can only do that during work hours (i.e. next week).

@strongholdmedia
Copy link
Author

Ah, now I see what the issue is.

laravel/passport migration creates the client id as
$table->unsignedInteger('client_id');
This basically means the docblock is wrong.
That you will receive a string in the place of integer is

  • only true for PDO,
  • presumably only with the MySQL "legacy" driver, and
  • only when using emulated prepares.

See here:

https://stackoverflow.com/questions/20079320/php-pdo-mysql-how-do-i-return-integer-and-numeric-columns-from-mysql-as-int/20123337#20123337

Since we fixed some issues of the PDO MySQL Native Driver in PHP 7.3 (please see laravel/framework#24214, and php/php-src#3257 for further reference), it is expectable that anyone turning off emulated prepares with PDO-MySQL, or the native driver itself, will receive integers as integers (that is the very nature of the database interface to use native types wherever available).

So that the current behavior that is documented is only valid under legacy circumstances.

Given that, again, this change does not break anything in retrospect, but actually makes the server more failsafe, I would keep insisting on this be merged.

@strongholdmedia
Copy link
Author

Of course, I could then cast both to ints, but that'd break possible other uses of the server where they did not opt on using numerical client ids (that IIRC is not mandatory).

@Sephster
Copy link
Member

Thanks for your investigation into this. The client entity interface has stipulated that a string should be returned since as early as 2015. Because this is an implementation issue, where the implementation might not match the requirements of the library,

I think this would be better solved downstream in Passport. I'm going to close this issue and recommend this is raised in Passport to see if they will implement a fix for this. Thank you for your time spent looking at this. It is very much appreciated.

@Sephster Sephster closed this Sep 29, 2019
@strongholdmedia
Copy link
Author

Okay, thank you, I will.
Please consider fixing the documentation though then.
Requirements toward client entities in the client repository interface documentation are completely omitted, and the only hint that it should be a string is in the documentation of some of the repository interfaces in a not very straightforward notion of

getClient()->getIdentifier() : string

@strongholdmedia
Copy link
Author

Should anyone want to use this fix, please mind that you will need to change all the grants you use. I just missed that. 😳

@strongholdmedia
Copy link
Author

I wish to let you know that Passport actually uses Entity trait from oauth2-server, that has identifier tagged as "mixed", with no type enforcement.
So, strictly speaking, it is not necessarily an implementation issue.

@Sephster
Copy link
Member

Sephster commented Oct 2, 2019

Thanks @strongholdmedia - This is because the entity trait is common with a number of interfaces. The user identifier repository is defined to return mixed so this is why the trait does the same. All other interfaces return strings as their identifiers. Cheers

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.

2 participants