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

failureRedirect is never used when using with oauth2 #387

Closed
boybundit opened this Issue Dec 30, 2016 · 7 comments

Comments

Projects
None yet
3 participants
@boybundit
Copy link
Contributor

boybundit commented Dec 30, 2016

Steps to reproduce

When using with oauth2 plugin with e.g. FacebookStrategy, failureRedirect option is never used by auth.express.authenticate() when it receives callback with error e.g. /auth/facebook/callback?error=access_denied&error_code=200&error_description=Permissions+error&error_reason=user_denied#_=_ and it will throw NotAuthenticated error instead.

Expected behavior

When callback URL with error is triggered, and failureRedirect is provided, it should redirect accordingly.

Actual behavior

NotAuthenticated error is thrown.

System configuration

Module versions (especially the part that's not working):

+-- feathers@2.0.3
+-- feathers-authentication@1.0.2
+-- feathers-authentication-jwt@0.3.1
+-- feathers-authentication-oauth2@0.2.3
+-- passport-facebook@2.1.1

NodeJS version: 6.9.1

Operating System: Windows 10

Browser Version: Chrome 55.0.2883.87 m

Module Loader: Node.js

@boybundit

This comment has been minimized.

Copy link
Contributor Author

boybundit commented Dec 30, 2016

To be specific, it is in express/authenticate.js.

I understand that for both oauth1 and oauth2 plugins, their callback handler already include auth.express.failureRedirect(authSettings), so we should call next(err) to let the error bubble up the middleware chain properly, instead of rejecting a promise.

       if (result.fail) {
        if (options.failureRedirect && !options.__oauth) {
          debug(`Redirecting to ${options.failureRedirect}`);
          res.status(302);
          return res.redirect(options.failureRedirect);
        }

        const { challenge, status = 401 } = result;
        let message = challenge && challenge.message ? challenge.message : challenge;

        if (options.failureMessage) {
          message = options.failureMessage;
        }

        res.status(status);
        return Promise.reject(new errors[status](message, challenge));
      }
@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Jan 20, 2017

Weird. That promise rejection should be caught here: https://github.com/feathersjs/feathers-authentication/blob/master/src/express/authenticate.js#L66

I might need to add a few more tests to test redirect. Will take a look over the next few days.

@ekryski ekryski added the Bug label Jan 20, 2017

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Feb 20, 2017

I've verified manually that this is working. Will try and add some more tests today for it.

@buske

This comment has been minimized.

Copy link

buske commented Mar 23, 2017

I'm experiencing the same issue. The promise is getting caught as expected, and the error is indeed bubbling so that the necessary middleware is executing. However, the feathers-authentication failureRedirect middleware requires res.hook.data.__redirect to already be set to the redirect URL (https://github.com/feathersjs/feathers-authentication/blob/master/src/express/failure-redirect.js#L13).

But __redirect is never set in feathers-authentication:authenticate, and is only set by feathers-authentication-oauth2 if the app.service('authentication').create call fails (https://github.com/feathersjs/feathers-authentication-oauth2/blob/master/src/express/handler.js#L34-L37). It isn't set when the app.authenticate call fails (https://github.com/feathersjs/feathers-authentication-oauth2/blob/master/src/index.js#L69). I think there needs to be another middleware added to the callback handler to set __redirect accordingly so the failureRedirect actually redirects.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Mar 24, 2017

Ahhhhh. Good catch @buske!

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Mar 24, 2017

OAuth2 has been fixed and release as v0.2.4.

Just need to update OAuth1 to close this out.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Mar 31, 2017

I started to fix OAuth1 but @buske came through and did it 2 days ago. 👏 .

OAuth1 has been fixed and released as v0.2.4.

@ekryski ekryski closed this Mar 31, 2017

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.