-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add NodeJS example #1
base: main
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,68 @@ | |||
const cookieParser = require("cookie-parser") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use semicolons at ends of statements for consistency?
const express = require("express") | ||
const session = require("express-session") | ||
const OAuth2Strategy = require("passport-oauth2") | ||
const passport = require("passport") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can/should we use http://www.passportjs.org/docs/openid/ instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The passport-openid library requires the user to enter their OpenID identifier for authentication, not sure if this is the expected user interaction like other examples? I also don't see where it can set client_id/client_secret etc.
I tried to make this NodeJS implementation similar to Django and Flask examples. In those examples, it appeared that they were using the OAuth library, which I was also using OAuth in this NodeJS example.
And I forgot the exact reasons why I ended up not using passport-openid, but I did spend a fair amount of time experimenting passport-openid and decided not to use it.
// Check for environment variables | ||
const variables = ["CLIENT_ID", "CLIENT_SECRET", "SERVER_METADATA_URL"] | ||
variables.forEach((variable) => { | ||
if (!Object.keys(process.env).includes(variable)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use 4 spaces throughout?
|
||
// Configure application | ||
const app = express() | ||
const port = 3000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use 8080 for IDE's sake for now?
app.set("view engine", "ejs") | ||
app.use(cookieParser()); | ||
app.use(session({ | ||
secret: "example-secret", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can Express use file storage instead of signed cookies, to avoid secrets?
tokenURL: config["token_endpoint"], | ||
clientID: process.env["CLIENT_ID"], | ||
clientSecret: process.env["CLIENT_SECRET"], | ||
callbackURL: `http://localhost:${port}/callback` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to avoid hardcoding the host and port, instead letting passport
figure that out so that we need only provide the /callback
path?
fetch(`${config["userinfo_endpoint"]}?access_token=${accessToken}`) | ||
.then(res => res.json()) | ||
.then(json => {return cb(null, json)}) | ||
})) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try using callbacks instead of promises, assuming OpenID library simplifies this too?
"express-session": "^1.17.2", | ||
"node-fetch": "^2.6.1", | ||
"passport": "^0.4.1", | ||
"passport-oauth2": "^1.6.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's update these versions and packages as needed, since it's been a while?
"devDependencies": {}, | ||
"scripts": { | ||
"test": "echo \"Error: no test specified\" && exit 1" | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's omit scripts
as unneeded? And devDependencies
and any other keys that npm
doesn't require?
<title>nodejs</title> | ||
</head> | ||
<body> | ||
<%if (userinfo) {%> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does EJS not recommend spaces after/before %
stylistically?
No description provided.