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

Add support for authentication with Okta #299

Merged
merged 2 commits into from Mar 24, 2017

Conversation

@jsatk
Copy link
Contributor

jsatk commented Feb 27, 2017

I need support for Okta with Bell. I've currently forked it and have patched it in, but I think this would be useful for the mainline bell release.

Our use of Okta also depends on exposing the payload. It appears that that was already done in this PR. #273

I'd love to see #273 land and then this.

I tried my best to follow all current style and best practices in Bell. Please @ldesplat let me know if you have any requests, feedback, etc.

@@ -1,36 +1,37 @@
'use strict';

exports = module.exports = {
arcgisonline: require('./arcgisonline'),

This comment has been minimized.

Copy link
@AdriVanHoudt

AdriVanHoudt Feb 27, 2017

Contributor

This makes my brain smile 👌

@jsatk

This comment has been minimized.

Copy link
Contributor Author

jsatk commented Mar 8, 2017

@AdriVanHoudt @ldesplat would love to see this merged or get the proper feedback to get this to a mergeable state so I can drop my private fork of this repo. Thanks!

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Mar 8, 2017

@jsatk

This comment has been minimized.

Copy link
Contributor Author

jsatk commented Mar 9, 2017

Thanks for the feedback @AdriVanHoudt. Will add tomorrow!

@jsatk jsatk force-pushed the jsatk:add-support-for-okta branch from f3117e9 to 4522d41 Mar 9, 2017
@jsatk

This comment has been minimized.

Copy link
Contributor Author

jsatk commented Mar 9, 2017

@AdriVanHoudt Just added documentation to https://github.com/hapijs/bell/blob/master/Providers.md,

Please let me know if there is anything else. Thanks!

Copy link
Contributor

AdriVanHoudt left a comment

LGTM with 1 question


const results = Joi.validate(options, internals.schema);
Hoek.assert(!results.error, results.error);
const settings = results.value;

This comment has been minimized.

Copy link
@AdriVanHoudt

AdriVanHoudt Mar 10, 2017

Contributor

Should you not assert there is an uri since you do not provide a default?

This comment has been minimized.

Copy link
@jsatk

jsatk Mar 10, 2017

Author Contributor

This is following a very similar pattern as https://github.com/hapijs/bell/blob/master/lib/providers/auth0.js#L12.

It was my understanding that line 17 using Joi.validate would ensure folks pass in the expected schema.

This comment has been minimized.

Copy link
@AdriVanHoudt

AdriVanHoudt Mar 13, 2017

Contributor

Oh I totally missed the internals.schema having uri in it 🙄

@jsatk

This comment has been minimized.

Copy link
Contributor Author

jsatk commented Mar 10, 2017

@AdriVanHoudt Awesome. Thanks so much for the review. I responded to your comment. Please let me know if there are any further questions.

And again, this PR requires #273 to go first and therefore is blocked by #273 merging.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Mar 13, 2017

Well the dependency on #273 is just specific for your team no? I don't see a technical dependency?

@ldesplat ldesplat added the feature label Mar 13, 2017
@ldesplat ldesplat added this to the 8.6.0 milestone Mar 13, 2017
@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Mar 13, 2017

Thanks for the PR @jsatk and for the review @AdriVanHoudt

I'll make a release later this week with #273 as well.

@jsatk

This comment has been minimized.

Copy link
Contributor Author

jsatk commented Mar 13, 2017

Thanks so much @ldesplat.

@jsatk

This comment has been minimized.

Copy link
Contributor Author

jsatk commented Mar 13, 2017

@AdriVanHoudt good point. You're correct. I'm thinking through the lense of what my team needs. 😂

@jsatk

This comment has been minimized.

Copy link
Contributor Author

jsatk commented Mar 17, 2017

Hi @ldesplat & @AdriVanHoudt. Any update on when the 8.6.0 release will happen? Thanks!

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Mar 18, 2017

@ldesplat said later this week so you'll have to wait until he has some time to do it, that is how OSS works ;)

@jsatk

This comment has been minimized.

Copy link
Contributor Author

jsatk commented Mar 19, 2017

@AdriVanHoudt totally cool. Just asking. Thanks.

@ldesplat ldesplat merged commit 6effe33 into hapijs:master Mar 24, 2017
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
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.