From b63c00f26f0219de9ad4d7d85fea801fbefad05f Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Mon, 15 Apr 2019 17:46:55 +0000 Subject: [PATCH] [next] Auth (#2257) * feat: improved auth features + performance * fix: auth check logic * fix: tests --- .dockerignore | 26 ++-- .gitignore | 12 +- .../admin/containers/AuthCheckContainer.tsx | 14 +- .../containers/RedirectLoginContainer.tsx | 103 -------------- .../admin/test/auth/restricted.spec.tsx | 15 -- .../client/ui/components/CallOut/CallOut.css | 1 + .../server/app/handlers/api/auth/local.ts | 4 +- src/core/server/app/handlers/api/install.ts | 6 +- .../passport/strategies/facebook.ts | 4 +- .../middleware/passport/strategies/google.ts | 4 +- .../app/middleware/passport/strategies/jwt.ts | 53 +++---- .../passport/strategies/oidc/index.ts | 129 +++++++++++------- .../passport/strategies/verifiers/jwt.ts | 4 +- .../passport/strategies/verifiers/oidc.ts | 69 ++++++++++ .../passport/strategies/verifiers/sso.ts | 8 +- src/core/server/index.ts | 3 + src/core/server/models/user.ts | 82 ++++------- src/core/server/services/users/index.ts | 14 +- 18 files changed, 245 insertions(+), 306 deletions(-) delete mode 100644 src/core/client/admin/containers/RedirectLoginContainer.tsx create mode 100644 src/core/server/app/middleware/passport/strategies/verifiers/oidc.ts diff --git a/.dockerignore b/.dockerignore index e3916e43e..8567ba47c 100644 --- a/.dockerignore +++ b/.dockerignore @@ -1,30 +1,30 @@ -# excluded because we'll likely need to rebuild this. -node_modules - -# static assets are rebuild in the docker container. -dist - # tests are not run in the docker container. __tests__ +coverage # we won't use the .git folder in production. .git -# hide the environment config. -.env +# don't include the dependancies +node_modules -# don't include logs. +# don't include any logs npm-debug.log* -yarn-error.log -# hide OS specific files. +# don't include any yarn files +yarn-error.log +yarn.lock + +# don't include any OS/editor files +.env .idea/ .vs .docz *.swp *.DS_STORE -# hide generated files. +# don't include any generated files +dist *.css.d.ts __generated__ - +README.md.orig diff --git a/.gitignore b/.gitignore index 706e32dd0..8f2d61ab9 100644 --- a/.gitignore +++ b/.gitignore @@ -1,17 +1,23 @@ +# don't include the dependancies node_modules -dist -.env + +# don't include any logs npm-debug.log* + +# don't include any yarn files yarn-error.log yarn.lock -coverage +# don't include any OS/editor files +.env .idea/ .vs .docz *.swp *.DS_STORE +# don't include any generated files +dist *.css.d.ts __generated__ README.md.orig diff --git a/src/core/client/admin/containers/AuthCheckContainer.tsx b/src/core/client/admin/containers/AuthCheckContainer.tsx index 4d2b4ce0d..87f6b3ce5 100644 --- a/src/core/client/admin/containers/AuthCheckContainer.tsx +++ b/src/core/client/admin/containers/AuthCheckContainer.tsx @@ -52,10 +52,7 @@ class AuthCheckContainer extends React.Component { } private hasAccess(props: Props = this.props) { - const { - viewer, - settings: { auth }, - } = props.data!; + const { viewer } = props.data!; if (viewer) { if ( viewer.role === GQLUSER_ROLE.COMMENTER || @@ -66,15 +63,6 @@ class AuthCheckContainer extends React.Component { !can(viewer, props.data.route.data)) ) { return false; - } else if ( - !viewer.email || - !viewer.username || - (!viewer.profiles.some(p => p.__typename === "LocalProfile") && - auth.integrations.local.enabled && - (auth.integrations.local.targetFilter.admin || - auth.integrations.local.targetFilter.stream)) - ) { - return false; } return true; } diff --git a/src/core/client/admin/containers/RedirectLoginContainer.tsx b/src/core/client/admin/containers/RedirectLoginContainer.tsx deleted file mode 100644 index 7cf0b02f5..000000000 --- a/src/core/client/admin/containers/RedirectLoginContainer.tsx +++ /dev/null @@ -1,103 +0,0 @@ -import { Match, Router, withRouter } from "found"; -import React from "react"; - -import { RedirectLoginContainerQueryResponse } from "talk-admin/__generated__/RedirectLoginContainerQuery.graphql"; -import { - SetRedirectPathMutation, - withSetRedirectPathMutation, -} from "talk-admin/mutations"; -import { graphql } from "talk-framework/lib/relay"; -import { withRouteConfig } from "talk-framework/lib/router"; -import { GQLUSER_ROLE } from "talk-framework/schema"; - -interface Props { - match: Match; - router: Router; - setRedirectPath: SetRedirectPathMutation; - data: RedirectLoginContainerQueryResponse | null; -} - -class RedirectLoginContainer extends React.Component { - constructor(props: Props) { - super(props); - this.redirectIfNotLoggedIn(); - } - - public componentWillReceiveProps(nextProps: Props) { - this.redirectIfNotLoggedIn(nextProps); - } - - private shouldRedirectTo(props: Props = this.props): string | null { - if (!props.data) { - return null; - } - const { - viewer, - settings: { auth }, - } = props.data!; - if (viewer) { - if (viewer.role === GQLUSER_ROLE.COMMENTER) { - return "/admin/login"; - } else if ( - !viewer.email || - !viewer.username || - (!viewer.profiles.some(p => p.__typename === "LocalProfile") && - auth.integrations.local.enabled && - (auth.integrations.local.targetFilter.admin || - auth.integrations.local.targetFilter.stream)) - ) { - return "/admin/login"; - } - return ""; - } - return "/admin/login"; - } - - private redirectIfNotLoggedIn(props: Props = this.props) { - const redirect = this.shouldRedirectTo(props); - if (redirect) { - const location = props.match.location; - props.setRedirectPath({ - path: location.pathname + location.search + location.hash, - }); - props.router.replace(redirect); - } - } - - public render() { - if (this.shouldRedirectTo()) { - return null; - } - return this.props.children; - } -} - -const enhanced = withRouteConfig({ - query: graphql` - query RedirectLoginContainerQuery { - viewer { - username - email - profiles { - __typename - } - role - } - settings { - auth { - integrations { - local { - enabled - targetFilter { - admin - stream - } - } - } - } - } - } - `, -})(withRouter(withSetRedirectPathMutation(RedirectLoginContainer))); - -export default enhanced; diff --git a/src/core/client/admin/test/auth/restricted.spec.tsx b/src/core/client/admin/test/auth/restricted.spec.tsx index 19dd0e061..9bed1a3ff 100644 --- a/src/core/client/admin/test/auth/restricted.spec.tsx +++ b/src/core/client/admin/test/auth/restricted.spec.tsx @@ -59,21 +59,6 @@ it("show restricted screen for commenters and staff", async () => { } }); -it("show restricted screen when email is not set", async () => { - const { testRenderer } = createTestRenderer({ email: "" }); - await waitForElement(() => within(testRenderer.root).getByTestID("authBox")); -}); - -it("show restricted screen when username is not set", async () => { - const { testRenderer } = createTestRenderer({ username: "" }); - await waitForElement(() => within(testRenderer.root).getByTestID("authBox")); -}); - -it("show restricted screen local was not set (password)", async () => { - const { testRenderer } = createTestRenderer({ profiles: [] }); - await waitForElement(() => within(testRenderer.root).getByTestID("authBox")); -}); - it("sign out when clicking on sign in as", async () => { const { context, testRenderer } = createTestRenderer({ role: GQLUSER_ROLE.COMMENTER, diff --git a/src/core/client/ui/components/CallOut/CallOut.css b/src/core/client/ui/components/CallOut/CallOut.css index 2022383a5..61cd24855 100644 --- a/src/core/client/ui/components/CallOut/CallOut.css +++ b/src/core/client/ui/components/CallOut/CallOut.css @@ -8,6 +8,7 @@ box-sizing: border-box; border-width: 1px; border-style: solid; + word-break: break-word; } .colorRegular { diff --git a/src/core/server/app/handlers/api/auth/local.ts b/src/core/server/app/handlers/api/auth/local.ts index f851ac9a6..d4961edc6 100644 --- a/src/core/server/app/handlers/api/auth/local.ts +++ b/src/core/server/app/handlers/api/auth/local.ts @@ -9,7 +9,7 @@ import { import { validate } from "talk-server/app/request/body"; import { GQLUSER_ROLE } from "talk-server/graph/tenant/schema/__generated__/types"; import { LocalProfile } from "talk-server/models/user"; -import { upsert } from "talk-server/services/users"; +import { insert } from "talk-server/services/users"; import { Request } from "talk-server/types/express"; export interface SignupBody { @@ -65,7 +65,7 @@ export const signupHandler = ({ }; // Create the new user. - const user = await upsert(mongo, tenant, { + const user = await insert(mongo, tenant, { email, username, profiles: [profile], diff --git a/src/core/server/app/handlers/api/install.ts b/src/core/server/app/handlers/api/install.ts index 6cc03a443..5c4a35ab0 100644 --- a/src/core/server/app/handlers/api/install.ts +++ b/src/core/server/app/handlers/api/install.ts @@ -10,14 +10,14 @@ import { TenantInstalledAlreadyError } from "talk-server/errors"; import { GQLUSER_ROLE } from "talk-server/graph/tenant/schema/__generated__/types"; import { LocalProfile } from "talk-server/models/user"; import { install, InstallTenant } from "talk-server/services/tenant"; -import { upsert, UpsertUser } from "talk-server/services/users"; +import { insert, InsertUser } from "talk-server/services/users"; import { RequestHandler } from "talk-server/types/express"; export interface TenantInstallBody { tenant: Omit & { locale: LanguageCode | null; }; - user: Required & { password: string }>; + user: Required & { password: string }>; } const TenantInstallBodySchema = Joi.object().keys({ @@ -113,7 +113,7 @@ export const installHandler = ({ }; // Create the first admin user. - await upsert(mongo, tenant, { + await insert(mongo, tenant, { email, username, profiles: [profile], diff --git a/src/core/server/app/middleware/passport/strategies/facebook.ts b/src/core/server/app/middleware/passport/strategies/facebook.ts index 70a3eee0a..d293bba15 100644 --- a/src/core/server/app/middleware/passport/strategies/facebook.ts +++ b/src/core/server/app/middleware/passport/strategies/facebook.ts @@ -14,7 +14,7 @@ import { FacebookProfile, retrieveUserWithProfile, } from "talk-server/models/user"; -import { upsert } from "talk-server/services/users"; +import { insert } from "talk-server/services/users"; export type FacebookStrategyOptions = OAuth2StrategyOptions; @@ -71,7 +71,7 @@ export default class FacebookStrategy extends OAuth2Strategy< emailVerified = false; } - user = await upsert(this.mongo, tenant, { + user = await insert(this.mongo, tenant, { username: displayName, role: GQLUSER_ROLE.COMMENTER, email, diff --git a/src/core/server/app/middleware/passport/strategies/google.ts b/src/core/server/app/middleware/passport/strategies/google.ts index 91a736950..68184e6f2 100644 --- a/src/core/server/app/middleware/passport/strategies/google.ts +++ b/src/core/server/app/middleware/passport/strategies/google.ts @@ -14,7 +14,7 @@ import { GoogleProfile, retrieveUserWithProfile, } from "talk-server/models/user"; -import { upsert } from "talk-server/services/users"; +import { insert } from "talk-server/services/users"; export type GoogleStrategyOptions = OAuth2StrategyOptions; @@ -70,7 +70,7 @@ export default class GoogleStrategy extends OAuth2Strategy< emailVerified = false; } - user = await upsert(this.mongo, tenant, { + user = await insert(this.mongo, tenant, { username: displayName, role: GQLUSER_ROLE.COMMENTER, email, diff --git a/src/core/server/app/middleware/passport/strategies/jwt.ts b/src/core/server/app/middleware/passport/strategies/jwt.ts index 153d1213f..2dc1149b3 100644 --- a/src/core/server/app/middleware/passport/strategies/jwt.ts +++ b/src/core/server/app/middleware/passport/strategies/jwt.ts @@ -2,35 +2,31 @@ import jwt from "jsonwebtoken"; import { Strategy } from "passport-strategy"; import { AppOptions } from "talk-server/app"; -import { - JWTToken, - JWTVerifier, -} from "talk-server/app/middleware/passport/strategies/verifiers/jwt"; -import { - SSOToken, - SSOVerifier, -} from "talk-server/app/middleware/passport/strategies/verifiers/sso"; import { TenantNotFoundError, TokenInvalidError } from "talk-server/errors"; import { Tenant } from "talk-server/models/tenant"; import { User } from "talk-server/models/user"; import { extractJWTFromRequest } from "talk-server/services/jwt"; import { Request } from "talk-server/types/express"; +import { JWTToken, JWTVerifier } from "./verifiers/jwt"; +import { OIDCIDToken, OIDCVerifier } from "./verifiers/oidc"; +import { SSOToken, SSOVerifier } from "./verifiers/sso"; + export type JWTStrategyOptions = Pick< AppOptions, - "signingConfig" | "mongo" | "redis" + "signingConfig" | "mongo" | "redis" | "tenantCache" >; /** * Token is the various forms of the Token that can be verified. */ -type Token = SSOToken | JWTToken | object | string | null; +type Token = OIDCIDToken | SSOToken | JWTToken | object | string | null; /** * Verifier allows different implementations to offer ways to verify a given * Token. */ -interface Verifier { +export interface Verifier { /** * verify will perform the verification and return a User. */ @@ -50,18 +46,16 @@ interface Verifier { export class JWTStrategy extends Strategy { public name = "jwt"; - private verifiers: { - sso: Verifier; - jwt: Verifier; - }; + private verifiers: Verifier[]; constructor(options: JWTStrategyOptions) { super(); - this.verifiers = { - sso: new SSOVerifier(options), - jwt: new JWTVerifier(options), - }; + this.verifiers = [ + new OIDCVerifier(options), + new SSOVerifier(options), + new JWTVerifier(options), + ]; } private async verify(tokenString: string, tenant: Tenant) { @@ -70,22 +64,11 @@ export class JWTStrategy extends Strategy { throw new TokenInvalidError(tokenString, "token could not be decoded"); } - // TODO: add OIDC support. - // At the moment, OpenID Connect tokens are not supported here directly, - // instead, the default implementation redirects the user to the - // authorization endpoint where they login, and a redirection occurs - // yielding the token to us via the Authorization Code Flow. We then issue a - // Talk Token for that request, that the client uses after. - - // Handle SSO integrations. - if (this.verifiers.sso.supports(token, tenant)) { - return this.verifiers.sso.verify(tokenString, token, tenant); - } - - // Handle the raw JWT token. - if (this.verifiers.jwt.supports(token, tenant)) { - // Verify the token with the JWT verification strategy. - return this.verifiers.jwt.verify(tokenString, token, tenant); + // Try to verify the token. + for (const verifier of this.verifiers) { + if (verifier.supports(token, tenant)) { + return verifier.verify(tokenString, token, tenant); + } } // No verifier could be found. diff --git a/src/core/server/app/middleware/passport/strategies/oidc/index.ts b/src/core/server/app/middleware/passport/strategies/oidc/index.ts index 6944ef743..f6f2d33cd 100644 --- a/src/core/server/app/middleware/passport/strategies/oidc/index.ts +++ b/src/core/server/app/middleware/passport/strategies/oidc/index.ts @@ -7,15 +7,18 @@ import { Strategy } from "passport-strategy"; import { validate } from "talk-server/app/request/body"; import { reconstructURL } from "talk-server/app/url"; -import { - GQLOIDCAuthIntegration, - GQLUSER_ROLE, -} from "talk-server/graph/tenant/schema/__generated__/types"; +import { GQLUSER_ROLE } from "talk-server/graph/tenant/schema/__generated__/types"; +import logger from "talk-server/logger"; +import { OIDCAuthIntegration } from "talk-server/models/settings"; import { Tenant } from "talk-server/models/tenant"; -import { OIDCProfile, retrieveUserWithProfile } from "talk-server/models/user"; +import { + OIDCProfile, + retrieveUserWithProfile, + User, +} from "talk-server/models/user"; import TenantCache from "talk-server/services/tenant/cache"; import { TenantCacheAdapter } from "talk-server/services/tenant/cache/adapter"; -import { upsert } from "talk-server/services/users"; +import { insert } from "talk-server/services/users"; import { Request } from "talk-server/types/express"; export interface Params { @@ -85,11 +88,9 @@ const signingKeyFactory = (client: jwks.JwksClient): jwt.KeyFunction => ( }); }; -function getEnabledIntegration( - tenant: Tenant -): Required { - // Grab the OIDC Integration. - const integration = tenant.auth.integrations.oidc; +export function getEnabledIntegration( + integration: OIDCAuthIntegration +): Required { if (!integration.enabled) { // TODO: return a better error. throw new Error("integration not enabled"); @@ -109,7 +110,7 @@ function getEnabledIntegration( } // TODO: (wyattjoh) for some reason, type guards above to not allow coercion to this required type. - return integration as Required; + return integration as Required; } export const OIDCIDTokenSchema = Joi.object() @@ -135,9 +136,9 @@ export const OIDCIDTokenSchema = Joi.object() export async function findOrCreateOIDCUser( mongo: Db, tenant: Tenant, - integration: GQLOIDCAuthIntegration, + integration: OIDCAuthIntegration, token: OIDCIDToken -) { +): Promise | null> { // Unpack/validate the token content. const { sub, @@ -160,15 +161,11 @@ export async function findOrCreateOIDCUser( }; // Try to lookup user given their id provided in the `sub` claim. - let user = await retrieveUserWithProfile(mongo, tenant.id, { - // NOTE: (wyattjoh) as the current requirements do not allow multiple OIDC integrations, we are only getting the profile based on the OIDC provider. - type: "oidc", - id: sub, - }); + let user = await retrieveUserWithProfile(mongo, tenant.id, profile); if (!user) { if (!integration.allowRegistration) { // Registration is disabled, so we can't create the user user here. - return; + return null; } // FIXME: implement rules. @@ -177,7 +174,7 @@ export async function findOrCreateOIDCUser( const username = preferred_username || nickname || name; // Create the new user, as one didn't exist before! - user = await upsert(mongo, tenant, { + user = await insert(mongo, tenant, { username, role: GQLUSER_ROLE.COMMENTER, email, @@ -192,6 +189,47 @@ export async function findOrCreateOIDCUser( return user; } +export function findOrCreateOIDCUserWithToken( + mongo: Db, + tenant: Tenant, + client: JwksClient, + integration: OIDCAuthIntegration, + token: string +) { + return new Promise | null>((resolve, reject) => { + logger.trace({ tenantID: tenant.id }, "verifying oidc id_token"); + jwt.verify( + token, + signingKeyFactory(client), + { + issuer: integration.issuer, + }, + async (err, decoded) => { + logger.trace( + { tenantID: tenant.id }, + "finished verifying oidc id_token" + ); + if (err) { + // TODO: wrap error? + return reject(err); + } + + try { + const user = await findOrCreateOIDCUser( + mongo, + tenant, + integration, + decoded as OIDCIDToken + ); + return resolve(user); + } catch (err) { + return reject(err); + } + } + ); + }); +} + /** * OIDC_SCOPE is the set of scopes requested for users signing up via OIDC. */ @@ -218,7 +256,7 @@ export default class OIDCStrategy extends Strategy { private lookupJWKSClient( req: Request, tenantID: string, - oidc: Required + oidc: Required ): jwks.JwksClient { let tenantIntegration = this.cache.get(tenantID); if (!tenantIntegration) { @@ -249,7 +287,7 @@ export default class OIDCStrategy extends Strategy { return tenantIntegration.jwksClient; } - private userAuthenticatedCallback = ( + private userAuthenticatedCallback = async ( req: Request, accessToken: string, // ignore the access token, we don't use it. refreshToken: string, // ignore the refresh token, we don't use it. @@ -274,9 +312,9 @@ export default class OIDCStrategy extends Strategy { // Get the integration from the tenant. If needed, it will be used to create // a new strategy. - let integration: Required; + let integration: Required; try { - integration = getEnabledIntegration(tenant); + integration = getEnabledIntegration(tenant.auth.integrations.oidc); } catch (err) { // TODO: wrap error? return done(err); @@ -286,36 +324,23 @@ export default class OIDCStrategy extends Strategy { const client = this.lookupJWKSClient(req, tenant.id, integration); // Verify that the id_token is valid or not. - jwt.verify( - id_token, - signingKeyFactory(client), - { - issuer: integration.issuer, - }, - async (err, decoded) => { - if (err) { - // TODO: wrap error? - return done(err); - } - - try { - const user = await findOrCreateOIDCUser( - this.mongo, - tenant, - integration, - decoded as OIDCIDToken - ); - return done(null, user); - } catch (err) { - return done(err); - } - } - ); + try { + const user = await findOrCreateOIDCUserWithToken( + this.mongo, + tenant, + client, + integration, + id_token + ); + return done(null, user || undefined); + } catch (err) { + return done(err); + } }; private createStrategy( req: Request, - integration: Required + integration: Required ): OAuth2Strategy { const { clientID, clientSecret, authorizationURL, tokenURL } = integration; @@ -346,7 +371,7 @@ export default class OIDCStrategy extends Strategy { // Get the integration from the tenant. If needed, it will be used to create // a new strategy. - const integration = getEnabledIntegration(tenant); + const integration = getEnabledIntegration(tenant.auth.integrations.oidc); // Try to get the Tenant's cached integrations. let tenantIntegration = this.cache.get(tenant.id); diff --git a/src/core/server/app/middleware/passport/strategies/verifiers/jwt.ts b/src/core/server/app/middleware/passport/strategies/verifiers/jwt.ts index f1fff9519..a829f9253 100644 --- a/src/core/server/app/middleware/passport/strategies/verifiers/jwt.ts +++ b/src/core/server/app/middleware/passport/strategies/verifiers/jwt.ts @@ -8,6 +8,8 @@ import { Tenant } from "talk-server/models/tenant"; import { retrieveUser } from "talk-server/models/user"; import { checkJWTRevoked, JWTSigningConfig } from "talk-server/services/jwt"; +import { Verifier } from "../jwt"; + export interface JWTToken { /** * jti is the Token identifier. With normal login tokens, this is a randomly @@ -72,7 +74,7 @@ export interface JWTVerifierOptions { redis: Redis; } -export class JWTVerifier { +export class JWTVerifier implements Verifier { private signingConfig: JWTSigningConfig; private mongo: Db; private redis: Redis; diff --git a/src/core/server/app/middleware/passport/strategies/verifiers/oidc.ts b/src/core/server/app/middleware/passport/strategies/verifiers/oidc.ts new file mode 100644 index 000000000..83d269682 --- /dev/null +++ b/src/core/server/app/middleware/passport/strategies/verifiers/oidc.ts @@ -0,0 +1,69 @@ +import jwks, { JwksClient } from "jwks-rsa"; +import { Db } from "mongodb"; + +import { AppOptions } from "talk-server/app"; +import { Tenant } from "talk-server/models/tenant"; +import { TenantCacheAdapter } from "talk-server/services/tenant/cache/adapter"; + +import logger from "talk-server/logger"; +import { Verifier } from "../jwt"; +import { + findOrCreateOIDCUserWithToken, + getEnabledIntegration, + isOIDCToken, + OIDCIDToken, +} from "../oidc"; + +export type OIDCIDToken = OIDCIDToken; + +export type OIDCVerifierOptions = Pick< + AppOptions, + "mongo" | "redis" | "tenantCache" +>; + +export class OIDCVerifier implements Verifier { + private mongo: Db; + private cache: TenantCacheAdapter; + + constructor({ mongo, tenantCache }: OIDCVerifierOptions) { + this.mongo = mongo; + this.cache = new TenantCacheAdapter(tenantCache); + } + + public async verify(tokenString: string, token: OIDCIDToken, tenant: Tenant) { + // Ensure that the integration is enabled. + const integration = getEnabledIntegration(tenant.auth.integrations.oidc); + + // Grab the JWKS client to verify the SSO ID token. + let client = this.cache.get(tenant.id); + if (!client) { + logger.trace({ tenantID: tenant.id }, "jwks client not cached"); + client = jwks({ + jwksUri: integration.jwksURI, + }); + + this.cache.set(tenant.id, client); + } else { + logger.trace({ tenantID: tenant.id }, "jwks client cached"); + } + + return findOrCreateOIDCUserWithToken( + this.mongo, + tenant, + client, + integration, + tokenString + ); + } + + public supports( + token: OIDCIDToken | object, + tenant: Tenant + ): token is OIDCIDToken { + return ( + tenant.auth.integrations.oidc.enabled && + Boolean(tenant.auth.integrations.oidc.jwksURI) && + isOIDCToken(token) + ); + } +} diff --git a/src/core/server/app/middleware/passport/strategies/verifiers/sso.ts b/src/core/server/app/middleware/passport/strategies/verifiers/sso.ts index 88e4fe814..1b48c6bd4 100644 --- a/src/core/server/app/middleware/passport/strategies/verifiers/sso.ts +++ b/src/core/server/app/middleware/passport/strategies/verifiers/sso.ts @@ -9,7 +9,9 @@ import { } from "talk-server/graph/tenant/schema/__generated__/types"; import { Tenant } from "talk-server/models/tenant"; import { retrieveUserWithProfile, SSOProfile } from "talk-server/models/user"; -import { upsert } from "talk-server/services/users"; +import { insert } from "talk-server/services/users"; + +import { Verifier } from "../jwt"; export interface SSOStrategyOptions { mongo: Db; @@ -69,7 +71,7 @@ export async function findOrCreateSSOUser( // FIXME: (wyattjoh) implement rules! Not all users should be able to create an account via this method. // Create the new user, as one didn't exist before! - user = await upsert(mongo, tenant, { + user = await insert(mongo, tenant, { username, role: GQLUSER_ROLE.COMMENTER, email, @@ -109,7 +111,7 @@ export interface SSOVerifierOptions { mongo: Db; } -export class SSOVerifier { +export class SSOVerifier implements Verifier { private mongo: Db; constructor({ mongo }: SSOVerifierOptions) { diff --git a/src/core/server/index.ts b/src/core/server/index.ts index d296de6c9..bffa3b476 100644 --- a/src/core/server/index.ts +++ b/src/core/server/index.ts @@ -123,7 +123,10 @@ class Server { // Create the database indexes if it isn't disabled. if (!this.config.get("disable_mongodb_autoindexing")) { // Setup the database indexes. + logger.info("mongodb autoindexing is enabled, starting indexing"); await ensureIndexes(this.mongo); + } else { + logger.info("mongodb autoindexing is disabled, skipping indexing"); } // Launch all of the job processors. diff --git a/src/core/server/models/user.ts b/src/core/server/models/user.ts index af436cef3..756e6e980 100644 --- a/src/core/server/models/user.ts +++ b/src/core/server/models/user.ts @@ -1,5 +1,5 @@ import bcrypt from "bcryptjs"; -import { Db } from "mongodb"; +import { Db, MongoError } from "mongodb"; import uuid from "uuid"; import { Omit, Sub } from "talk-common/types"; @@ -17,7 +17,7 @@ import { createConnectionOrderVariants, createIndexFactory, } from "talk-server/models/helpers/indexing"; -import Query, { FilterQuery } from "talk-server/models/helpers/query"; +import Query from "talk-server/models/helpers/query"; import { TenantResource } from "talk-server/models/tenant"; import { Connection, @@ -97,12 +97,15 @@ export async function createUserIndexes(mongo: Db) { // UNIQUE { profiles.type, profiles.id } await createIndex( { tenantID: 1, "profiles.type": 1, "profiles.id": 1 }, + { unique: true, partialFilterExpression: { profiles: { $exists: true } } } + ); + + // { profiles } + await createIndex( + { tenantID: 1, profiles: 1, email: 1 }, { - unique: true, - // We're filtering by the first entry in the profiles array to ensure we - // only enforce uniqueness when the profiles array has at least a single - // profile. - partialFilterExpression: { "profiles.0": { $exists: true } }, + partialFilterExpression: { profiles: { $exists: true } }, + background: true, } ); @@ -141,15 +144,15 @@ function hashPassword(password: string): Promise { return bcrypt.hash(password, 10); } -export type UpsertUserInput = Omit< +export type InsertUserInput = Omit< User, "id" | "tenantID" | "tokens" | "createdAt" >; -export async function upsertUser( +export async function insertUser( mongo: Db, tenantID: string, - input: UpsertUserInput + input: InsertUserInput ) { const now = new Date(); @@ -158,13 +161,18 @@ export async function upsertUser( // default are the properties set by the application when a new user is // created. - const defaults: Sub = { + const defaults: Sub = { id, tenantID, tokens: [], createdAt: now, }; + // Guard against empty login profiles (they need some way to login). + if (input.profiles.length === 0) { + throw new Error("users require at least one profile"); + } + // Mutate the profiles to ensure we mask handle any secrets. const profiles: Profile[] = []; for (let profile of input.profiles) { @@ -189,52 +197,22 @@ export async function upsertUser( profiles, }; - // Create a query that will utilize a findOneAndUpdate to facilitate an upsert - // operation to ensure no user has the same profile and/or email address. If - // any user is found to have the same profile as any of the profiles specified - // in the new user object, then we should error here. - const filter = createUpsertUserFilter(user); + try { + // Insert it into the database. This may throw an error. + await collection(mongo).insert(user); + } catch (err) { + // Evaluate the error, if it is in regards to violating the unique index, + // then return a duplicate Story error. + if (err instanceof MongoError && err.code === 11000) { + throw new DuplicateUserError(); + } - // Create the upsert/update operation. - const update: { $setOnInsert: Readonly } = { - $setOnInsert: user, - }; - - // Insert it into the database. This may throw an error. - const result = await collection(mongo).findOneAndUpdate(filter, update, { - // We are using this to create a user, so we need to upsert it. - upsert: true, - - // False to return the updated document instead of the original document. - // This lets us detect if the document was updated or not. - returnOriginal: false, - }); - - // Check to see if this was a new user that was upserted, or one was found - // that matched existing records. We are sure here that the record exists - // because we're returning the updated document and performing an upsert - // operation. - if (result.value!.id !== id) { - throw new DuplicateUserError(); + throw err; } - return result.value!; + return user; } -const createUpsertUserFilter = (user: Readonly) => { - const query: FilterQuery = { - // Query by the profiles if the user is being created with one. - $or: user.profiles.map(profile => ({ profiles: { $elemMatch: profile } })), - }; - - if (user.email) { - // Query by the email address if the user is being created with one. - query.$or.push({ email: user.email }); - } - - return query; -}; - export async function retrieveUser(mongo: Db, tenantID: string, id: string) { return collection(mongo).findOne({ tenantID, id }); } diff --git a/src/core/server/services/users/index.ts b/src/core/server/services/users/index.ts index c767f7245..0a7f42085 100644 --- a/src/core/server/services/users/index.ts +++ b/src/core/server/services/users/index.ts @@ -26,6 +26,8 @@ import { Tenant } from "talk-server/models/tenant"; import { createUserToken, deactivateUserToken, + insertUser, + InsertUserInput, LocalProfile, setUserEmail, setUserLocalProfile, @@ -35,8 +37,6 @@ import { updateUserPassword, updateUserRole, updateUserUsername, - upsertUser, - UpsertUserInput, User, } from "talk-server/models/user"; @@ -51,7 +51,7 @@ import { JWTSigningConfig, signPATString } from "../jwt"; * @param username the username to be tested */ function validateUsername(tenant: Tenant, username: string) { - // TODO: replace these static regex/length with database options in the Tenant eventually + // FIXME: replace these static regex/length with database options in the Tenant eventually if (!USERNAME_REGEX.test(username)) { throw new UsernameContainsInvalidCharactersError(); @@ -104,16 +104,16 @@ function validateEmail(tenant: Tenant, email: string) { } } -export type UpsertUser = UpsertUserInput; +export type InsertUser = InsertUserInput; /** - * upsert will upsert the User into the database for the Tenant. + * insert will upsert the User into the database for the Tenant. * * @param mongo mongo database to interact with * @param tenant Tenant where the User will be added to * @param input the input for creating the User */ -export async function upsert(mongo: Db, tenant: Tenant, input: UpsertUser) { +export async function insert(mongo: Db, tenant: Tenant, input: InsertUser) { if (input.username) { validateUsername(tenant, input.username); } @@ -134,7 +134,7 @@ export async function upsert(mongo: Db, tenant: Tenant, input: UpsertUser) { } } - const user = await upsertUser(mongo, tenant.id, input); + const user = await insertUser(mongo, tenant.id, input); return user; }