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

Refactor SAML auth, make /admin/queues require auth #640

Merged
merged 8 commits into from
Feb 7, 2020

Conversation

humphd
Copy link
Contributor

@humphd humphd commented Feb 2, 2020

Fixes #202

Description

This is a WIP experimental PR to try and get our authentication code working smoothly. I've focused on the node/passport bits, and gotten it to the point that it works in basic HTML. The next step is connecting this into our GatsbyJS app.

Here's some of what I've done:

  • Refactored the code to centralize it, and reduce how much we need
  • Added default values to our env.example for all SAML_* values, so things "Just Work" (@manekenpix is suggesting we try and do this more often, and I think it's a good idea).
  • moved /login to /auth and we now have these routes: /auth/login, /auth/logout, and /auth/callback which enable our SAML SSO flow
  • I've gutted the current homepage and added links for logging in, logging out, and also a link to the Admin Queues area. I've made this so you have to be authenticated to get to it, which lets us test things easily.

I have a bunch of TODOs in the code, which need work. The main thing I'm stuck on is that you should be able to do this:

router.get('/some/route/you/want/authenticated', passport.authenticate('saml'), (req, res) => {
  // This only gets called if you are now authenticated...
  const user = req.user;
  // You can use user now.
});

I have a stupid redirect issue, where it goes back to the homepage instead of falling through to the proper handler.

To test this, pull my branch and do this:

docker-compose up --build

Now go to http://localhost:3000. You can try going to any of the buttons at the top:

Screen Shot 2020-02-01 at 7 44 18 PM

When you're logging in/out, you can look at your cookies:

Screen Shot 2020-02-01 at 7 44 53 PM

@humphd humphd force-pushed the login-experiment branch 2 times, most recently from b2c73c6 to 6beab85 Compare February 2, 2020 03:14

<h1>Telescope</h1>

<p>NOTE: this page is going away soon, just for testing login/logout.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what this is referring too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This entire web page will be replaced by our GatsbyJS app. I just have this here now for testing that the authenticated routes work. I added this note so people didn't wonder why I had this page.

delete req.session.returnTo;
}

res.redirect(returnTo || telescopeHomeUrl);
Copy link
Contributor

Choose a reason for hiding this comment

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

interesting - i was just saying that seneca works needs this ability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's really critical, and you have to hack it in place yourself.

.eslintrc.js Outdated
/**
* Some passport properties are only exposed via underscore names
*/
'no-underscore-dangle': 'off',
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the advantage/disadvantage of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just needed it to get eslint to pass, since I (currently) have to access the logout function I need via passport._strategy and our rules don't allow names with leading _. I might be able to fix this later so I don't need to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general, anything named _name is a "private" variable, and you aren't usually supposed to access it.

@humphd
Copy link
Contributor Author

humphd commented Feb 3, 2020

This is a great, short article on the kind of login flow that I'm using here to protect these api routes: https://auth0.com/docs/login/spa/authenticate-with-cookies

Grommers00
Grommers00 previously approved these changes Feb 4, 2020
@vercel
Copy link

vercel bot commented Feb 4, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/humphd/telescope/hw3b1cxwq
✅ Preview: https://telescope-git-fork-humphd-login-experiment.humphd.now.sh

@humphd humphd added this to In progress in 0.6 Release: Grade A Dogfood via automation Feb 6, 2020
@humphd humphd self-assigned this Feb 6, 2020
@humphd humphd added area: sso Authentication area: web server Issues related to the web server labels Feb 6, 2020
@c3ho c3ho self-requested a review February 6, 2020 21:35
@Grommers00 Grommers00 self-requested a review February 6, 2020 22:54
@humphd humphd merged commit a37dd06 into Seneca-CDOT:master Feb 7, 2020
0.6 Release: Grade A Dogfood automation moved this from In progress to Done Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: sso Authentication area: web server Issues related to the web server
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Restrict access to admin route
3 participants