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

local auth should not redirect by default #89

Closed
jack-guy opened this Issue Mar 1, 2016 · 16 comments

Comments

Projects
None yet
5 participants
@jack-guy
Copy link

jack-guy commented Mar 1, 2016

Currently one can set successRedirect or failureRedirect but not a null value for either. This is an issue when you are relying on CORS, because browsers do not permit a redirect after a pre-flight options request, which many frameworks depend on.

There should be an option to disable redirects and instead just send back a general success response. I've hacked it together for my project, but should be standardized (and documented) IMO. I would submit a PR, but I'm not exactly sure what the response should be.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Mar 1, 2016

@harangue Yes I agree. I assume you are using local authentication without Ajax or sockets in this case?

The redirect was initially introduced for OAuth but I think is also included for local auth when not using ajax. I could see how that would cause issues with CORS. Do you mind posting a gist or some sample code highlight the issue so that I have a better understanding of how you are sending data for auth?

@jack-guy

This comment has been minimized.

Copy link
Author

jack-guy commented Mar 1, 2016

@ekryski I'm using the feathers-authentication client. Simply calling

app.authenticate({
    type: 'local',
    username: username,
    password: password
});

I get a successful request (with cookies set), but it's a redirect to /auth/success.

XMLHttpRequest cannot load http://localhost:3000/auth/local. The request was redirected to 'http://localhost:3000/auth/success', which is disallowed for cross-origin requests that require preflight.

Problematically, when I change the res.redirect in middleware/index.js to a res.send({}); as an attempted fix, the feathers-authentication client expects the response to contain both the token and user data. Not sure how it's supposed to get that from the redirect either, so it may just be broken entirely.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Mar 1, 2016

@harangue Yup got it. Thanks for posting that! It really shouldn't behave that way. I'll probably just remove the redirect altogether from local auth (it's not really needed) or at the very least make it optional.

IMO I made a mistake adding it there. The redirect really is only needed for OAuth (and potentially form posts), if you needed to redirect and are auth'ing over ajax or sockets then you can do that on the client.

@ekryski ekryski changed the title There should be an option for noRedirect local auth should not redirect by default Mar 1, 2016

@ekryski ekryski added the Bug label Mar 1, 2016

@jack-guy

This comment has been minimized.

Copy link
Author

jack-guy commented Mar 1, 2016

Awesome! Should I just switch to websocket authentication in the mean time?

@jack-guy

This comment has been minimized.

Copy link
Author

jack-guy commented Mar 1, 2016

Oh - this is important. Just upgraded to 0.4.0 and feathers-localstorage won't work either because options fails

XMLHttpRequest cannot load http://localhost:3000/storage/user. Response for preflight has invalid HTTP status code 404

Should I open a separate issue?

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Mar 1, 2016

@harangue Nope this one is fine. I'm going to be working in feathers-auth all day today so I should be able to get a bunch of stuff closed.

@jack-guy

This comment has been minimized.

Copy link
Author

jack-guy commented Mar 1, 2016

@ekryski Scratch that last one. Made a mistake in my config. :)

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Mar 1, 2016

👍

@jack-guy

This comment has been minimized.

Copy link
Author

jack-guy commented Mar 1, 2016

@ekryski I'll just keep adding things as I run into them here, if that's cool. :)

    app.token = function() {
      return storage().get('token').then(data => data.value);
    };

I don't think it should assume you're authenticated. The natural thing for me to try was using app.token to check if I was logged in, but that of course throws an error if there's no data.value;

@jack-guy

This comment has been minimized.

Copy link
Author

jack-guy commented Mar 1, 2016

@ekryski Pretty sure hook.params.token in client/hooks.js is actually supposed to be hook.params.token.value. The client was setting Authorization: [object Object] for me until I changed that.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Mar 1, 2016

@harangue you are correct. I thought I had committed that fix while working on the RN app.

@jack-guy

This comment has been minimized.

Copy link
Author

jack-guy commented Mar 2, 2016

@ekryski Another issue I encountered - the documentation claims that you can set idField on your authentication({}) call and that it will be available to restrictToSelf. This is not the case. I noticed there was a commit to fix this for populateUser, can we have one for restrictToSelf as well? :)

I can start trying to submit PRs for some of these, if you don't have the time. I might make mistakes though, so let me know.

@gurisko

This comment has been minimized.

Copy link

gurisko commented Mar 5, 2016

+1 for supporting false for both successRedirect and failureRedirect

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Mar 6, 2016

With the current version, 0.4.1, Local auth won't attempt to redirect (over REST) if you specify an Accept:application/json header.

jQuery.ajax({
    url: "http://api.dev:8080/login",
    type: "POST",
    headers: {
        "Accept": "application/json",
        "Content-Type": "application/json",
    },
    contentType: "application/json",
    data: JSON.stringify({
        "email": "user@feathersjs.com",
        "password": "password"
    })
})
@ArnaudValensi

This comment has been minimized.

Copy link

ArnaudValensi commented Mar 17, 2016

I have the same issue. I think we only needs to disable redirection for local auth, because if we want to use both local auth and facebook/github/.. we stil need successRedirect and failureRedirect.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Mar 18, 2016

I think @ArnaudValensi is right. I started looking at this today. Rather than disable for local auth altogether, I think the best option is to allow people to specify which auth providers they want to use a redirect for success and failure.

The reason being that if you are doing a form post for local auth then it should redirect to your own route.

@ekryski ekryski modified the milestone: 1.0 release Mar 19, 2016

@ekryski ekryski referenced this issue Mar 21, 2016

Merged

v0.6 - Bugs fixes, new hooks, and hook tests #109

10 of 10 tasks complete

@ekryski ekryski closed this in 3efffc5 Mar 24, 2016

@ekryski ekryski modified the milestones: 1.0, 0.6 Mar 26, 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.