Skip to content
This repository has been archived by the owner on Oct 26, 2019. It is now read-only.

Add authentication from Auth0 #297

Merged
merged 2 commits into from Oct 19, 2016
Merged

Add authentication from Auth0 #297

merged 2 commits into from Oct 19, 2016

Conversation

danielfrg
Copy link
Collaborator

This PR adds a simple auth from Auth0.

@parente
Copy link
Member

parente commented Oct 16, 2016

Thanks for the contribution. Can you add a note about it on the auth wiki page so folks know how to use it? https://github.com/jupyter-incubator/dashboards_server/wiki/Authentication

@danielfrg
Copy link
Collaborator Author

I don't have permission to edit the wiki. Should I write it in a markdown file? I could move the auth docs from the wiki to the repo and add the auth0 specifics.

@parente
Copy link
Member

parente commented Oct 17, 2016

I don't have permission to edit the wiki.

Oh wow. I thought the wiki used to be public read/write. I guess the default changed?

I could move the auth docs from the wiki to the repo and add the auth0 specifics.

If you're feeling up to the task, that's very generous of you. Alternatively, you can drop the markdown here and I can do it. Or, if you're planning on contributing more over time, we can add you as a committer so you can update the wiki directly.

@danielfrg
Copy link
Collaborator Author

I am not the best node dev but I am planning on using the dashboards more so that will probably include a couple of contributions. So if you can add me as a contributor I can add the docs to the wiki.

@parente
Copy link
Member

parente commented Oct 17, 2016

Invite in the mail.

Copy link
Collaborator

@jhpedemonte jhpedemonte left a comment

Choose a reason for hiding this comment

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

Looks good overall, but haven't tested. Had some minor comments.

passport.authenticate('auth0', { failureRedirect: '/login' }),
function(req, res) {
if (!req.user) {
throw new Error('user null');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Provide a more useful error message here, such as "User name must be set".


app.post('/logout', function(req, res){
req.logout();
res.redirect('https://' + config.get('AUTH0_DOMAIN') + '/v2/logout?returnTo=' + config.get('PUBLIC_LINK_PATTERN') + '&client_id=' + config.get('AUTH0_CLIENT_ID'));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's better to construct URLs using an API rather than string concatenation. In particular, this works around issues where a user may or may not append a '/'.

In other parts of the code, we've used url-join, but not sure how well it handles URL params. If that doesn't work, consider using Node.js' url.format().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I ended up using node url.format as suggested.

@danielfrg
Copy link
Collaborator Author

Invite in the mail.

Thanks! I added a small section to the wiki on Auth0.

Copy link
Collaborator

@jhpedemonte jhpedemonte left a comment

Choose a reason for hiding this comment

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

Thanks, changes look good.

@jhpedemonte jhpedemonte merged commit b72e872 into jupyter:master Oct 19, 2016
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.

None yet

3 participants