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

Respect auth mode for redirects #186

Merged
merged 5 commits into from
May 27, 2018
Merged

Conversation

sholladay
Copy link
Contributor

This is a redo of PR #155, updated for hapi 17, and with no merge conflicts. I made this a new PR for a blank slate and so that the other PR can remain discoverable for hapi 16 users.

The goal here is to reduce boilerplate for configuring this plugin and to simplify routes.

hapi-auth-cookie has a strange quirk where it redirects to login when the user visits a route whose auth mode is try or optional. The reason this is unexpected is that the whole point of hapi's try and optional modes is to allow route handlers to process unauthenticated requests. hapi-auth-cookie makes this impossible, because it redirects the user away, and thus the route handler never executes. This is rather frustrating because these routes are designed to not force the user to login.

This PR ensures that redirectTo respects the auth mode set for the route. It only redirects when the auth mode is required. You can still override the behavior in the route configuration, but that is the new default. This is a breaking change, but it would really make my day to see this merged, since I use optional routes a lot.

@mrlannigan mrlannigan added feature New functionality or improvement breaking changes Change that can breaking existing code labels Jan 21, 2018
@mrlannigan mrlannigan added this to the 9.0.0 milestone Jan 21, 2018
@sholladay
Copy link
Contributor Author

sholladay commented Mar 25, 2018

@mrlannigan I fixed the merge conflicts caused by #183 going in first.

Is there anything else I can do to help get this merged? I sincerely believe that this change will be very popular with the rest of the community.

@ilyaigpetrov
Copy link

This PR is extremely desired, please, don't put it off.
@sholladay thank you for your work, I plan to use your sholladay/hapi-auth-cookie#hapi17-redirect in my package.json until it will be merged.

Copy link
Contributor

@mrlannigan mrlannigan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you again for this! I apologize for the delay.

@mrlannigan mrlannigan merged commit ca2e2d5 into hapijs:master May 27, 2018
@mrlannigan
Copy link
Contributor

Version 9.0.0 has been released to npm.

@sholladay sholladay deleted the hapi17-redirect branch June 28, 2018 18:04
@lock
Copy link

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking changes Change that can breaking existing code feature New functionality or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants