From 728790e38f490eb338493cf147f5796461943dd8 Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Sun, 5 Jun 2022 13:18:51 -0700 Subject: [PATCH] feat: Validate Google, Azure, OIDC SSO access (#3590) * chore: Store expiresAt on UserAuthentications. This represents the time that the accessToken is no longer valid and should be exchanged using the refreshToken * feat: Check and expire Google SSO * fix: Better handling of multiple auth methods Added more docs * fix: Retry access validation with network errors * Small refactor, add Azure token validation support * doc * test * lint * OIDC refresh support * CheckSSOAccessTask -> ValidateSSOAccessTask Added lastValidatedAt column Skip checks if validated within 5min Some edge cases around encrypted columns --- server/commands/accountProvisioner.ts | 4 + server/commands/userCreator.ts | 2 + server/emails/mailer.tsx | 1 + ...25054603-user-authentication-expires-at.js | 32 +++++ server/models/AuthenticationProvider.ts | 31 +++++ server/models/User.ts | 40 ++++-- server/models/UserAuthentication.ts | 114 +++++++++++++++++- server/models/decorators/Encrypted.ts | 10 +- server/queues/tasks/ValidateSSOAccessTask.ts | 55 +++++++++ server/routes/api/auth.ts | 3 + server/routes/auth/providers/azure.ts | 3 +- server/routes/auth/providers/google.ts | 2 + server/routes/auth/providers/index.ts | 4 +- server/routes/auth/providers/oidc.ts | 13 +- server/routes/auth/providers/slack.ts | 2 + server/utils/azure.ts | 9 ++ server/utils/google.ts | 9 ++ server/utils/oauth.ts | 83 +++++++++++++ server/utils/oidc.ts | 10 ++ 19 files changed, 413 insertions(+), 14 deletions(-) create mode 100644 server/migrations/20220525054603-user-authentication-expires-at.js create mode 100644 server/queues/tasks/ValidateSSOAccessTask.ts create mode 100644 server/utils/azure.ts create mode 100644 server/utils/google.ts create mode 100644 server/utils/oauth.ts create mode 100644 server/utils/oidc.ts diff --git a/server/commands/accountProvisioner.ts b/server/commands/accountProvisioner.ts index 8056c25c6..e19e44629 100644 --- a/server/commands/accountProvisioner.ts +++ b/server/commands/accountProvisioner.ts @@ -34,6 +34,7 @@ type Props = { scopes: string[]; accessToken?: string; refreshToken?: string; + expiresIn?: number; }; }; @@ -83,6 +84,9 @@ async function accountProvisioner({ ip, authentication: { ...authenticationParams, + expiresAt: authenticationParams.expiresIn + ? new Date(Date.now() + authenticationParams.expiresIn * 1000) + : undefined, authenticationProviderId: authenticationProvider.id, }, }); diff --git a/server/commands/userCreator.ts b/server/commands/userCreator.ts index b5d47628a..569e165bc 100644 --- a/server/commands/userCreator.ts +++ b/server/commands/userCreator.ts @@ -22,6 +22,7 @@ type Props = { scopes: string[]; accessToken?: string; refreshToken?: string; + expiresAt?: Date; }; }; @@ -36,6 +37,7 @@ export default async function userCreator({ ip, }: Props): Promise { const { authenticationProviderId, providerId, ...rest } = authentication; + const auth = await UserAuthentication.findOne({ where: { providerId, diff --git a/server/emails/mailer.tsx b/server/emails/mailer.tsx index b13e29dd5..f366391a9 100644 --- a/server/emails/mailer.tsx +++ b/server/emails/mailer.tsx @@ -44,6 +44,7 @@ export class Mailer { "email", "Couldn't generate a test account with ethereal.email at this time – emails will not be sent." ); + return; } this.transporter = nodemailer.createTransport(options); diff --git a/server/migrations/20220525054603-user-authentication-expires-at.js b/server/migrations/20220525054603-user-authentication-expires-at.js new file mode 100644 index 000000000..4fff2516f --- /dev/null +++ b/server/migrations/20220525054603-user-authentication-expires-at.js @@ -0,0 +1,32 @@ +'use strict'; + +module.exports = { + up: async (queryInterface, Sequelize) => { + await queryInterface.sequelize.transaction(async (transaction) => { + await queryInterface.addColumn("user_authentications", "expiresAt", { + type: Sequelize.DATE, + allowNull: true, + transaction + }); + await queryInterface.addColumn("user_authentications", "lastValidatedAt", { + type: Sequelize.DATE, + allowNull: true, + transaction + }); + }); + }, + down: async (queryInterface) => { + await queryInterface.sequelize.transaction(async (transaction) => { + await queryInterface.removeColumn( + "user_authentications", + "lastValidatedAt", + { + transaction + } + ); + await queryInterface.removeColumn("user_authentications", "expiresAt", { + transaction + }); + }); + }, +}; diff --git a/server/models/AuthenticationProvider.ts b/server/models/AuthenticationProvider.ts index d659c6a90..60a7ecaf0 100644 --- a/server/models/AuthenticationProvider.ts +++ b/server/models/AuthenticationProvider.ts @@ -12,6 +12,10 @@ import { IsUUID, PrimaryKey, } from "sequelize-typescript"; +import env from "@server/env"; +import AzureClient from "@server/utils/azure"; +import GoogleClient from "@server/utils/google"; +import OIDCClient from "@server/utils/oidc"; import { ValidationError } from "../errors"; import Team from "./Team"; import UserAuthentication from "./UserAuthentication"; @@ -57,6 +61,33 @@ class AuthenticationProvider extends Model { // instance methods + /** + * Create an OAuthClient for this provider, if possible. + * + * @returns A configured OAuthClient instance + */ + get oauthClient() { + switch (this.name) { + case "google": + return new GoogleClient( + env.GOOGLE_CLIENT_ID || "", + env.GOOGLE_CLIENT_SECRET || "" + ); + case "azure": + return new AzureClient( + env.AZURE_CLIENT_ID || "", + env.AZURE_CLIENT_SECRET || "" + ); + case "oidc": + return new OIDCClient( + env.OIDC_CLIENT_ID || "", + env.OIDC_CLIENT_SECRET || "" + ); + default: + return undefined; + } + } + disable = async () => { const res = await (this .constructor as typeof AuthenticationProvider).findAndCountAll({ diff --git a/server/models/User.ts b/server/models/User.ts index 8dc9eeeef..963921360 100644 --- a/server/models/User.ts +++ b/server/models/User.ts @@ -1,7 +1,7 @@ import crypto from "crypto"; import { addMinutes, subMinutes } from "date-fns"; import JWT from "jsonwebtoken"; -import { Transaction, QueryTypes, Op } from "sequelize"; +import { Transaction, QueryTypes, SaveOptions, Op } from "sequelize"; import { Table, Column, @@ -299,8 +299,25 @@ class User extends ParanoidModel { }); }; - // Returns a session token that is used to make API requests and is stored - // in the client browser cookies to remain logged in. + /** + * Rotate's the users JWT secret. This has the effect of invalidating ALL + * previously issued tokens. + * + * @param options Save options + * @returns Promise that resolves when database persisted + */ + rotateJwtSecret = (options: SaveOptions) => { + User.setRandomJwtSecret(this); + return this.save(options); + }; + + /** + * Returns a session token that is used to make API requests and is stored + * in the client browser cookies to remain logged in. + * + * @param expiresAt The time the token will expire at + * @returns The session token + */ getJwtToken = (expiresAt?: Date) => { return JWT.sign( { @@ -312,8 +329,13 @@ class User extends ParanoidModel { ); }; - // Returns a temporary token that is only used for transferring a session - // between subdomains or domains. It has a short expiry and can only be used once + /** + * Returns a temporary token that is only used for transferring a session + * between subdomains or domains. It has a short expiry and can only be used + * once. + * + * @returns The transfer token + */ getTransferToken = () => { return JWT.sign( { @@ -326,8 +348,12 @@ class User extends ParanoidModel { ); }; - // Returns a temporary token that is only used for logging in from an email - // It can only be used to sign in once and has a medium length expiry + /** + * Returns a temporary token that is only used for logging in from an email + * It can only be used to sign in once and has a medium length expiry + * + * @returns The email signin token + */ getEmailSigninToken = () => { return JWT.sign( { diff --git a/server/models/UserAuthentication.ts b/server/models/UserAuthentication.ts index 06211eaa8..8527af730 100644 --- a/server/models/UserAuthentication.ts +++ b/server/models/UserAuthentication.ts @@ -1,4 +1,8 @@ +import { addMinutes, subMinutes } from "date-fns"; +import invariant from "invariant"; +import { SaveOptions } from "sequelize"; import { + BeforeCreate, BelongsTo, Column, DataType, @@ -6,6 +10,8 @@ import { Table, Unique, } from "sequelize-typescript"; +import Logger from "@server/logging/Logger"; +import { AuthenticationError } from "../errors"; import AuthenticationProvider from "./AuthenticationProvider"; import User from "./User"; import IdModel from "./base/IdModel"; @@ -44,6 +50,12 @@ class UserAuthentication extends IdModel { @Column providerId: string; + @Column(DataType.DATE) + expiresAt: Date; + + @Column(DataType.DATE) + lastValidatedAt: Date; + // associations @BelongsTo(() => User, "userId") @@ -53,13 +65,113 @@ class UserAuthentication extends IdModel { @Column(DataType.UUID) userId: string; - @BelongsTo(() => AuthenticationProvider, "providerId") + @BelongsTo(() => AuthenticationProvider, "authenticationProviderId") authenticationProvider: AuthenticationProvider; @ForeignKey(() => AuthenticationProvider) @Unique @Column(DataType.UUID) authenticationProviderId: string; + + @BeforeCreate + static setValidated(model: UserAuthentication) { + model.lastValidatedAt = new Date(); + } + + // instance methods + + /** + * Validates that the tokens within this authentication record are still + * valid. Will update the record with a new access token if it is expired. + * + * @param options SaveOptions + * @param force Force validation to occur with third party provider + * @returns true if the accessToken or refreshToken is still valid + */ + public async validateAccess( + options: SaveOptions, + force = false + ): Promise { + // Check a maximum of once every 5 minutes + if (this.lastValidatedAt > subMinutes(Date.now(), 5) && !force) { + return true; + } + + const authenticationProvider = await this.$get("authenticationProvider", { + transaction: options.transaction, + }); + invariant( + authenticationProvider, + "authenticationProvider must exist for user authentication" + ); + + await this.refreshAccessTokenIfNeeded(authenticationProvider, options); + + try { + const client = authenticationProvider.oauthClient; + if (client) { + await client.userInfo(this.accessToken); + } + + // write to db when we last checked + this.lastValidatedAt = new Date(); + await this.save({ + transaction: options.transaction, + }); + + return true; + } catch (error) { + if (error instanceof AuthenticationError) { + return false; + } + + // Throw unknown errors to trigger a retry of the task. + throw error; + } + } + + /** + * Updates the accessToken and refreshToken in the database if expiring. If the + * accessToken is still valid or the AuthenticationProvider does not support + * refreshTokens then no work is done. + * + * @param options SaveOptions + * @returns true if the tokens were updated + */ + private async refreshAccessTokenIfNeeded( + authenticationProvider: AuthenticationProvider, + options: SaveOptions + ): Promise { + if (this.expiresAt > addMinutes(Date.now(), 5) || !this.refreshToken) { + return false; + } + + Logger.info("utils", "Refreshing expiring access token", { + id: this.id, + userId: this.userId, + }); + + const client = authenticationProvider.oauthClient; + if (client) { + const response = await client.rotateToken(this.refreshToken); + + // Not all OAuth providers return a new refreshToken so we need to guard + // against setting to an empty value. + if (response.refreshToken) { + this.refreshToken = response.refreshToken; + } + this.accessToken = response.accessToken; + this.expiresAt = response.expiresAt; + this.save(options); + } + + Logger.info("utils", "Successfully refreshed expired access token", { + id: this.id, + userId: this.userId, + }); + + return true; + } } export default UserAuthentication; diff --git a/server/models/decorators/Encrypted.ts b/server/models/decorators/Encrypted.ts index 8f996ade1..bc2b0da18 100644 --- a/server/models/decorators/Encrypted.ts +++ b/server/models/decorators/Encrypted.ts @@ -1,3 +1,4 @@ +import { isNil } from "lodash"; import vaults from "@server/database/vaults"; import Logger from "@server/logging/Logger"; @@ -19,6 +20,9 @@ export function getEncryptedColumn(target: any, propertyKey: string): string { try { return Reflect.getMetadata(key, target, propertyKey).get.call(target); } catch (err) { + if (err.message.includes("Unexpected end of JSON input")) { + return ""; + } if (err.message.includes("bad decrypt")) { Logger.error( `Failed to decrypt database column (${propertyKey}). The SECRET_KEY environment variable may have changed since installation.`, @@ -39,5 +43,9 @@ export function setEncryptedColumn( propertyKey: string, value: string ) { - Reflect.getMetadata(key, target, propertyKey).set.call(target, value); + if (isNil(value)) { + target.setDataValue(propertyKey, value); + } else { + Reflect.getMetadata(key, target, propertyKey).set.call(target, value); + } } diff --git a/server/queues/tasks/ValidateSSOAccessTask.ts b/server/queues/tasks/ValidateSSOAccessTask.ts new file mode 100644 index 000000000..857e01857 --- /dev/null +++ b/server/queues/tasks/ValidateSSOAccessTask.ts @@ -0,0 +1,55 @@ +import { sequelize } from "@server/database/sequelize"; +import Logger from "@server/logging/Logger"; +import { User, UserAuthentication } from "@server/models"; +import BaseTask, { TaskPriority } from "./BaseTask"; + +type Props = { + userId: string; +}; + +export default class ValidateSSOAccessTask extends BaseTask { + public async perform({ userId }: Props) { + await sequelize.transaction(async (transaction) => { + const userAuthentications = await UserAuthentication.findAll({ + where: { userId }, + transaction, + lock: transaction.LOCK.UPDATE, + }); + if (userAuthentications.length === 0) { + return; + } + + // Check the validity of all the user's associated authentications. + const valid = await Promise.all( + userAuthentications.map(async (authentication) => + authentication.validateAccess({ transaction }) + ) + ); + + // If any are valid then we're done here. + if (valid.includes(true)) { + return; + } + + // If all are invalid then we need to revoke the users Outline sessions. + const user = await User.findByPk(userId, { + transaction, + lock: transaction.LOCK.UPDATE, + }); + + Logger.info( + "task", + `Authentication token no longer valid for ${user?.id}` + ); + + await user?.rotateJwtSecret({ transaction }); + }); + } + + public get options() { + return { + attempts: 2, + priority: TaskPriority.Background, + }; + } +} diff --git a/server/routes/api/auth.ts b/server/routes/api/auth.ts index 470232dc2..ca0a6ac84 100644 --- a/server/routes/api/auth.ts +++ b/server/routes/api/auth.ts @@ -6,6 +6,7 @@ import env from "@server/env"; import auth from "@server/middlewares/authentication"; import { Team, TeamDomain } from "@server/models"; import { presentUser, presentTeam, presentPolicies } from "@server/presenters"; +import ValidateSSOAccessTask from "@server/queues/tasks/ValidateSSOAccessTask"; import providers from "../auth/providers"; const router = new Router(); @@ -111,6 +112,8 @@ router.post("auth.info", auth(), async (ctx) => { }); invariant(team, "Team not found"); + await ValidateSSOAccessTask.schedule({ userId: user.id }); + ctx.body = { data: { user: presentUser(user, { diff --git a/server/routes/auth/providers/azure.ts b/server/routes/auth/providers/azure.ts index 557fc548a..98dd047a6 100644 --- a/server/routes/auth/providers/azure.ts +++ b/server/routes/auth/providers/azure.ts @@ -39,7 +39,7 @@ if (env.AZURE_CLIENT_ID && env.AZURE_CLIENT_SECRET) { req: Request, accessToken: string, refreshToken: string, - params: { id_token: string }, + params: { expires_in: number; id_token: string }, _profile: Profile, done: ( err: Error | null, @@ -105,6 +105,7 @@ if (env.AZURE_CLIENT_ID && env.AZURE_CLIENT_SECRET) { providerId: profile.oid, accessToken, refreshToken, + expiresIn: params.expires_in, scopes, }, }); diff --git a/server/routes/auth/providers/google.ts b/server/routes/auth/providers/google.ts index e85cd1de2..899d74edd 100644 --- a/server/routes/auth/providers/google.ts +++ b/server/routes/auth/providers/google.ts @@ -49,6 +49,7 @@ if (env.GOOGLE_CLIENT_ID && env.GOOGLE_CLIENT_SECRET) { req: Request, accessToken: string, refreshToken: string, + params: { expires_in: number }, profile: GoogleProfile, done: ( err: Error | null, @@ -85,6 +86,7 @@ if (env.GOOGLE_CLIENT_ID && env.GOOGLE_CLIENT_SECRET) { providerId: profile.id, accessToken, refreshToken, + expiresIn: params.expires_in, scopes, }, }); diff --git a/server/routes/auth/providers/index.ts b/server/routes/auth/providers/index.ts index 63d50ffc1..364ffb7b1 100644 --- a/server/routes/auth/providers/index.ts +++ b/server/routes/auth/providers/index.ts @@ -3,7 +3,7 @@ import { sortBy } from "lodash"; import { signin } from "@shared/utils/urlHelpers"; import { requireDirectory } from "@server/utils/fs"; -interface AuthenicationProvider { +interface AuthenticationProviderConfig { id: string; name: string; enabled: boolean; @@ -11,7 +11,7 @@ interface AuthenicationProvider { router: Router; } -const providers: AuthenicationProvider[] = []; +const providers: AuthenticationProviderConfig[] = []; requireDirectory(__dirname).forEach(([module, id]) => { // @ts-expect-error ts-migrate(2339) FIXME: Property 'config' does not exist on type 'unknown'... Remove this comment to see the full error message diff --git a/server/routes/auth/providers/oidc.ts b/server/routes/auth/providers/oidc.ts index 4e53e4d0e..276c204ba 100644 --- a/server/routes/auth/providers/oidc.ts +++ b/server/routes/auth/providers/oidc.ts @@ -3,13 +3,16 @@ import { Request } from "koa"; import Router from "koa-router"; import { get } from "lodash"; import { Strategy } from "passport-oauth2"; -import accountProvisioner from "@server/commands/accountProvisioner"; +import accountProvisioner, { + AccountProvisionerResult, +} from "@server/commands/accountProvisioner"; import env from "@server/env"; import { OIDCMalformedUserInfoError, AuthenticationError, } from "@server/errors"; import passportMiddleware from "@server/middlewares/passport"; +import { User } from "@server/models"; import { StateStore, request } from "@server/utils/passport"; const router = new Router(); @@ -60,8 +63,13 @@ if (env.OIDC_CLIENT_ID && env.OIDC_CLIENT_SECRET) { req: Request, accessToken: string, refreshToken: string, + params: { expires_in: number }, profile: Record, - done: any + done: ( + err: Error | null, + user: User | null, + result?: AccountProvisionerResult + ) => void ) { try { if (!profile.email) { @@ -102,6 +110,7 @@ if (env.OIDC_CLIENT_ID && env.OIDC_CLIENT_SECRET) { providerId: profile.sub, accessToken, refreshToken, + expiresIn: params.expires_in, scopes, }, }); diff --git a/server/routes/auth/providers/slack.ts b/server/routes/auth/providers/slack.ts index 694a6b39b..9810f9b64 100644 --- a/server/routes/auth/providers/slack.ts +++ b/server/routes/auth/providers/slack.ts @@ -66,6 +66,7 @@ if (env.SLACK_CLIENT_ID && env.SLACK_CLIENT_SECRET) { req: Request, accessToken: string, refreshToken: string, + params: { expires_in: number }, profile: SlackProfile, done: ( err: Error | null, @@ -94,6 +95,7 @@ if (env.SLACK_CLIENT_ID && env.SLACK_CLIENT_SECRET) { providerId: profile.user.id, accessToken, refreshToken, + expiresIn: params.expires_in, scopes, }, }); diff --git a/server/utils/azure.ts b/server/utils/azure.ts new file mode 100644 index 000000000..7c1339873 --- /dev/null +++ b/server/utils/azure.ts @@ -0,0 +1,9 @@ +import OAuthClient from "./oauth"; + +export default class AzureClient extends OAuthClient { + endpoints = { + authorize: "https://login.microsoftonline.com/common/oauth2/v2.0/authorize", + token: "https://login.microsoftonline.com/common/oauth2/v2.0/token", + userinfo: "https://graph.microsoft.com/v1.0/me", + }; +} diff --git a/server/utils/google.ts b/server/utils/google.ts new file mode 100644 index 000000000..eaa1d1f37 --- /dev/null +++ b/server/utils/google.ts @@ -0,0 +1,9 @@ +import OAuthClient from "./oauth"; + +export default class GoogleClient extends OAuthClient { + endpoints = { + authorize: "https://accounts.google.com/o/oauth2/auth", + token: "https://accounts.google.com/o/oauth2/token", + userinfo: "https://www.googleapis.com/oauth2/v3/userinfo", + }; +} diff --git a/server/utils/oauth.ts b/server/utils/oauth.ts new file mode 100644 index 000000000..a9426e4a8 --- /dev/null +++ b/server/utils/oauth.ts @@ -0,0 +1,83 @@ +import fetch from "fetch-with-proxy"; +import { AuthenticationError, InvalidRequestError } from "../errors"; + +export default abstract class OAuthClient { + private clientId: string; + private clientSecret: string; + + protected endpoints = { + authorize: "", + token: "", + userinfo: "", + }; + + constructor(clientId: string, clientSecret: string) { + this.clientId = clientId; + this.clientSecret = clientSecret; + } + + userInfo = async (accessToken: string) => { + let data; + let response; + + try { + response = await fetch(this.endpoints.userinfo, { + method: "GET", + headers: { + Authorization: `Bearer ${accessToken}`, + "Content-Type": "application/json", + }, + }); + data = await response.json(); + } catch (err) { + throw InvalidRequestError(err.message); + } + + const success = response.status >= 200 && response.status < 300; + if (!success) { + throw AuthenticationError(); + } + + return data; + }; + + rotateToken = async ( + refreshToken: string + ): Promise<{ + accessToken: string; + refreshToken?: string; + expiresAt: Date; + }> => { + let data; + let response; + + try { + response = await fetch(this.endpoints.token, { + method: "POST", + headers: { + "Content-Type": "application/x-www-form-urlencoded", + }, + body: new URLSearchParams({ + client_id: this.clientId, + client_secret: this.clientSecret, + refresh_token: refreshToken, + grant_type: "refresh_token", + }), + }); + data = await response.json(); + } catch (err) { + throw InvalidRequestError(err.message); + } + + const success = response.status >= 200 && response.status < 300; + if (!success) { + throw AuthenticationError(); + } + + return { + refreshToken: data.refresh_token, + accessToken: data.access_token, + expiresAt: new Date(Date.now() + data.expires_in * 1000), + }; + }; +} diff --git a/server/utils/oidc.ts b/server/utils/oidc.ts new file mode 100644 index 000000000..ca98e33a3 --- /dev/null +++ b/server/utils/oidc.ts @@ -0,0 +1,10 @@ +import env from "@server/env"; +import OAuthClient from "./oauth"; + +export default class OIDCClient extends OAuthClient { + endpoints = { + authorize: env.OIDC_AUTH_URI || "", + token: env.OIDC_TOKEN_URI || "", + userinfo: env.OIDC_USERINFO_URI || "", + }; +}