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

Expose query in spite of errors #295

Closed
sholladay opened this issue Feb 13, 2017 · 4 comments
Closed

Expose query in spite of errors #295

sholladay opened this issue Feb 13, 2017 · 4 comments
Assignees
Labels
Milestone

Comments

@sholladay
Copy link
Contributor

@sholladay sholladay commented Feb 13, 2017

The query feature is awesome as it allows me to return the user to where they came from after logging in.

However, because bell attaches it it to request.auth.credentials, and credentials are only available if there are no errors from bell, the end result is that I cannot help them go back if authentication fails for any reason.

Here is an example flow that would be a great user experience:

  1. Client requests GET /login?next=%2Fmy-page
  2. OAuth dance...
  3. Server responds 302 Found (Location: /my-page)

And if there is an error during step 2, the Location header would have something like ?auth_error=msg appended to it so that /my-page could render a nice pretty error for the user while allowing them to resume whatever they were doing.

Currently, the non-error case is easily achievable. But in the case where there is an error, my auth route has no knowledge of /my-page and so has nothing to redirect to or append to.

By always exposing the query object, we can achieve the above flow even in the error case and provide a much better user experience.

@ldesplat

This comment has been minimized.

Copy link
Contributor

@ldesplat ldesplat commented Feb 13, 2017

Yes, you are right we should. Looking at https://hapijs.com/api#serverauthschemename-scheme it seems like we should be able to return the credentials object anyways when we hit an error rather than creating a new request.auth key which is more owned by Hapi than us. Remember that the presence of the credentials object does not mean that request.auth.isAuthenticated === true.

The docs are a bit unclear, but looks like it should be possible. That's a great request. If you can implement it then we'll make sure it gets properly reviewed in time. I know I am currently behind on quite a few PRs!

@sholladay

This comment has been minimized.

Copy link
Contributor Author

@sholladay sholladay commented Feb 13, 2017

Looking at https://hapijs.com/api#serverauthschemename-scheme it seems like we should be able to return the credentials object anyways when we hit an error

Definitely. Although, this could be considered semver major depending on how you look at it. Maybe not because...

Remember that the presence of the credentials object does not mean that request.auth.isAuthenticated === true.

You are absolutely right. The docs are clear that isAuthenticated is the canonical way. I had a hard time wording what I meant about that. Just changed it to say "credentials are only available if there are no errors from bell".

rather than creating a new request.auth key which is more owned by Hapi than us

Is that happening? I looked through the code a bit and couldn't find anything like that. But I may be missing something amidst all of the machinery. Not even sure how I would set anything on request.auth beyond credentials and artifacts.

@ldesplat

This comment has been minimized.

Copy link
Contributor

@ldesplat ldesplat commented Feb 14, 2017

Sounds like we're saying close to the same thing. But I want to be sure

By moving request.auth.credentials.query to request.auth.query, we can achieve the above flow even in the error case and provide a much better user experience.

I am proposing that we can implement the return of request.auth.credentials.query on errors since it appears supported by the Hapi API.

I think it would be a semver minor change, but Bell has lots of instances where it needs to raise the major so I can make proper release notes spelling out that point very clearly that request.auth.credentials presence does not mean something is authenticated.

Though I have to say, we may have made a mistake returning the query in credentials rather than artifacts. It seems like we mixed up the concepts but it's probably not important enough for a real breaking change here.

@ldesplat ldesplat added the feature label Feb 14, 2017
@sholladay

This comment has been minimized.

Copy link
Contributor Author

@sholladay sholladay commented Feb 14, 2017

I am proposing that we can implement the return of request.auth.credentials.query on errors

Yeah, that's fine too. I care more about being able to access it than where exactly it lives. Though it does feel strange to refer to it as a credential. And I support putting it on artifacts. 👍

@sholladay sholladay changed the title Move query to request.auth Expose query in spite of errors Feb 14, 2017
@hueniverse hueniverse self-assigned this Feb 26, 2018
@hueniverse hueniverse added this to the 9.0.1 milestone Feb 26, 2018
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.