From 2c90bba774ac107e4bdaa38248803c90083cde3c Mon Sep 17 00:00:00 2001 From: lukeIam <2lukeiam@gmail.com> Date: Wed, 20 Sep 2023 18:37:55 +0100 Subject: [PATCH] small refactorings --- server/Auth.js | 57 ++++++++++++++++++++++++++++----------- server/Server.js | 7 +++-- server/SocketAuthority.js | 29 +++++--------------- 3 files changed, 51 insertions(+), 42 deletions(-) diff --git a/server/Auth.js b/server/Auth.js index 17fea247..d68ef876 100644 --- a/server/Auth.js +++ b/server/Auth.js @@ -17,10 +17,10 @@ class Auth { } /** - * Inializes all passportjs stragegies and other passportjs ralated initialization. + * Inializes all passportjs strategies and other passportjs ralated initialization. */ async initPassportJs() { - // Check if we should load the local strategy + // Check if we should load the local strategy (username + password login) if (global.ServerSettings.authActiveAuthMethods.includes("local")) { passport.use(new LocalStrategy(this.localAuthCheckUserPw.bind(this))) } @@ -33,13 +33,17 @@ class Auth { callbackURL: global.ServerSettings.authGoogleOauth20CallbackURL }, (async function (accessToken, refreshToken, profile, done) { // TODO: do we want to create the users which does not exist? + + // get user by email const user = await Database.userModel.getUserByEmail(profile.emails[0].value.toLowerCase()) if (!user || !user.isActive) { + // deny login done(null, null) return } + // permit login return done(null, user) }).bind(this))) } @@ -59,17 +63,23 @@ class Auth { }, (function (issuer, profile, done) { // TODO: do we want to create the users which does not exist? + + // get user by email var user = Database.userModel.getUserByEmail(profile.emails[0].value.toLowerCase()) if (!user || !user.isActive) { + // deny login done(null, null) return } + // permit login return done(null, user) }).bind(this))) } + // should be already initialied here - but ci had some problems so check again + // token is required to encrypt/protect the info in jwts if (!global.ServerSettings.tokenSecret) { await this.initTokenSecret() } @@ -83,22 +93,19 @@ class Auth { // define how to seralize a user (to be put into the session) passport.serializeUser(function (user, cb) { process.nextTick(function () { - // only store username and id to session - // TODO: do we want to store more info in the session? + // only store id to session return cb(null, JSON.stringify({ - "username": user.username, "id": user.id, - "email": user.email, })) }) }) - // define how to deseralize a user (use the username to get it from the database) + // define how to deseralize a user (use the ID to get it from the database) passport.deserializeUser((function (user, cb) { process.nextTick((async function () { const parsedUserInfo = JSON.parse(user) - // TODO: do the matching on username or better on id? - const dbUser = await Database.userModel.getUserByUsername(parsedUserInfo.username.toLowerCase()) + // load the user by ID that is stored in the session + const dbUser = await Database.userModel.getUserById(parsedUserInfo.id) return cb(null, dbUser) }).bind(this)) }).bind(this)) @@ -110,23 +117,28 @@ class Auth { * @param {*} res Response object. */ paramsToCookies(req, res) { - if (req.query.isRest && (req.query.isRest.toLowerCase() == "true" || req.query.isRest.toLowerCase() == "false")) { + if (req.query.isRest && req.query.isRest.toLowerCase() == "true") { + // store the isRest flag to the is_rest cookie res.cookie('is_rest', req.query.isRest.toLowerCase(), { maxAge: 120000 * 120, // Hack - this semms to be in UTC?? httpOnly: true }) } else { + // no isRest-flag set -> set is_rest cookie to false res.cookie('is_rest', "false", { maxAge: 120000 * 120, // Hack - this semms to be in UTC?? httpOnly: true }) + + // check if we are missing a callback parameter - we need one if isRest=false if (!req.query.callback || req.query.callback === "") { res.status(400).send({ message: 'No callback parameter' }) return } + // store the callback url to the auth_cb cookie res.cookie('auth_cb', req.query.callback, { maxAge: 120000 * 120, // Hack - this semms to be in UTC?? httpOnly: true @@ -142,6 +154,7 @@ class Auth { * @param {*} res Response object. */ async handleLoginSuccessBasedOnCookie(req, res) { + // get userLogin json (information about the user, server and the session) const data_json = await this.getUserLoginResponsePayload(req.user) if (req.cookies.is_rest && req.cookies.is_rest === "true") { @@ -152,7 +165,7 @@ class Auth { // UI request -> check if we have a callback url // TODO: do we want to somehow limit the values for auth_cb? if (req.cookies.auth_cb && req.cookies.auth_cb.startsWith("http")) { - // UI request -> redirect + // UI request -> redirect to auth_cb url and send the jwt token as parameter res.redirect(302, `${req.cookies.auth_cb}?setToken=${data_json.user.token}`) } else { @@ -165,7 +178,7 @@ class Auth { * Creates all (express) routes required for authentication. * @param {express.Router} router */ - initAuthRoutes(router) { + async initAuthRoutes(router) { // Local strategy login route (takes username and password) router.post('/login', passport.authenticate('local'), (async function (req, res) { @@ -177,6 +190,7 @@ class Auth { // google-oauth20 strategy login route (this redirects to the google login) router.get('/auth/google', (req, res, next) => { const auth_func = passport.authenticate('google', { scope: ['email'] }) + // params (isRest, callback) to a cookie that will be send to the client this.paramsToCookies(req, res) auth_func(req, res, next) }) @@ -184,12 +198,14 @@ class Auth { // google-oauth20 strategy callback route (this receives the token from google) router.get('/auth/google/callback', passport.authenticate('google'), + // on a successfull login: read the cookies and react like the client requested (callback or json) this.handleLoginSuccessBasedOnCookie.bind(this) ) // openid strategy login route (this redirects to the configured openid login provider) router.get('/auth/openid', (req, res, next) => { const auth_func = passport.authenticate('openidconnect') + // params (isRest, callback) to a cookie that will be send to the client this.paramsToCookies(req, res) auth_func(req, res, next) }) @@ -197,6 +213,7 @@ class Auth { // openid strategy callback route (this receives the token from the configured openid login provider) router.get('/auth/openid/callback', passport.authenticate('openidconnect'), + // on a successfull login: read the cookies and react like the client requested (callback or json) this.handleLoginSuccessBasedOnCookie.bind(this) ) @@ -239,7 +256,7 @@ class Auth { } /** - * Function to generate a jwt token for a given user. + * Function to validate a jwt token for a given user. * @param {string} token * @returns the tokens data. */ @@ -253,7 +270,7 @@ class Auth { } /** - * Generate a token for each user. + * Generate a token which is used to encrpt/protect the jwts. */ async initTokenSecret() { if (process.env.TOKEN_SECRET) { // User can supply their own token secret @@ -279,12 +296,15 @@ class Auth { * @param {function} done */ jwtAuthCheck(jwt_payload, done) { - const user = Database.userModel.getUserByUsername(jwt_payload.username.toLowerCase()) + // load user by id from the jwt token + const user = Database.userModel.getUserById(jwt_payload.id) if (!user || !user.isActive) { + // deny login done(null, null) return } + // approve login done(null, user) return } @@ -296,6 +316,7 @@ class Auth { * @param {function} done */ async localAuthCheckUserPw(username, password, done) { + // Load the user given it's username const user = await Database.userModel.getUserByUsername(username.toLowerCase()) if (!user || !user.isActive) { @@ -306,9 +327,11 @@ class Auth { // Check passwordless root user if (user.id === 'root' && (!user.pash || user.pash === '')) { if (password) { + // deny login done(null, null) return } + // approve login done(null, user) return } @@ -316,9 +339,11 @@ class Auth { // Check password match const compare = await bcrypt.compare(password, user.pash) if (compare) { + // approve login done(null, user) return } + // deny login done(null, null) return } @@ -343,7 +368,7 @@ class Auth { /** * Return the login info payload for a user. * @param {string} username - * @returns {string} jsonPayload + * @returns {Promise} jsonPayload */ async getUserLoginResponsePayload(user) { const libraryIds = await Database.libraryModel.getAllLibraryIds() diff --git a/server/Server.js b/server/Server.js index 7cc6fa76..cf55061d 100644 --- a/server/Server.js +++ b/server/Server.js @@ -87,6 +87,7 @@ class Server { } authMiddleware(req, res, next) { + // ask passportjs if the current request is authenticated this.auth.isAuthenticated(req, res, next) } @@ -145,7 +146,7 @@ class Server { resave: false, saveUninitialized: false, cookie: { - // also send the cookie if were hare not on https + // also send the cookie if were are not on https (not every use has https) secure: false }, })) @@ -155,8 +156,6 @@ class Server { app.use(passport.session()) // config passport.js await this.auth.initPassportJs() - // use auth on all routes - not used now - // app.use(passport.authenticate('session')) const router = express.Router() app.use(global.RouterBasePath, router) @@ -200,7 +199,7 @@ class Server { }) // Auth routes - this.auth.initAuthRoutes(router) + await this.auth.initAuthRoutes(router) // Client dynamic routes const dyanimicRoutes = [ diff --git a/server/SocketAuthority.js b/server/SocketAuthority.js index 81136ecf..28e59e40 100644 --- a/server/SocketAuthority.js +++ b/server/SocketAuthority.js @@ -2,8 +2,6 @@ const SocketIO = require('socket.io') const Logger = require('./Logger') const Database = require('./Database') const Auth = require('./Auth') -const passport = require('passport') -const expressSession = require('express-session') class SocketAuthority { constructor() { @@ -85,23 +83,6 @@ class SocketAuthority { } }) - /* - const wrap = middleware => (socket, next) => middleware(socket.request, {}, next); - - io.use(wrap(expressSession({ - secret: global.ServerSettings.tokenSecret, - resave: false, - saveUninitialized: false, - cookie: { - // also send the cookie if were hare not on https - secure: false - }, - }))); - - io.use(wrap(passport.initialize())); - io.use(wrap(passport.session())); - */ - this.io.on('connection', (socket) => { this.clients[socket.id] = { id: socket.id, @@ -168,14 +149,18 @@ class SocketAuthority { // When setting up a socket connection the user needs to be associated with a socket id // for this the client will send a 'auth' event that includes the users API token async authenticateSocket(socket, token) { - // TODO + // we don't use passport to authenticate the jwt we get over the socket connection. + // it's easier to directly verify/decode it. const token_data = Auth.validateAccessToken(token) - if (!token_data || !token_data.username) { + if (!token_data || !token_data.id) { + // Token invalid Logger.error('Cannot validate socket - invalid token') return socket.emit('invalid_token') } - const user = await Database.userModel.getUserByUsername(token_data.username) + // get the user via the id from the decoded jwt. + const user = await Database.userModel.getUserById(token_data.id) if (!user) { + // user not found Logger.error('Cannot validate socket - invalid token') return socket.emit('invalid_token') }