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

Try mode #112

Merged
merged 3 commits into from
Oct 17, 2015
Merged

Try mode #112

merged 3 commits into from
Oct 17, 2015

Conversation

nelsonic
Copy link
Member

Adds regression test for try and optional auth modes.
see: #111
Without the lines:

if (!token && request.auth.mode !== 'required') {       
   return reply.continue({ credentials: {} });      
}

it actually works exactly as expected.
Confirmed by the master in: hapijs/hapi#2843

@alanshaw I took the regression tests directly from your branch: alanshaw@736067f
please checkout this branch and confirm it works as expected.
Thanks. 👍

@nelsonic
Copy link
Member Author

@alanshaw when you get a chance please confirm this works for you. thanks! 👍

@alanshaw
Copy link
Member

LGTM - the tests in my application pass with 5.1.0, fail with 5.1.1 and pass again using this branch.

@nelsonic
Copy link
Member Author

@alanshaw thanks for confirming that, are you happy for us to 🚢 this branch as the new version of the plugin?

@alanshaw
Copy link
Member

Please do!

@nelsonic nelsonic assigned iteles and unassigned alanshaw Oct 17, 2015
@nelsonic
Copy link
Member Author

@iteles this fix has been approved by @alanshaw.
please review/merge when you get back to your desk
and let me know when I can npm publish. thanks. 👍

iteles added a commit that referenced this pull request Oct 17, 2015
@iteles iteles merged commit de2a6a3 into master Oct 17, 2015
@nelsonic nelsonic deleted the try-mode branch October 17, 2015 11:58
@iteles
Copy link
Member

iteles commented Oct 17, 2015

Done! :shipit:

@nelsonic
Copy link
Member Author

Shipped. :shipit:
@alanshaw 5.1.2 is on NPM.
Thanks again! 👍

@alanshaw
Copy link
Member

plz check your gitter messages

@nelsonic
Copy link
Member Author

@alanshaw sec report filed. thanks for the advice. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants