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

updated tests/implementations to work with hapi 17 #340

Closed
wants to merge 2 commits into from

Conversation

AVVS
Copy link

@AVVS AVVS commented Dec 31, 2017

Did some refactoring, all tests pass, though I'd clean up code a bit

@metoikos
Copy link

I've tested with Twitter in a live app and worked like a charm.

server.auth.strategy('arcgisonline', 'bell', {
provider: 'arcgisonline',
password: 'cookie_encryption_password_secure',
isSecure: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be true for the purposes of an example IMHO, with a comment that it should only be set to false for development.

Copy link
Author

Choose a reason for hiding this comment

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

Well, I merely updated to async/await, and did some linting (ie top-level await is not yet possible, hence the wrap)

@benatkin
Copy link
Contributor

benatkin commented Jan 2, 2018

I got GitHub OAuth working with this. 👍🏻

@AVVS AVVS mentioned this pull request Jan 3, 2018
@wraithgar
Copy link
Contributor

wraithgar commented Jan 5, 2018

I've got this working happy-path with quite a few providers, including a custom one. However there is a change between this branch and the current bell behavior that I think is worth bringing up.

If the request that bell makes to the temporary url (and probably others, haven't tested those) has an error, the response code and response body from that request will bubble up to the client making the request.

The behavior used to be (correctly I would think) that a 500 error would be sent back to the client.

@marpstar
Copy link

marpstar commented Jan 8, 2018

FWIW, I tested this with LinkedIn tonight and it worked great.

@qraynaud
Copy link

qraynaud commented Jan 19, 2018

Working great with Google too. Thanks for the hard work!

@flippidippi
Copy link
Contributor

Any timeline on when this will be merged? Can't use hapi 17 without bell.

@qraynaud
Copy link

@flipxfx you can just put in your package.json:

"bell": "makeomatic/bell#hapi17"

That will do the trick nicely in the meantime and will allow you to make all the development effort on your side. You can go up to production with this if you are on a small scale project or at least up to staging on anything else. That's still quite a thing. Once done, maintaining a dev branch on v17 until release should be fairly easy and require a minimal amount of work.

@qraynaud
Copy link

I think that @wraithgar is correct about the fact that an error generated by a provider should result on an error 500 and not bubble up to the client.

What are your thoughts on this @AVVS ? And are you willing to fix this if you are in agreement over this ?

@hueniverse hueniverse closed this Feb 26, 2018
@hueniverse hueniverse self-assigned this Feb 26, 2018
@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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants