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

0.8 - OAuth fixes #304

Merged
merged 8 commits into from Oct 11, 2016

Conversation

Projects
None yet
3 participants
@marshallswain
Copy link
Member

marshallswain commented Oct 6, 2016

This fixes a couple of problems that were occurring in the 1st half of the OAuth flow (up until showing the GitHub auth page) I think the entire OAuth flow should work for every supported provider, now.

  1. The feathers provider plugin name was overwriting the OAuth provider name.
  2. If the callbackURL didn't have a leading /, Passport would mess it up. (includes tests)

See commit messages for more details.

marshallswain added some commits Oct 6, 2016

normalize the callbackURL
If the callbackURL doesn’t begin with a slash, the Passport `authenticate()` call will mess up the URL depending on the referrer’s url.  So if the page is `domain.com/auth/github`, the callback URL will become `domain.com/auth/auth/github`.  This normalizes the URL so that relative URLs always begin with a leading slash and absolute URLs don’t get touched.
Normalize comparison URL & fix typo
This performs the same normalization to the comparison URL for the callback (as is done for the callbackURL).  Also fixes typo in callbackUrl, updates it to callbackURL.
Always use service lookup.
Previously, we were setting up this._tokenService and this._userService in the `setup` function.  This was a problem because not all services are registered by the time `setup` runs.  In order to continue to allow the passing of either strings or an actual service object, this PR checks if we only have a string reference to the service stored.  If so, it uses the `app.service` method of service lookup before attempting to use the service.  This also fixes a problem that was occurring when trying to call this._userService or this._tokenService when this was undefined inside the middleware callbacks.
@marshallswain

This comment has been minimized.

Copy link
Member Author

marshallswain commented Oct 8, 2016

I fixed a few more things, but it's still not quite finished. The OAuth flow completes correctly, now, but the response needs to be fixed, still. It's currently returning the JSON (includes the token and user fields) in the response instead of redirecting. So, we need to implement the successRedirect, which is probably something that will need to be configured for each OAuth provider. I suppose we could still use /auth/success as the default.

@daffl, @ekryski, @corymsmith

@@ -36,22 +38,24 @@ export class OAuth2Service {
updateUser(user, data) {
const idField = this.options.user.idField;
const id = user[idField];
const userService = typeof this._userService === 'string' ? this.app.service(this._userService) : this._userService;

This comment has been minimized.

@ekryski

ekryski Oct 9, 2016

Member

@marshallswain rather than duplicating this in each method can we DRY it up and put this in the setup like I have here: https://github.com/feathersjs/feathers-authentication/blob/0.8/src/services/oauth2.js#L176-L177?

This comment has been minimized.

@marshallswain

marshallswain Oct 9, 2016

Author Member

That's how it was originally setup, but if you do something like this:

app.configure(authentication);
app.use('/api/users', service(options));

The users service hasn't been setup by the time the auth plugin's setup method runs, so this._userService and this._tokenService are both stored on the class as undefined. I made it more DRY by putting the lookup into a class method in that last commit.

This comment has been minimized.

@ekryski

ekryski Oct 11, 2016

Member

Hmm. I don't remember that being an issue but you could be right that order might matter. In any case this is totally good for now. I might double check that order matters when it is in the setup method but it's pretty low priority right now. 👍

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Oct 9, 2016

Other than my one comment, looks good. :shipit:

We have the default success handler (which is just redirect middleware) so you can set successRedirect for each OAuth provider you register and it will handle it. Alternatively you can set your own successHandler if you want to register your own middleware after OAuth.

So I think we are pretty well covered there but maybe if we are using the default successhandler and don't have a successRedirect we should set a default one or throw an error telling the developer they need to set one for OAuth. I personally prefer the latter.

marshallswain added some commits Oct 9, 2016

DRY up the dynamic token and user service lookup.
This moves the dynamic service lookup to a function to be a bit more DRY.
OAuth require successRedirect with default successHandler
If a successHandler hasn’t been passed, then the default `successHandler` will be used.  The `successRedirect` will now be required in that scenario.
@marshallswain

This comment has been minimized.

Copy link
Member Author

marshallswain commented Oct 9, 2016

@ekryski I committed a line for the successHandler. Please check to make sure I understood you correctly.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Oct 11, 2016

yup looks good homie. :shipit:

First cut for authentication middleware (#305)
* First cut for authentication middleware

* Fix service methods

* Allow passing user service and service name

@marshallswain marshallswain merged commit 531898a into 0.8 Oct 11, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@marshallswain marshallswain deleted the 0.8-oauth-fixes branch Oct 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.