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

[b8] master token fix #1622

Merged
merged 8 commits into from
Nov 7, 2018
Merged

[b8] master token fix #1622

merged 8 commits into from
Nov 7, 2018

Conversation

luceos
Copy link
Member

@luceos luceos commented Oct 31, 2018

Changes proposed in this pull request:

Two issues covered in this PR:

  • Fixes the master token not working. The id column on the api_keys table is no longer used for the token strings. We introduced the key column for that. The AuthenticateWithHeader middleware now uses that.
  • The 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 the last_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.
  • Backend changes: tests are green (run php vendor/bin/phpunit).

luceos and others added 2 commits October 31, 2018 15:54
…holds key and adds flexibility of user_id column
[ci skip] [skip ci]
@luceos luceos changed the title Dk/fix master token [b8] master token fix Oct 31, 2018
@franzliedke
Copy link
Contributor

Can we get the bugfix without the new feature, please? I want the former now, but more time to think about the latter.

@luceos
Copy link
Member Author

luceos commented Oct 31, 2018

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.

@tobyzerner
Copy link
Contributor

I'm happy with this :)

tobyzerner
tobyzerner previously approved these changes Oct 31, 2018
@franzliedke
Copy link
Contributor

In that case, tests would be awesome. 😁

@luceos
Copy link
Member Author

luceos commented Oct 31, 2018

Yes, give me a day to patch at least a test or two..

@luceos luceos changed the title [b8] master token fix WIP: [b8] master token fix Oct 31, 2018
luceos added a commit that referenced this pull request Nov 1, 2018
Fixes phpdoc while working on #1622
@luceos luceos mentioned this pull request Nov 1, 2018
@luceos
Copy link
Member Author

luceos commented Nov 1, 2018

@flarum/core please weaponise your ocd goggles and give an opinion on the test. Let me give the reasoning first:

  • I can't really use the existing Flarum Client we use internally, because it circumvents the middleware.
  • I decided to push the request through the middleware so we can sensibly test the pipeline. The test injects a custom middleware instance using the (obsoleted) ConfigureMiddleware class, we'll need a replacement for that.
  • The problem with resolving flarum.api.middleware is that the DispatchRoute middleware is appended immediately when resolved and there's no way to re-arrange the entries in the pipeline.

These tests took more time than I hoped, bear with me 👍

@luceos luceos dismissed tobyzerner’s stale review November 1, 2018 12:46

no longer valid due to tests

@tobyzerner
Copy link
Contributor

The problem with resolving flarum.api.middleware is that the DispatchRoute middleware is appended immediately when resolved and there's no way to re-arrange the entries in the pipeline.

DispatchRoute is added to the pipe via afterResolving (see ForumServiceProvider) so using resolving should work fine?

@luceos
Copy link
Member Author

luceos commented Nov 2, 2018

Thanks for pointing that one out @tobscure, I assumed afterResolving was the only callback available.

Modified and patched.

@luceos luceos changed the title WIP: [b8] master token fix [b8] master token fix Nov 5, 2018
@franzliedke franzliedke merged commit bb0fc16 into master Nov 7, 2018
@franzliedke franzliedke deleted the dk/fix-master-token branch November 7, 2018 21:34
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