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

Very WIP feat(passportjs): example using passport #2210

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

easherma
Copy link
Collaborator

No description provided.

Copy link
Collaborator Author

@easherma easherma left a comment

Choose a reason for hiding this comment

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

left comments for myself

@@ -8,7 +8,6 @@ if (process.env.NEW_RELIC_LICENSE_KEY) {

const compression = require('compression')
const cookieParser = require('cookie-parser')
const cookieSession = require('cookie-session')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

express vs cookieSession:
express session seems more storage agnostic? Its used by several examples; not sure that examples couldn't be adapted to use what we already have, though

Copy link
Member

Choose a reason for hiding this comment

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

cookie-session was definitely in the example usage in the past. If this had changed in more recent years, we can consider updating (in a separate PR)

@@ -199,6 +238,7 @@ app.get('/terms-of-service', (req, res) => res.render('tos'))
// API routes
app.use('', apiRoutes)
app.use('', serviceRoutes)
app.use('/', authRouter)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

pretty temporary, though I'd also like to learn more about the way our current implementation of routes differs from many express examples I've seen. not a problem to conform to what we have already, though.

Copy link
Member

Choose a reason for hiding this comment

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

Can you say more about this?

Copy link
Member

Choose a reason for hiding this comment

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

The general rule of thumb is that our implementation may differ from current examples because the conventions change over time. We are not necessarily following every convention change closely, and updating them to match. This is especially true when old patterns are not explicitly deprecated, or if we don't have good reasons (beyond style) to change.

@@ -0,0 +1,63 @@
var express = require('express')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is 'boilerplate' but basically where the redirect happens.
at the moment, this goes to the auth0 login default, and so in effect isn't much different than #2035

I'd like to play around with this more to see if we can use passport with auth0 but wrapped around our existing login embed form.

The advantage here might be more when we want to add additional providers like coil, etc. We have a pattern or strategy to follow.

@louh louh temporarily deployed to streetmix-easherma-pass-kvxfwn January 22, 2021 19:29 Inactive
@@ -34,6 +38,43 @@ compileSVGSprites(
'image'
)

// config express-session
const theSession = {
secret: '72a1e6ba625661ee10dfd9610abb5499acc276da06375e56ed2d3789d0aa11ae',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is, i think, a locally generated token so I don't know if it needs to not be committed like other secrets

@louh
Copy link
Member

louh commented Jan 28, 2021

@easherma Is this superceded by #2214?

@easherma
Copy link
Collaborator Author

Not really. It was my first stab at getting passport to work with auth0 in order to add support for username password login

Base automatically changed from master to main February 17, 2021 18:44
@codecov
Copy link

codecov bot commented Mar 17, 2021

Codecov Report

Merging #2210 (06359c0) into main (51112eb) will decrease coverage by 0.17%.
The diff coverage is 0.00%.

❗ Current head 06359c0 differs from pull request most recent head 659394b. Consider uploading reports for the commit 659394b to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2210      +/-   ##
==========================================
- Coverage   47.81%   47.63%   -0.18%     
==========================================
  Files         267      268       +1     
  Lines        9002     9036      +34     
  Branches     2173     2178       +5     
==========================================
  Hits         4304     4304              
- Misses       4209     4238      +29     
- Partials      489      494       +5     
Impacted Files Coverage Δ
app/auth_routes.js 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51112eb...659394b. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants