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

Auth token and avatarUrl security improvements #1514

Merged
merged 15 commits into from Sep 22, 2018

Conversation

@tobyzerner
Copy link
Member

commented Jul 20, 2018

Supersedes #1388
Fixes #1357
Fixes #1359

Changes proposed in this pull request:
This PR updates the AuthenticationResponseFactory class so that you can add additional data to an auth token's payload without it being used to identify the user. This allows the avatarUrl to be safely stored in the auth token, rather than passed through the frontend as a "suggestion".

The ability to set an avatarUrl as an API attribute is removed completely, fixing #1357.

This PR also removes AbstractOAuth2Controller. There is no reason to provide an implementation for a specific oAuth2 library in core; it's not generic enough (eg. auth-twitter can't use it).

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run php vendor/bin/phpunit).

Required changes:

@luceos

This comment has been minimized.

Copy link
Member

commented Jul 20, 2018

Scanned over the PR real quick, it seems to me that passing over the full payload will be sufficient for any use case.

Not sure if the column size is sufficient for all use cases, especially if multiple scopes have been enabled providing json about campaigns, pledges and what not.. I think that was also the reason I had not pushed the full payload along; this could be circumvented in the implementation of a Patreon controller easily by removing unnecessary payload.

@tobyzerner

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2018

@luceos My concern is that when registration takes place the items in the payload are automatically set as attributes on the user. I guess this won't be flexible enough?

@luceos

This comment has been minimized.

Copy link
Member

commented Jul 20, 2018

payload are automatically set as attributes on the user.

I am not sure i understand but if you suggest that any property in the payload will be assigned to the user than you will have an issue. Eg we would need to know what tier the user is on at patreon.com; we allow passing that over so that after creation we can assign the role. The tier or tier_id column does not exist, so the database will complain that a user is being saved with attributes that it does not have columns for.

So we need a way to send additional payload from the response of the oauth2 server to after user creation/updating without setting them on the user.

@franzliedke
Copy link
Member

left a comment

Code looks great.

@franzliedke franzliedke referenced this pull request Jul 21, 2018
['identificationFields' => array_keys($identification)]
);
}
$payload = array_merge($identification, $suggestions);

This comment has been minimized.

Copy link
@franzliedke

franzliedke Jul 21, 2018

Member

Not sure if this is critical at all, but I could imagine that preferring the keys in $identification over those in $suggestions (by swapping the parameter order here) might be somewhat safer?

@tobyzerner

This comment has been minimized.

Copy link
Member Author

commented Jul 21, 2018

I'm going to amend this to address the flexibility issues discussed above, and to fix #897.

@tobyzerner tobyzerner changed the title Auth token and avatarUrl security improvements [WIP] Auth token and avatarUrl security improvements Jul 22, 2018

@tobyzerner

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2018

Have just pushed changes. See example usage in flarum/auth-facebook#15. /cc @luceos @franzliedke

@tobyzerner tobyzerner requested review from franzliedke and luceos Sep 15, 2018

@tobyzerner tobyzerner changed the title [WIP] Auth token and avatarUrl security improvements Auth token and avatarUrl security improvements Sep 15, 2018

@luceos

luceos approved these changes Sep 18, 2018

@franzliedke franzliedke force-pushed the tz/1359-auth-tokens branch 2 times, most recently from da85d69 to c75b248 Sep 21, 2018

@franzliedke
Copy link
Member

left a comment

I rebased, fixed the conflict and fixed the tests (related to migrations being out-of-date).

auth-twitter and auth-github still need an update. Once these are done, feel free to merge. Great work. 👍

@franzliedke

This comment has been minimized.

Copy link
Member

commented Sep 21, 2018

So this fixes #1357, #1359 and #897? If that's correct, please make sure all of them get closed (and assigned to mileston beta8) as well.

EDIT: There are some other issues referenced in some of these, you might wanna double-check those, too.

tobyzerner added a commit to flarum/auth-facebook that referenced this pull request Sep 22, 2018

tobyzerner added a commit to flarum/auth-github that referenced this pull request Sep 22, 2018

tobyzerner added a commit to flarum/auth-twitter that referenced this pull request Sep 22, 2018

tobyzerner and others added some commits Jul 20, 2018

Remove AbstractOAuth2Controller
There is no reason to provide an implementation for a specific oAuth2
library in core; it's not generic enough (eg. auth-twitter can't use it).

This code could be moved into another package which auth extensions
depend on, but it's a negligible amount of relatively simple code that
I don't think it's worth the trouble.
Change AuthenticationResponseFactory interface
* AuthenticationResponseFactory now accepts a multidimensional array
  of data:
  - `identification` is an array of attributes used to identify
    the user.
  - `attributes` is an array of additional attributes to store
    in the token payload (avatarUrl goes here).
  - `suggestions` is an array of attributes to pass to the frontend
    (these will not be stored).

* Rename the `identificationFields` key to `provided`.
Tighten avatarUrl security
* avatarUrl is only read from an auth token payload when registering
  a new user.

* avatarUrl is ignored when updating a user.
Apply fixes from StyleCI
[ci skip] [skip ci]
Introduce login providers
Users can have many login providers (a combination of a provider name
and an identifier for that user, eg. their Facebook ID).

After retrieving user data from a provider (eg. Facebook), you pass the
login provider details into the Auth\ResponseFactory. If an associated
user is found, a response that logs them in will be returned. If not, a
registration token will be created so the user can proceed to sign up.
Once the token is fulfilled, the login provider will be associated with
the user.
Apply fixes from StyleCI
[ci skip] [skip ci]

@tobyzerner tobyzerner force-pushed the tz/1359-auth-tokens branch from 6ee44e5 to c6cfd66 Sep 22, 2018

@tobyzerner tobyzerner merged commit 5dfb9b4 into master Sep 22, 2018

3 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
WIP ready for review
Details
continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/styleci/push The analysis has passed
Details

@tobyzerner tobyzerner deleted the tz/1359-auth-tokens branch Sep 22, 2018

tobyzerner added a commit to flarum/auth-twitter that referenced this pull request Sep 22, 2018

tobyzerner added a commit to flarum/auth-github that referenced this pull request Sep 22, 2018

Update for core auth token changes (#12)
* See flarum/core#1514
* Update to league/oauth2 v2.0

tobyzerner added a commit to flarum/auth-facebook that referenced this pull request Sep 22, 2018

Update for core auth token changes (#15)
* See flarum/core#1514
* Update to league/oauth2 v2.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.