Use Auth0 hosted login page. #12

xoen · 2018-02-13T10:29:50Z

What

This should also mean a user logged in other part of the platform
will be already logged in (SSO)

Refactorings (or lack of)

I'm not a great fan of massive refactoring. I focused on adding the feature.

The only thing I tweaked was adding a bit more logging to the middleware module which I also renamed to authorisation as that's what it does so it should be named accordingly.

Ticket

https://trello.com/c/t90fEmBs/647-4-update-rstudio-auth-proxy-to-use-hosted-auth0-login

This should also mean a user logged in other part of the platform will be already logged in (SSO) Ticket: https://trello.com/c/t90fEmBs/647-4-update-rstudio-auth-proxy-to-use-hosted-auth0-login

xoen · 2018-02-13T10:31:28Z

app/config.js

 clientID: process.env.AUTH0_CLIENT_ID,
 clientSecret: process.env.AUTH0_CLIENT_SECRET,
- callbackURL: process.env.AUTH0_CALLBACK_URL || 'http:https://localhost:3000/callback'
+ domain: process.env.AUTH0_DOMAIN,
+ scope: 'openid profile',


Added scope and passReqToCallback here. Sorted the keys.

This module was dealing with authorisation, so it should be named accordingly.

We only use the user nickname to check if it's authorised to access RStudio. This field should come from from the `profile` scope. SEE: https://auth0.com/docs/scopes/current

andyhd

Looks good, just minor comments

andyhd · 2018-02-14T12:02:25Z

app/authorisation.js

 } else {
 res.sendStatus(403);
 next(new Error('User is not authorized'));
 }
 };

-function isAuthorized(req) {
+function _isAuthorized(req) {


It doesn't make sense to prefix Javascript functions with _ - that is a Python idiom indicating that a function or method is intended to be private.
All functions in Javascript are private to the module unless explicitly exported.

Does this pass eslint?

We don't have eslint running in this project. I'll add the lint task and see what I can savage.

andyhd · 2018-02-14T12:03:08Z

app/authorisation.js

 } catch (error) {
- // do nothing
+ log.error('Error while checking user nickname: ', error)


Missing semicolon

Does this output anything useful? You might need to do JSON.stringify(error)

I tested this yesterday by manually throw a weird json-y exception and bole escaped it correctly (which is great). I think log this is not a bad idea as this is kind of a programming error (e.g. the token doesn't include the nickname field).

andyhd · 2018-02-14T12:06:37Z

app/index.js

@@ -25,12 +25,15 @@ nunjucks.configure(path.join(__dirname, 'views'), {
 app.use(require('cookie-parser')());
 app.use(require('express-session')(config.session));

-passport.use(new Auth0Strategy(
+strategy = new Auth0Strategy(


This sould be const strategy = or at least var strategy =

Good point 👍

andyhd · 2018-02-14T12:11:51Z

app/routes.js

@@ -1,36 +1,52 @@
 var config = require('./config');
 var ensureLoggedIn = require('connect-ensure-login').ensureLoggedIn;
 var express = require('express');
-var middleware = require('./middleware');
+var authorisation = require('./authorisation');


This should be spelled authorization. The concept of authorization in technology is abbreviated to "authz", and it is already spelled -ize in existing code in middleware.js.

This always confused me as in American they use zs but in UK we use ses.

In the company were I used to work they preferred ses because "we're in UK innit?" - should we stick with the American spelling or the British one? (not that I mind either way)

I'll change to the z here then.

If it was another word, I don't think I would have mentioned it. But authorization is AuthZ (and authentication is AuthN), so I think in this case it's worth being consistent and using the z spelling.

andyhd · 2018-02-14T12:23:57Z

app/routes.js

+ }
+ } else {
+ passport.authenticate('auth0-oidc', {
+ state: uuidv4(),


Does state need to be passed here? If passport does not generate a state value by default, does it check one if supplied? Otherwise, the UUID should be stored in the session and checked in the callback.

OK, tested without the state and seems to work so I'll remove this and the dependency (hopefully we'll not regret this :D)

andyhd · 2018-02-14T12:26:41Z

app/routes.js

+}
+
+router.get('/logout', logout);
+router.get('/auth-sign-out', logout);


Instead of this, you can do

router.get(['/logout', '/auth-sign-out'], (req, res) => { ... });

(also using american spelling consistently)

xoen · 2018-02-14T15:49:08Z

@andyhd I made few changes to address your comments, please have another look, focus particularly on the changes in the last 9 commits (a lot of noise as I linted some of the files). Cheers

This is necessary as we're starting to use ES6 for some of the code.

Use Auth0 hosted login page.

f50b600

This should also mean a user logged in other part of the platform will be already logged in (SSO) Ticket: https://trello.com/c/t90fEmBs/647-4-update-rstudio-auth-proxy-to-use-hosted-auth0-login

xoen added the wip label Feb 13, 2018

xoen commented Feb 13, 2018

View reviewed changes

xoen added 3 commits February 13, 2018 10:52

Renamed 'middleware' module to 'authorisation' and better logging

2c82519

This module was dealing with authorisation, so it should be named accordingly.

Updated Auth0 scopes: Only 'profile' should be required

9d41ab8

We only use the user nickname to check if it's authorised to access RStudio. This field should come from from the `profile` scope. SEE: https://auth0.com/docs/scopes/current

Auth0 Login hosted page logout

df308e3

xoen force-pushed the ag--auth0-hosted-login-page branch from 16488d6 to df308e3 Compare February 14, 2018 10:52

xoen removed the wip label Feb 14, 2018

xoen requested review from axemonkey, andyhd and kerin February 14, 2018 11:53

andyhd reviewed Feb 14, 2018

View reviewed changes

xoen added 9 commits February 14, 2018 14:23

Added lint task to manually run ESLint

dff1ea2

Removed unused 'nunjucks' and views/layout.html

d92bc20

Linted 'authorization' middleware and moved under /app/middleware

faf39ba

(also using american spelling consistently)

Linted app/index.js

a98cf47

Linted app/config.js

707b4ce

Linted app/routes.js (mostly)

b7cb8f5

Linted app.errors.js

cf91f69

Removed (apparently) unnecessary 'state' param when authenticating

236e725

PR amendment: Compact way of defining logouts routes

5b3243b

Upgrade node to version 8.9 (LTS)

da103c9

This is necessary as we're starting to use ES6 for some of the code.

andyhd approved these changes Feb 15, 2018

View reviewed changes

xoen merged commit 333c205 into master Feb 16, 2018

xoen deleted the ag--auth0-hosted-login-page branch February 16, 2018 09:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Auth0 hosted login page. #12

Use Auth0 hosted login page. #12

xoen commented Feb 13, 2018 •

edited

Loading

xoen Feb 13, 2018 •

edited

Loading

andyhd left a comment

andyhd Feb 14, 2018

andyhd Feb 14, 2018

xoen Feb 14, 2018

andyhd Feb 14, 2018

andyhd Feb 14, 2018

xoen Feb 14, 2018

andyhd Feb 14, 2018

andyhd Feb 14, 2018

xoen Feb 14, 2018

andyhd Feb 14, 2018

xoen Feb 14, 2018

andyhd Feb 14, 2018

andyhd Feb 14, 2018

xoen Feb 14, 2018

andyhd Feb 14, 2018

xoen commented Feb 14, 2018

Use Auth0 hosted login page. #12

Use Auth0 hosted login page. #12

Conversation

xoen commented Feb 13, 2018 • edited Loading

What

Refactorings (or lack of)

Ticket

xoen Feb 13, 2018 • edited Loading

Choose a reason for hiding this comment

andyhd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xoen commented Feb 14, 2018

xoen commented Feb 13, 2018 •

edited

Loading

xoen Feb 13, 2018 •

edited

Loading