-
-
Notifications
You must be signed in to change notification settings - Fork 835
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
[b8] master token fix #1622
[b8] master token fix #1622
Conversation
…holds key and adds flexibility of user_id column
[ci skip] [skip ci]
Can we get the bugfix without the new feature, please? I want the former now, but more time to think about the latter. |
Not adopting that code will mean that an Api Key created connected to a specific user can still authenticate for any user. That seems highly unintentional. And yes I can split it up, or we can rethink this logic and improve it for beta 9. |
I'm happy with this :) |
In that case, tests would be awesome. 😁 |
Yes, give me a day to patch at least a test or two.. |
Fixes phpdoc while working on #1622
@flarum/core please weaponise your ocd goggles and give an opinion on the test. Let me give the reasoning first:
These tests took more time than I hoped, bear with me 👍 |
DispatchRoute is added to the pipe via |
Thanks for pointing that one out @tobscure, I assumed Modified and patched. |
Changes proposed in this pull request:
Two issues covered in this PR:
id
column on theapi_keys
table is no longer used for the token strings. We introduced thekey
column for that. TheAuthenticateWithHeader
middleware now uses that.api_keys
column now has an user_id column, this allows generating a token forced for a specific user. In order to adhere to that while still allowing full admin use, the middleware no longer requires a; userId=
part to see whether an ApiKey is available. In addition it now applies a touched at date in thelast_activity_at
column the same way the AccessToken works.Reviewers should focus on:
Is this okay? Any security complications we can see in the PR?
Do we need a follow up issue to clean up the code duplication with ApiKey and AccessToken?
I think this needs a test too.. At least confirming the bare functionality of master tokens work because I believe it's a very strong feature used by integrations..
Confirmed
Frontend changes: tested on a local Flarum installation.php vendor/bin/phpunit
).