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

Fixes #266 Implements azure active directory as bell azuread module #267

Merged
merged 1 commit into from Oct 4, 2016

Conversation

@sudheesh001
Copy link
Contributor

sudheesh001 commented Sep 23, 2016

Signed off by: Sudheesh Singanamalla susingan@microsoft.com

@ldesplat ldesplat added this to the 9.0.0 milestone Sep 25, 2016
@ldesplat ldesplat added the feature label Sep 25, 2016
@kamronbatman

This comment has been minimized.

Copy link
Contributor

kamronbatman commented Sep 26, 2016

@sudheesh001 Thank you so much for the contribution!

I know it is minor, but can you please fix the linting?

Linting results:
    /home/travis/build/hapijs/bell/lib/providers/azuread.js:
        Line 3: hapi/hapi-scope-start - Missing blank line at beginning of function.
        Line 10: space-infix-ops - Infix operators must be spaced.
        Line 10: space-infix-ops - Infix operators must be spaced.
        Line 11: space-infix-ops - Infix operators must be spaced.
        Line 11: space-infix-ops - Infix operators must be spaced.
        Line 15: space-infix-ops - Infix operators must be spaced.
        Line 15: space-infix-ops - Infix operators must be spaced.

Thanks!

@sudheesh001 sudheesh001 force-pushed the sudheesh001:AzureADIntegration branch from 2ad124f to 5a3b74c Sep 27, 2016
@sudheesh001

This comment has been minimized.

Copy link
Contributor Author

sudheesh001 commented Sep 27, 2016

@kamronbatman Sure, I've fixed the linting issues now. 😄

@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Sep 27, 2016

Thank you! Now, once you understand how the tests are done, I think you could make this PR 100% good to go :)

See https://github.com/hapijs/bell/blob/master/test/providers/google.js#L45 as a reference. You will see I've highlighted Line 45 which basically tells you that we mock all the responses. This is done because the tests run fast & more importantly we don't have a proper way to store & share secrets so that we can run our tests directly against the providers.

So the gist of it is copy another test for one of the providers. Change the profile mock to reflect what azuread would respond with and then https://github.com/hapijs/bell/blob/master/test/providers/google.js#L45 change what we should be expecting as results.
Make sure to also change this line to your provider: https://github.com/hapijs/bell/blob/master/test/providers/google.js#L34

That's it. That usually covers everything. I don't see any if checks in your code so 1 test will suffice.

Signed off by: Sudheesh Singanamalla <susingan@microsoft.com>
@sudheesh001 sudheesh001 force-pushed the sudheesh001:AzureADIntegration branch from 5a3b74c to 9e61294 Sep 29, 2016
@sudheesh001

This comment has been minimized.

Copy link
Contributor Author

sudheesh001 commented Sep 29, 2016

@ldesplat Thanks, having a look at the google test made it clear on how the tests are happening. I've implemented the tests for the azuread integration and this pull request is now hopefully complete. Please review. 😄

@sudheesh001

This comment has been minimized.

Copy link
Contributor Author

sudheesh001 commented Oct 4, 2016

@kamronbatman @ldesplat ; ping 😄 Did you have a chance to look through this ?

@ldesplat ldesplat merged commit a44e5fb into hapijs:master Oct 4, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ldesplat ldesplat modified the milestones: 9.0.0, 8.3.0 Oct 4, 2016
@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Oct 4, 2016

Thank you for your work on this. Making the 8.3.0 release right now.

I schedule 1 hour, two days a week, to work on Bell but depending on other priorities it may get bumped! 💯

@wy193777

This comment has been minimized.

Copy link
Contributor

wy193777 commented Oct 20, 2017

When users wants to use this, just put azuread on provide field like bellow? The list of provides on API.md doesn't include azuread, so I'm not sure how to use it.

server.auth.strategy('azure-ad', 'bell', {
        provider: 'azuread',
        password: 'cookie_encryption_password_secure',
        clientId: 'my_azure_client_id',
        clientSecret: 'my_azure_client_secret',
        isSecure: false     // Terrible idea but required if not using HTTPS especially if developing locally
    });
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.