From fa72d5deda8662bb249bb6c2469d93267bf9a07f Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Fri, 19 Oct 2018 16:05:58 -0600 Subject: [PATCH 01/18] feat: support new auth methods for Tenants - New Time scalar type is implemented on the Server - Single Sign-On keys can now be generated - Single Sign-On keys can be regenerated - Single Sign-On keys now store the date they were generated on. - Initial implementation of `AuthenticationTargetFilter`'s --- scripts/generateSchemaTypes.js | 3 +- src/core/server/graph/common/scalars/time.ts | 36 +++++ .../graph/management/resolvers/index.ts | 6 +- .../server/graph/tenant/mutators/settings.ts | 4 +- .../server/graph/tenant/resolvers/index.ts | 3 + .../server/graph/tenant/resolvers/mutation.ts | 4 + .../server/graph/tenant/schema/schema.graphql | 134 +++++++++++++++++- src/core/server/models/tenant.ts | 42 ++++++ src/core/server/services/tenant/index.ts | 22 +++ 9 files changed, 249 insertions(+), 5 deletions(-) create mode 100644 src/core/server/graph/common/scalars/time.ts diff --git a/scripts/generateSchemaTypes.js b/scripts/generateSchemaTypes.js index c52393830..11e373843 100644 --- a/scripts/generateSchemaTypes.js +++ b/scripts/generateSchemaTypes.js @@ -37,7 +37,7 @@ async function main() { 'import { Cursor } from "talk-server/models/connection";', 'import TenantContext from "talk-server/graph/tenant/context";', ], - customScalarType: { Cursor: "Cursor", Time: "string" }, + customScalarType: { Cursor: "Cursor", Time: "Date" }, }, }, { @@ -48,6 +48,7 @@ async function main() { importStatements: [ 'import ManagementContext from "talk-server/graph/management/context";', ], + customScalarType: { Time: "Date" }, }, }, ]; diff --git a/src/core/server/graph/common/scalars/time.ts b/src/core/server/graph/common/scalars/time.ts new file mode 100644 index 000000000..66e5c9fc0 --- /dev/null +++ b/src/core/server/graph/common/scalars/time.ts @@ -0,0 +1,36 @@ +import { GraphQLScalarType } from "graphql"; +import { Kind } from "graphql/language"; +import { DateTime } from "luxon"; + +export default new GraphQLScalarType({ + name: "Time", + description: "Time represented as an ISO8601 string.", + serialize(value) { + if (typeof value === "string") { + return value; + } + + return value.toISOString(); + }, + parseValue(value) { + return new Date(value); + }, + parseLiteral(ast) { + switch (ast.kind) { + case Kind.STRING: + // This handles an empty string. + if (ast.value && ast.value.length === 0) { + return null; + } + + const date = DateTime.fromISO(ast.value, {}); + if (!date.isValid) { + return null; + } + + return date.toJSDate(); + default: + return null; + } + }, +}); diff --git a/src/core/server/graph/management/resolvers/index.ts b/src/core/server/graph/management/resolvers/index.ts index b1bcbcef8..7cb79b325 100644 --- a/src/core/server/graph/management/resolvers/index.ts +++ b/src/core/server/graph/management/resolvers/index.ts @@ -1,5 +1,9 @@ +import Time from "talk-server/graph/common/scalars/time"; + import { GQLResolver } from "talk-server/graph/management/schema/__generated__/types"; -const Resolvers: GQLResolver = {}; +const Resolvers: GQLResolver = { + Time, +}; export default Resolvers; diff --git a/src/core/server/graph/tenant/mutators/settings.ts b/src/core/server/graph/tenant/mutators/settings.ts index 19a7c04e0..d678f5c61 100644 --- a/src/core/server/graph/tenant/mutators/settings.ts +++ b/src/core/server/graph/tenant/mutators/settings.ts @@ -3,9 +3,11 @@ import { isNull, omitBy } from "lodash"; import TenantContext from "talk-server/graph/tenant/context"; import { GQLSettingsInput } from "talk-server/graph/tenant/schema/__generated__/types"; import { Tenant } from "talk-server/models/tenant"; -import { update } from "talk-server/services/tenant"; +import { regenerateSSOKey, update } from "talk-server/services/tenant"; export default ({ mongo, redis, tenantCache, tenant }: TenantContext) => ({ update: (input: GQLSettingsInput): Promise => update(mongo, redis, tenantCache, tenant, omitBy(input, isNull)), + regenerateSSOKey: (): Promise => + regenerateSSOKey(mongo, redis, tenantCache, tenant), }); diff --git a/src/core/server/graph/tenant/resolvers/index.ts b/src/core/server/graph/tenant/resolvers/index.ts index 999760912..8081b892b 100644 --- a/src/core/server/graph/tenant/resolvers/index.ts +++ b/src/core/server/graph/tenant/resolvers/index.ts @@ -1,4 +1,6 @@ import Cursor from "talk-server/graph/common/scalars/cursor"; +import Time from "talk-server/graph/common/scalars/time"; + import { GQLResolver } from "talk-server/graph/tenant/schema/__generated__/types"; import Asset from "./asset"; @@ -20,6 +22,7 @@ const Resolvers: GQLResolver = { Profile, Query, User, + Time, }; export default Resolvers; diff --git a/src/core/server/graph/tenant/resolvers/mutation.ts b/src/core/server/graph/tenant/resolvers/mutation.ts index 77f0ca109..1b308ff4f 100644 --- a/src/core/server/graph/tenant/resolvers/mutation.ts +++ b/src/core/server/graph/tenant/resolvers/mutation.ts @@ -47,6 +47,10 @@ const Mutation: GQLMutationTypeResolver = { comment: await ctx.mutators.Comment.deleteFlag(input), clientMutationId: input.clientMutationId, }), + regenerateSSOKey: async (source, { input }, ctx) => ({ + settings: await ctx.mutators.Settings.regenerateSSOKey(), + clientMutationId: input.clientMutationId, + }), }; export default Mutation; diff --git a/src/core/server/graph/tenant/schema/schema.graphql b/src/core/server/graph/tenant/schema/schema.graphql index 5bea357e0..4bd6d0e2f 100644 --- a/src/core/server/graph/tenant/schema/schema.graphql +++ b/src/core/server/graph/tenant/schema/schema.graphql @@ -244,12 +244,32 @@ type Wordlist { ## Auth ################################################################################ +########################## +## AuthenticationTargetFilter +########################## + +""" +AuthenticationTargetFilter when non-null, will specify the targets that a +specific authentication integration will be enabled on. +""" +type AuthenticationTargetFilter { + admin: Boolean! + stream: Boolean! +} + ########################## ## LocalAuthIntegration ########################## type LocalAuthIntegration { enabled: Boolean! + + """ + targetFilter will restrict where the authentication integration should be + displayed. If the value of targetFilter is null, then the authentication + integration should be displayed in all targets. + """ + targetFilter: AuthenticationTargetFilter } ########################## @@ -264,11 +284,23 @@ embed to allow single sign on. type SSOAuthIntegration { enabled: Boolean! + """ + targetFilter will restrict where the authentication integration should be + displayed. If the value of targetFilter is null, then the authentication + integration should be displayed in all targets. + """ + targetFilter: AuthenticationTargetFilter + """ key is the secret that is used to sign tokens. """ key: String @auth(roles: [ADMIN]) + """ + keyGeneratedAt is the Time that the key was effective from. + """ + keyGeneratedAt: Time @auth(roles: [ADMIN]) + """ displayNameEnable when enabled, will allow Users to set and view their displayName's. @@ -287,6 +319,16 @@ will be used in the admin to provide staff logins for users. type OIDCAuthIntegration { enabled: Boolean! + """ + targetFilter will restrict where the authentication integration should be + displayed. If the value of targetFilter is null, then the authentication + integration should be displayed in all targets. + """ + targetFilter: AuthenticationTargetFilter + + """ + name is the label assigned to reference the provider of the OIDC integration. + """ name: String clientID: String @auth(roles: [ADMIN]) clientSecret: String @auth(roles: [ADMIN]) @@ -308,6 +350,14 @@ type OIDCAuthIntegration { type GoogleAuthIntegration { enabled: Boolean! + + """ + targetFilter will restrict where the authentication integration should be + displayed. If the value of targetFilter is null, then the authentication + integration should be displayed in all targets. + """ + targetFilter: AuthenticationTargetFilter + clientID: String @auth(roles: [ADMIN]) clientSecret: String @auth(roles: [ADMIN]) } @@ -318,6 +368,14 @@ type GoogleAuthIntegration { type FacebookAuthIntegration { enabled: Boolean! + + """ + targetFilter will restrict where the authentication integration should be + displayed. If the value of targetFilter is null, then the authentication + integration should be displayed in all targets. + """ + targetFilter: AuthenticationTargetFilter + clientID: String @auth(roles: [ADMIN]) clientSecret: String @auth(roles: [ADMIN]) } @@ -1258,17 +1316,31 @@ input SettingsWordlistInput { suspect: [String!] } +input SettingsAuthenticationTargetFilterInput { + admin: Boolean! + stream: Boolean! +} + input SettingsLocalAuthIntegrationInput { enabled: Boolean + + """ + targetFilter will restrict where the authentication integration should be + displayed. If the value of targetFilter is null, then the authentication + integration should be displayed in all targets. + """ + targetFilter: SettingsAuthenticationTargetFilterInput } input SettingsSSOAuthIntegrationInput { enabled: Boolean """ - key is the secret that is used to sign tokens. + targetFilter will restrict where the authentication integration should be + displayed. If the value of targetFilter is null, then the authentication + integration should be displayed in all targets. """ - key: String + targetFilter: SettingsAuthenticationTargetFilterInput """ displayNameEnable when enabled, will allow Users to set and view their @@ -1280,11 +1352,23 @@ input SettingsSSOAuthIntegrationInput { input SettingsOIDCAuthIntegrationInput { enabled: Boolean + """ + targetFilter will restrict where the authentication integration should be + displayed. If the value of targetFilter is null, then the authentication + integration should be displayed in all targets. + """ + targetFilter: SettingsAuthenticationTargetFilterInput + + """ + name is the label assigned to reference the provider of the OIDC integration. + """ name: String clientID: String clientSecret: String authorizationURL: String tokenURL: String + jwksURI: String + issuer: String """ displayNameEnable when enabled, will allow Users to set and view their @@ -1295,12 +1379,28 @@ input SettingsOIDCAuthIntegrationInput { input SettingsGoogleAuthIntegrationInput { enabled: Boolean + + """ + targetFilter will restrict where the authentication integration should be + displayed. If the value of targetFilter is null, then the authentication + integration should be displayed in all targets. + """ + targetFilter: SettingsAuthenticationTargetFilterInput + clientID: String clientSecret: String } input SettingsFacebookAuthIntegrationInput { enabled: Boolean + + """ + targetFilter will restrict where the authentication integration should be + displayed. If the value of targetFilter is null, then the authentication + integration should be displayed in all targets. + """ + targetFilter: SettingsAuthenticationTargetFilterInput + clientID: String clientSecret: String } @@ -1777,6 +1877,29 @@ type DeleteCommentFlagPayload { clientMutationId: String! } +################## +## regenerateSSOKey +################## + +input RegenerateSSOKeyInput { + """ + clientMutationId is required for Relay support. + """ + clientMutationId: String! +} + +type RegenerateSSOKeyPayload { + """ + settings is the Settings that the SSO key was regenerated on. + """ + settings: Settings + + """ + clientMutationId is required for Relay support. + """ + clientMutationId: String! +} + ################## ## Mutation ################## @@ -1799,6 +1922,13 @@ type Mutation { updateSettings(input: UpdateSettingsInput!): UpdateSettingsPayload @auth(roles: [ADMIN, MODERATOR]) + """ + regenerateSSOKey will regenerate the SSO key used to sign secrets. This will + invalidate any existing user sessions. + """ + regenerateSSOKey(input: RegenerateSSOKeyInput!): RegenerateSSOKeyPayload + @auth(roles: [ADMIN]) + """ createCommentReaction will create a Reaction authored by the current logged in User on a Comment. diff --git a/src/core/server/models/tenant.ts b/src/core/server/models/tenant.ts index 3030e3cdd..166dcc9c2 100644 --- a/src/core/server/models/tenant.ts +++ b/src/core/server/models/tenant.ts @@ -1,3 +1,4 @@ +import crypto from "crypto"; import { Db } from "mongodb"; import uuid from "uuid"; @@ -203,3 +204,44 @@ export async function updateTenant( return result.value || null; } + +/** + * regenerateTenantSSOKey will regenerate the SSO key used for Single Sing-On + * for the specified Tenant. All existing user sessions signed with the old + * secret will be invalidated. + */ +export async function regenerateTenantSSOKey(db: Db, id: string) { + // Generate a new key. We generate a key of minimum length 32 up to 37 bytes, + // as 16 was the minimum length recommended. + // + // Reference: https://security.stackexchange.com/a/96176 + const key = crypto + .randomBytes(32 + Math.floor(Math.random() * 5)) + .toString("hex"); + + // Construct the update. + const update: DeepPartial = { + auth: { + integrations: { + sso: { + key, + keyGeneratedAt: new Date(), + }, + }, + }, + }; + + // Update the Tenant with this new key. + const result = await collection(db).findOneAndUpdate( + { id }, + // Serialize the deep update into the Tenant. + { + $set: dotize(update), + }, + // False to return the updated document instead of the original + // document. + { returnOriginal: false } + ); + + return result.value || null; +} diff --git a/src/core/server/services/tenant/index.ts b/src/core/server/services/tenant/index.ts index 54ab1d5a0..8bcfca5eb 100644 --- a/src/core/server/services/tenant/index.ts +++ b/src/core/server/services/tenant/index.ts @@ -5,6 +5,7 @@ import { GQLSettingsInput } from "talk-server/graph/tenant/schema/__generated__/ import { createTenant, CreateTenantInput, + regenerateTenantSSOKey, Tenant, updateTenant, } from "talk-server/models/tenant"; @@ -76,3 +77,24 @@ export async function canInstall(cache: TenantCache) { export async function isInstalled(cache: TenantCache) { return (await cache.count()) > 0; } + +/** + * regenerateSSOKey will regenerate the Single Sign-On key for the specified + * Tenant and notify all other Tenant's connected that the Tenant was updated. + */ +export async function regenerateSSOKey( + mongo: Db, + redis: Redis, + cache: TenantCache, + tenant: Tenant +) { + const updatedTenant = await regenerateTenantSSOKey(mongo, tenant.id); + if (!updatedTenant) { + return null; + } + + // Update the tenant cache. + await cache.update(redis, updatedTenant); + + return updatedTenant; +} From 3b6f3c9ea89de0ded46af09f78518145abe1d352 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Mon, 22 Oct 2018 15:52:17 -0600 Subject: [PATCH 02/18] fix: rename wordlist to wordList --- .../app/middleware/passport/strategies/oidc.ts | 3 --- src/core/server/graph/tenant/schema/schema.graphql | 14 +++++++------- src/core/server/models/settings.ts | 6 +++--- src/core/server/models/tenant.ts | 2 +- src/core/server/services/comments/index.ts | 1 - .../services/comments/moderation/phases/index.ts | 10 +++++----- .../phases/{premod.ts => preModerate.ts} | 2 +- .../comments/moderation/phases/wordlist.ts | 10 +++++----- 8 files changed, 22 insertions(+), 26 deletions(-) rename src/core/server/services/comments/moderation/phases/{premod.ts => preModerate.ts} (94%) diff --git a/src/core/server/app/middleware/passport/strategies/oidc.ts b/src/core/server/app/middleware/passport/strategies/oidc.ts index b810a18f9..f4d07a108 100644 --- a/src/core/server/app/middleware/passport/strategies/oidc.ts +++ b/src/core/server/app/middleware/passport/strategies/oidc.ts @@ -209,9 +209,6 @@ export default class OIDCStrategy extends Strategy { this.mongo = mongo; this.cache = new TenantCacheAdapter(tenantCache); - - // Connect the cache adapter. - this.cache.subscribe(); } private lookupJWKSClient( diff --git a/src/core/server/graph/tenant/schema/schema.graphql b/src/core/server/graph/tenant/schema/schema.graphql index 4bd6d0e2f..268e303c4 100644 --- a/src/core/server/graph/tenant/schema/schema.graphql +++ b/src/core/server/graph/tenant/schema/schema.graphql @@ -226,9 +226,9 @@ enum MODERATION_MODE { } """ -Wordlist describes all the available wordlists. +WordList describes all the available wordLists. """ -type Wordlist { +type WordList { """ banned words will by default reject the comment if it is found. """ @@ -704,9 +704,9 @@ type Settings { email: Email! @auth(roles: [ADMIN]) """ - wordlist will return a given list of words. + wordList will return a given list of words. """ - wordlist: Wordlist! @auth(roles: [ADMIN, MODERATOR]) + wordList: WordList! @auth(roles: [ADMIN, MODERATOR]) """ auth contains all the settings related to authentication and authorization. @@ -1304,7 +1304,7 @@ input SettingsEmailInput { fromAddress: String } -input SettingsWordlistInput { +input SettingsWordListInput { """ banned words will by default reject the comment if it is found. """ @@ -1641,9 +1641,9 @@ input SettingsInput { organizationContactEmail: String """ - wordlist will return a given list of words. + wordList will return a given list of words. """ - wordlist: SettingsWordlistInput + wordList: SettingsWordListInput """ email is the set of credentials and settings associated with the organization. diff --git a/src/core/server/models/settings.ts b/src/core/server/models/settings.ts index 6434f30c8..4091b8f37 100644 --- a/src/core/server/models/settings.ts +++ b/src/core/server/models/settings.ts @@ -6,7 +6,7 @@ import { GQLKarma, GQLMODERATION_MODE, GQLReactionConfiguration, - GQLWordlist, + GQLWordList, } from "talk-server/graph/tenant/schema/__generated__/types"; // export interface EmailDomainRuleCondition { @@ -89,9 +89,9 @@ export interface Settings extends ModerationSettings { karma: GQLKarma; /** - * wordlist stores all the banned/suspect words. + * wordList stores all the banned/suspect words. */ - wordlist: GQLWordlist; + wordList: GQLWordList; /** * Set of configured authentication integrations. diff --git a/src/core/server/models/tenant.ts b/src/core/server/models/tenant.ts index 166dcc9c2..aa66d8229 100644 --- a/src/core/server/models/tenant.ts +++ b/src/core/server/models/tenant.ts @@ -75,7 +75,7 @@ export async function createTenant(mongo: Db, input: CreateTenantInput) { charCount: { enabled: false, }, - wordlist: { + wordList: { suspect: [], banned: [], }, diff --git a/src/core/server/services/comments/index.ts b/src/core/server/services/comments/index.ts index 82e10c97c..3de8b8ee5 100644 --- a/src/core/server/services/comments/index.ts +++ b/src/core/server/services/comments/index.ts @@ -92,7 +92,6 @@ export async function create( // Insert and handle creating the actions. comment = await addCommentActions(mongo, tenant, comment, inputs); - // asse } if (input.parent_id) { diff --git a/src/core/server/services/comments/moderation/phases/index.ts b/src/core/server/services/comments/moderation/phases/index.ts index a0a724cc6..973e615c6 100644 --- a/src/core/server/services/comments/moderation/phases/index.ts +++ b/src/core/server/services/comments/moderation/phases/index.ts @@ -1,15 +1,15 @@ import { IntermediateModerationPhase } from "talk-server/services/comments/moderation"; -import { premod } from "talk-server/services/comments/moderation/phases/premod"; -import { toxic } from "talk-server/services/comments/moderation/phases/toxic"; import { assetClosed } from "./assetClosed"; import { commentingDisabled } from "./commentingDisabled"; import { commentLength } from "./commentLength"; import { karma } from "./karma"; import { links } from "./links"; +import { preModerate } from "./preModerate"; import { spam } from "./spam"; import { staff } from "./staff"; -import { wordlist } from "./wordlist"; +import { toxic } from "./toxic"; +import { wordList } from "./wordList"; /** * The moderation phases to apply for each comment being processed. @@ -18,11 +18,11 @@ export const moderationPhases: IntermediateModerationPhase[] = [ commentLength, assetClosed, commentingDisabled, - wordlist, + wordList, staff, links, karma, spam, toxic, - premod, + preModerate, ]; diff --git a/src/core/server/services/comments/moderation/phases/premod.ts b/src/core/server/services/comments/moderation/phases/preModerate.ts similarity index 94% rename from src/core/server/services/comments/moderation/phases/premod.ts rename to src/core/server/services/comments/moderation/phases/preModerate.ts index 7ccdcfb73..da1cd2420 100755 --- a/src/core/server/services/comments/moderation/phases/premod.ts +++ b/src/core/server/services/comments/moderation/phases/preModerate.ts @@ -13,7 +13,7 @@ const testModerationMode = (settings: Partial) => // This phase checks to see if the settings have premod enabled, if they do, // the comment is premod, otherwise, it's just none. -export const premod: IntermediateModerationPhase = ({ +export const preModerate: IntermediateModerationPhase = ({ asset, tenant, }): IntermediatePhaseResult | void => { diff --git a/src/core/server/services/comments/moderation/phases/wordlist.ts b/src/core/server/services/comments/moderation/phases/wordlist.ts index 05f6431b9..71f8eb860 100755 --- a/src/core/server/services/comments/moderation/phases/wordlist.ts +++ b/src/core/server/services/comments/moderation/phases/wordlist.ts @@ -9,8 +9,8 @@ import { } from "talk-server/services/comments/moderation"; import { containsMatchingPhrase } from "talk-server/services/comments/moderation/wordlist"; -// This phase checks the comment against the wordlist. -export const wordlist: IntermediateModerationPhase = ({ +// This phase checks the comment against the wordList. +export const wordList: IntermediateModerationPhase = ({ tenant, comment, }): IntermediatePhaseResult | void => { @@ -21,9 +21,9 @@ export const wordlist: IntermediateModerationPhase = ({ // Decide the status based on whether or not the current asset/settings // has pre-mod enabled or not. If the comment was rejected based on the - // wordlist, then reject it, otherwise if the moderation setting is + // wordList, then reject it, otherwise if the moderation setting is // premod, set it to `premod`. - if (containsMatchingPhrase(tenant.wordlist.banned, comment.body)) { + if (containsMatchingPhrase(tenant.wordList.banned, comment.body)) { // Add the flag related to Trust to the comment. return { status: GQLCOMMENT_STATUS.REJECTED, @@ -42,7 +42,7 @@ export const wordlist: IntermediateModerationPhase = ({ // If the wordlist has matched the suspect word filter and we haven't disabled // auto-flagging suspect words, then we should flag the comment! - if (containsMatchingPhrase(tenant.wordlist.suspect, comment.body)) { + if (containsMatchingPhrase(tenant.wordList.suspect, comment.body)) { return { actions: [ { From 0bb64f1d9700132c911f906695869ecbcf6fb47b Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Tue, 23 Oct 2018 15:27:23 -0600 Subject: [PATCH 03/18] feat: added OIDC discovery --- .../passport/strategies/oidc/discover.ts | 47 +++++++++++++++++++ .../strategies/{oidc.ts => oidc/index.ts} | 0 .../strategies/{ => oidc}/oidc.spec.ts | 0 .../server/graph/tenant/mutators/settings.ts | 15 +++++- .../server/graph/tenant/resolvers/mutation.ts | 4 ++ .../server/graph/tenant/schema/schema.graphql | 39 +++++++++++++++ src/core/server/services/tenant/index.ts | 18 +++++++ src/types/webfinger.d.ts | 16 ------- 8 files changed, 121 insertions(+), 18 deletions(-) create mode 100644 src/core/server/app/middleware/passport/strategies/oidc/discover.ts rename src/core/server/app/middleware/passport/strategies/{oidc.ts => oidc/index.ts} (100%) rename src/core/server/app/middleware/passport/strategies/{ => oidc}/oidc.spec.ts (100%) delete mode 100644 src/types/webfinger.d.ts diff --git a/src/core/server/app/middleware/passport/strategies/oidc/discover.ts b/src/core/server/app/middleware/passport/strategies/oidc/discover.ts new file mode 100644 index 000000000..afc14afc6 --- /dev/null +++ b/src/core/server/app/middleware/passport/strategies/oidc/discover.ts @@ -0,0 +1,47 @@ +import fetch from "node-fetch"; +import { URL } from "url"; + +/** + * Configuration that Talk is expecting. + */ +export interface DiscoveryConfiguration { + authorizationURL?: string; + tokenURL?: string; + jwksURI?: string; +} + +/** + * Subset of configuration as defined in the set of Provider Metadata: + * + * https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata + */ +interface DiscoveryRawConfiguration { + authorization_endpoint?: string; + token_endpoint?: string; + jwks_uri?: string; +} + +/** + * discover will discover the configuration for the issuer. + * + * @param issuer the Issuer URL that should be used to determine the + * configuration + */ +export async function discover(issuer: URL): Promise { + // Any provider MUST provide a .well-known url that is JSON parsable based + // on the issuer: https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfig + const configurationURL = + issuer.origin + + issuer.pathname.replace(/\/$/, "") + + "/.well-known/openid-configuration"; + const res = await fetch(configurationURL); + + // Parse the configuration + const meta: DiscoveryRawConfiguration = await res.json(); + + return { + authorizationURL: meta.authorization_endpoint, + tokenURL: meta.token_endpoint, + jwksURI: meta.jwks_uri, + }; +} diff --git a/src/core/server/app/middleware/passport/strategies/oidc.ts b/src/core/server/app/middleware/passport/strategies/oidc/index.ts similarity index 100% rename from src/core/server/app/middleware/passport/strategies/oidc.ts rename to src/core/server/app/middleware/passport/strategies/oidc/index.ts diff --git a/src/core/server/app/middleware/passport/strategies/oidc.spec.ts b/src/core/server/app/middleware/passport/strategies/oidc/oidc.spec.ts similarity index 100% rename from src/core/server/app/middleware/passport/strategies/oidc.spec.ts rename to src/core/server/app/middleware/passport/strategies/oidc/oidc.spec.ts diff --git a/src/core/server/graph/tenant/mutators/settings.ts b/src/core/server/graph/tenant/mutators/settings.ts index d678f5c61..079a9389e 100644 --- a/src/core/server/graph/tenant/mutators/settings.ts +++ b/src/core/server/graph/tenant/mutators/settings.ts @@ -1,13 +1,24 @@ import { isNull, omitBy } from "lodash"; import TenantContext from "talk-server/graph/tenant/context"; -import { GQLSettingsInput } from "talk-server/graph/tenant/schema/__generated__/types"; +import { + GQLDiscoverOIDCConfigurationInput, + GQLOIDCConfiguration, + GQLSettingsInput, +} from "talk-server/graph/tenant/schema/__generated__/types"; import { Tenant } from "talk-server/models/tenant"; -import { regenerateSSOKey, update } from "talk-server/services/tenant"; +import { + discoverOIDCConfiguration, + regenerateSSOKey, + update, +} from "talk-server/services/tenant"; export default ({ mongo, redis, tenantCache, tenant }: TenantContext) => ({ update: (input: GQLSettingsInput): Promise => update(mongo, redis, tenantCache, tenant, omitBy(input, isNull)), regenerateSSOKey: (): Promise => regenerateSSOKey(mongo, redis, tenantCache, tenant), + discoverOIDCConfiguration: ( + input: GQLDiscoverOIDCConfigurationInput + ): Promise => discoverOIDCConfiguration(input.issuer), }); diff --git a/src/core/server/graph/tenant/resolvers/mutation.ts b/src/core/server/graph/tenant/resolvers/mutation.ts index 1b308ff4f..7255071d7 100644 --- a/src/core/server/graph/tenant/resolvers/mutation.ts +++ b/src/core/server/graph/tenant/resolvers/mutation.ts @@ -51,6 +51,10 @@ const Mutation: GQLMutationTypeResolver = { settings: await ctx.mutators.Settings.regenerateSSOKey(), clientMutationId: input.clientMutationId, }), + discoverOIDCConfiguration: async (source, { input }, ctx) => ({ + configuration: await ctx.mutators.Settings.discoverOIDCConfiguration(input), + clientMutationId: input.clientMutationId, + }), }; export default Mutation; diff --git a/src/core/server/graph/tenant/schema/schema.graphql b/src/core/server/graph/tenant/schema/schema.graphql index 268e303c4..65cde5509 100644 --- a/src/core/server/graph/tenant/schema/schema.graphql +++ b/src/core/server/graph/tenant/schema/schema.graphql @@ -1900,6 +1900,41 @@ type RegenerateSSOKeyPayload { clientMutationId: String! } +################## +## discoverOIDCConfiguration +################## + +input DiscoverOIDCConfigurationInput { + """ + + """ + issuer: String! + + """ + clientMutationId is required for Relay support. + """ + clientMutationId: String! +} + +type OIDCConfiguration { + authorizationURL: String + tokenURL: String + jwksURI: String +} + +type DiscoverOIDCConfigurationPayload { + """ + configuration was the discovered configuration for the OpenID Connect server + given the provided issuer. + """ + configuration: OIDCConfiguration + + """ + clientMutationId is required for Relay support. + """ + clientMutationId: String! +} + ################## ## Mutation ################## @@ -1929,6 +1964,10 @@ type Mutation { regenerateSSOKey(input: RegenerateSSOKeyInput!): RegenerateSSOKeyPayload @auth(roles: [ADMIN]) + discoverOIDCConfiguration( + input: DiscoverOIDCConfigurationInput! + ): DiscoverOIDCConfigurationPayload @auth(roles: [ADMIN]) + """ createCommentReaction will create a Reaction authored by the current logged in User on a Comment. diff --git a/src/core/server/services/tenant/index.ts b/src/core/server/services/tenant/index.ts index 8bcfca5eb..2d0f634a9 100644 --- a/src/core/server/services/tenant/index.ts +++ b/src/core/server/services/tenant/index.ts @@ -1,5 +1,6 @@ import { Redis } from "ioredis"; import { Db } from "mongodb"; +import { URL } from "url"; import { GQLSettingsInput } from "talk-server/graph/tenant/schema/__generated__/types"; import { @@ -10,6 +11,7 @@ import { updateTenant, } from "talk-server/models/tenant"; +import { discover } from "talk-server/app/middleware/passport/strategies/oidc/discover"; import logger from "talk-server/logger"; import TenantCache from "./cache"; @@ -98,3 +100,19 @@ export async function regenerateSSOKey( return updatedTenant; } + +/** + * discoverOIDCConfiguration will discover the OpenID Connect configuration as + * is required by any OpenID Connect compatible service: + * + * https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfig + * + * @param issuerString the issuer that should be used as the discovery root. + */ +export async function discoverOIDCConfiguration(issuerString: string) { + // Parse the issuer. + const issuer = new URL(issuerString); + + // Discover the configuration. + return discover(issuer); +} diff --git a/src/types/webfinger.d.ts b/src/types/webfinger.d.ts deleted file mode 100644 index a13bfb998..000000000 --- a/src/types/webfinger.d.ts +++ /dev/null @@ -1,16 +0,0 @@ -declare module "webfinger" { - export interface WebfingerOptions { - webfingerOnly?: boolean; - } - - export interface WebfingerCallback { - (err: Error, jrd: { [key: string]: any }): void; - } - - export function webfinger( - resource: string, - res: string, - options: WebfingerOptions, - callback: WebfingerCallback - ): void; -} From 45cbac797278c273c150a8e3c438342f6ddf486b Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Wed, 24 Oct 2018 16:54:07 -0600 Subject: [PATCH 04/18] feat: added OIDC management mutations --- .../passport/strategies/oidc/discover.ts | 3 + .../passport/strategies/oidc/index.ts | 92 +++++--- src/core/server/app/router/api/auth.ts | 7 +- src/core/server/app/url.ts | 13 ++ .../server/graph/tenant/mutators/settings.ts | 31 +++ .../server/graph/tenant/resolvers/index.ts | 4 +- .../server/graph/tenant/resolvers/mutation.ts | 12 + .../tenant/resolvers/oidc_auth_integration.ts | 26 +++ .../server/graph/tenant/schema/schema.graphql | 217 +++++++++++++++--- src/core/server/models/tenant.ts | 101 +++++++- src/core/server/services/tenant/index.ts | 87 ++++++- 11 files changed, 524 insertions(+), 69 deletions(-) create mode 100644 src/core/server/graph/tenant/resolvers/oidc_auth_integration.ts diff --git a/src/core/server/app/middleware/passport/strategies/oidc/discover.ts b/src/core/server/app/middleware/passport/strategies/oidc/discover.ts index afc14afc6..ab12fa59c 100644 --- a/src/core/server/app/middleware/passport/strategies/oidc/discover.ts +++ b/src/core/server/app/middleware/passport/strategies/oidc/discover.ts @@ -5,6 +5,7 @@ import { URL } from "url"; * Configuration that Talk is expecting. */ export interface DiscoveryConfiguration { + issuer: string; authorizationURL?: string; tokenURL?: string; jwksURI?: string; @@ -16,6 +17,7 @@ export interface DiscoveryConfiguration { * https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata */ interface DiscoveryRawConfiguration { + issuer: string; authorization_endpoint?: string; token_endpoint?: string; jwks_uri?: string; @@ -40,6 +42,7 @@ export async function discover(issuer: URL): Promise { const meta: DiscoveryRawConfiguration = await res.json(); return { + issuer: meta.issuer, authorizationURL: meta.authorization_endpoint, tokenURL: meta.token_endpoint, jwksURI: meta.jwks_uri, 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 f4d07a108..4405e3697 100644 --- a/src/core/server/app/middleware/passport/strategies/oidc/index.ts +++ b/src/core/server/app/middleware/passport/strategies/oidc/index.ts @@ -32,23 +32,26 @@ export interface OIDCIDToken { iss: string; sub: string; exp: number; // TODO: use this as the source for how long an OIDC user can be logged in for - email?: string; + email: string; email_verified?: boolean; picture?: string; name?: string; nickname?: string; } -export interface StrategyItem { - strategy: OAuth2Strategy; - jwksClient?: JwksClient; -} +export type StrategyItem = Record< + string, + { + strategy: OAuth2Strategy; + jwksClient?: JwksClient; + } +>; export function isOIDCToken(token: OIDCIDToken | object): token is OIDCIDToken { if ( (token as OIDCIDToken).iss && (token as OIDCIDToken).sub && - (token as OIDCIDToken).email + (token as OIDCIDToken).aud ) { return true; } @@ -85,9 +88,18 @@ const signingKeyFactory = (client: jwks.JwksClient): jwt.KeyFunction => ( }; function getEnabledIntegration( - tenant: Tenant + tenant: Tenant, + oidcID: string ): Required { - const integration = tenant.auth.integrations.oidc; + if (!tenant.auth.integrations.oidc) { + // TODO: return a better error. + throw new Error("integration not found"); + } + + // Grab the OIDC Integration from the list of integrations. + const integration = tenant.auth.integrations.oidc.find( + ({ id }) => id === oidcID + ); if (!integration) { // TODO: return a better error. throw new Error("integration not found"); @@ -135,6 +147,7 @@ export const OIDCDisplayNameIDTokenSchema = OIDCIDTokenSchema.keys({ export async function findOrCreateOIDCUser( db: Db, tenant: Tenant, + integration: GQLOIDCAuthIntegration, token: OIDCIDToken ) { // Unpack/validate the token content. @@ -148,7 +161,7 @@ export async function findOrCreateOIDCUser( name, nickname, }: OIDCIDToken = validate( - tenant.auth.integrations.oidc!.displayNameEnable + integration.displayNameEnable ? OIDCDisplayNameIDTokenSchema : OIDCIDTokenSchema, token @@ -215,34 +228,45 @@ export default class OIDCStrategy extends Strategy { req: Request, tenantID: string, oidc: Required - ) { - let entry = this.cache.get(tenantID); - if (!entry) { + ): jwks.JwksClient { + let tenantIntegrations = this.cache.get(tenantID); + if (!tenantIntegrations || !tenantIntegrations[oidc.id]) { const strategy = this.createStrategy(req, oidc); // Create the entry. - entry = { - strategy, + tenantIntegrations = { + [oidc.id]: { + strategy, + }, + ...(tenantIntegrations || {}), }; // We don't reset the entry in the cache here because if we just created // it, we'll be creating the jwksClient anyways, so we'll update it there. } - if (!entry.jwksClient) { + const tenantIntegration = tenantIntegrations[oidc.id]; + + if (!tenantIntegration.jwksClient) { // Create the new JWKS client. const jwksClient = jwks({ jwksUri: oidc.jwksURI, }); // Set the jwksClient on the entry. - entry.jwksClient = jwksClient; + tenantIntegration.jwksClient = jwksClient; // Update the cached entry. - this.cache.set(tenantID, entry); + this.cache.set(tenantID, { + [oidc.id]: { + ...tenantIntegration, + jwksClient, + }, + ...tenantIntegrations, + }); } - return entry.jwksClient; + return tenantIntegration.jwksClient; } private userAuthenticatedCallback = ( @@ -268,11 +292,14 @@ export default class OIDCStrategy extends Strategy { return done(new Error("tenant not found")); } + // Grab the OIDC ID from the request. + const { oidcID }: { oidcID: string } = req.params; + // Get the integration from the tenant. If needed, it will be used to create // a new strategy. let integration: Required; try { - integration = getEnabledIntegration(tenant); + integration = getEnabledIntegration(tenant, oidcID); } catch (err) { // TODO: wrap error? return done(err); @@ -298,6 +325,7 @@ export default class OIDCStrategy extends Strategy { const user = await findOrCreateOIDCUser( this.mongo, tenant, + integration, decoded as OIDCIDToken ); return done(null, user); @@ -332,39 +360,45 @@ export default class OIDCStrategy extends Strategy { ); } - private async lookupStrategy(req: Request) { + private lookupStrategy(req: Request): OAuth2Strategy { const { tenant } = req.talk!; if (!tenant) { // TODO: return a better error. throw new Error("tenant not found"); } + // Get the OIDC ID. + const { oidcID }: { oidcID: string } = req.params; + // 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, oidcID); // Try to get the Tenant's cached integrations. - let entry = this.cache.get(tenant.id); - if (!entry) { + let tenantIntegrations = this.cache.get(tenant.id); + if (!tenantIntegrations || !tenantIntegrations[oidcID]) { // Create the strategy. const strategy = this.createStrategy(req, integration); // Reset the entry. - entry = { - strategy, + tenantIntegrations = { + [oidcID]: { + strategy, + }, + ...(tenantIntegrations || {}), }; // Update the cached integrations value. - this.cache.set(tenant.id, entry); + this.cache.set(tenant.id, tenantIntegrations); } - return entry.strategy; + return tenantIntegrations[oidcID].strategy; } - public async authenticate(req: Request) { + public authenticate(req: Request) { try { // Lookup the strategy. - const strategy = await this.lookupStrategy(req); + const strategy = this.lookupStrategy(req); if (!strategy) { throw new Error("strategy not found"); } diff --git a/src/core/server/app/router/api/auth.ts b/src/core/server/app/router/api/auth.ts index df7f7a05b..37a1ddfff 100644 --- a/src/core/server/app/router/api/auth.ts +++ b/src/core/server/app/router/api/auth.ts @@ -29,9 +29,12 @@ export function createNewAuthRouter(app: AppOptions, options: RouterOptions) { signupHandler({ db: app.mongo, signingConfig: app.signingConfig }) ); - router.get("/oidc", wrapAuthn(options.passport, app.signingConfig, "oidc")); router.get( - "/oidc/callback", + "/oidc/:oidcID", + wrapAuthn(options.passport, app.signingConfig, "oidc") + ); + router.get( + "/oidc/:oidcID/callback", wrapAuthn(options.passport, app.signingConfig, "oidc") ); diff --git a/src/core/server/app/url.ts b/src/core/server/app/url.ts index 3654c4c46..a37def2c5 100644 --- a/src/core/server/app/url.ts +++ b/src/core/server/app/url.ts @@ -1,3 +1,4 @@ +import { Tenant } from "talk-server/models/tenant"; import { Request } from "talk-server/types/express"; import { URL } from "url"; @@ -11,6 +12,18 @@ export function reconstructURL(req: Request, path: string = "/"): string { return url.href; } +/** + * reconstructTenantURL will reconstruct a URL based off of the Tenant's domain. + */ +export function reconstructTenantURL( + tenant: Pick, + path: string = "/" +): string { + const url = new URL(path, `https://${tenant.domain}`); + + return url.href; +} + export function doesRequireSchemePrefixing(url: string) { return !url.startsWith("http"); } diff --git a/src/core/server/graph/tenant/mutators/settings.ts b/src/core/server/graph/tenant/mutators/settings.ts index 079a9389e..d1ceaaef3 100644 --- a/src/core/server/graph/tenant/mutators/settings.ts +++ b/src/core/server/graph/tenant/mutators/settings.ts @@ -2,15 +2,21 @@ import { isNull, omitBy } from "lodash"; import TenantContext from "talk-server/graph/tenant/context"; import { + GQLCreateOIDCAuthIntegrationInput, + GQLDeleteOIDCAuthIntegrationInput, GQLDiscoverOIDCConfigurationInput, GQLOIDCConfiguration, GQLSettingsInput, + GQLUpdateOIDCAuthIntegrationInput, } from "talk-server/graph/tenant/schema/__generated__/types"; import { Tenant } from "talk-server/models/tenant"; import { + createOIDCAuthIntegration, + deleteOIDCAuthIntegration, discoverOIDCConfiguration, regenerateSSOKey, update, + updateOIDCAuthIntegration, } from "talk-server/services/tenant"; export default ({ mongo, redis, tenantCache, tenant }: TenantContext) => ({ @@ -21,4 +27,29 @@ export default ({ mongo, redis, tenantCache, tenant }: TenantContext) => ({ discoverOIDCConfiguration: ( input: GQLDiscoverOIDCConfigurationInput ): Promise => discoverOIDCConfiguration(input.issuer), + createOIDCAuthIntegration: ( + input: GQLCreateOIDCAuthIntegrationInput + ): Promise => + createOIDCAuthIntegration( + mongo, + redis, + tenantCache, + tenant, + input.configuration + ), + updateOIDCAuthIntegration: ( + input: GQLUpdateOIDCAuthIntegrationInput + ): Promise => + updateOIDCAuthIntegration( + mongo, + redis, + tenantCache, + tenant, + input.id, + input.configuration + ), + deleteOIDCAuthIntegration: ( + input: GQLDeleteOIDCAuthIntegrationInput + ): Promise => + deleteOIDCAuthIntegration(mongo, redis, tenantCache, tenant, input.id), }); diff --git a/src/core/server/graph/tenant/resolvers/index.ts b/src/core/server/graph/tenant/resolvers/index.ts index 8081b892b..036a35857 100644 --- a/src/core/server/graph/tenant/resolvers/index.ts +++ b/src/core/server/graph/tenant/resolvers/index.ts @@ -8,6 +8,7 @@ import AuthIntegrations from "./auth_integrations"; import Comment from "./comment"; import CommentCounts from "./comment_counts"; import Mutation from "./mutation"; +import OIDCAuthIntegration from "./oidc_auth_integration"; import Profile from "./profile"; import Query from "./query"; import User from "./user"; @@ -19,10 +20,11 @@ const Resolvers: GQLResolver = { CommentCounts, Cursor, Mutation, + OIDCAuthIntegration, Profile, Query, - User, Time, + User, }; export default Resolvers; diff --git a/src/core/server/graph/tenant/resolvers/mutation.ts b/src/core/server/graph/tenant/resolvers/mutation.ts index 7255071d7..7781d7689 100644 --- a/src/core/server/graph/tenant/resolvers/mutation.ts +++ b/src/core/server/graph/tenant/resolvers/mutation.ts @@ -55,6 +55,18 @@ const Mutation: GQLMutationTypeResolver = { configuration: await ctx.mutators.Settings.discoverOIDCConfiguration(input), clientMutationId: input.clientMutationId, }), + createOIDCAuthIntegration: async (source, { input }, ctx) => ({ + settings: await ctx.mutators.Settings.createOIDCAuthIntegration(input), + clientMutationId: input.clientMutationId, + }), + updateOIDCAuthIntegration: async (source, { input }, ctx) => ({ + settings: await ctx.mutators.Settings.updateOIDCAuthIntegration(input), + clientMutationId: input.clientMutationId, + }), + deleteOIDCAuthIntegration: async (source, { input }, ctx) => ({ + settings: await ctx.mutators.Settings.deleteOIDCAuthIntegration(input), + clientMutationId: input.clientMutationId, + }), }; export default Mutation; diff --git a/src/core/server/graph/tenant/resolvers/oidc_auth_integration.ts b/src/core/server/graph/tenant/resolvers/oidc_auth_integration.ts new file mode 100644 index 000000000..9e2890222 --- /dev/null +++ b/src/core/server/graph/tenant/resolvers/oidc_auth_integration.ts @@ -0,0 +1,26 @@ +import { reconstructTenantURL, reconstructURL } from "talk-server/app/url"; +import { + GQLOIDCAuthIntegration, + GQLOIDCAuthIntegrationTypeResolver, +} from "talk-server/graph/tenant/schema/__generated__/types"; + +const OIDCAuthIntegration: GQLOIDCAuthIntegrationTypeResolver< + GQLOIDCAuthIntegration +> = { + callbackURL: (integration, args, ctx) => { + const path = `/api/tenant/auth/oidc/${integration.id}`; + + // If the request is available, then prefer it over building from the tenant + // as the tenant does not include the port number. This should only really + // be a problem if the graph API is called internally. + if (ctx.req) { + return reconstructURL(ctx.req, path); + } + + // Note that when constructing the callback url with the tenant, the port + // information is lost. + return reconstructTenantURL(ctx.tenant, path); + }, +}; + +export default OIDCAuthIntegration; diff --git a/src/core/server/graph/tenant/schema/schema.graphql b/src/core/server/graph/tenant/schema/schema.graphql index 65cde5509..b29921796 100644 --- a/src/core/server/graph/tenant/schema/schema.graphql +++ b/src/core/server/graph/tenant/schema/schema.graphql @@ -317,6 +317,14 @@ OIDCAuthIntegration provides a way to store Open ID Connect credentials. This will be used in the admin to provide staff logins for users. """ type OIDCAuthIntegration { + """ + id is the identifier of the OIDCAuthIntegration. + """ + id: ID! + + """ + enabled, when true, allows the integration to be enabled. + """ enabled: Boolean! """ @@ -330,12 +338,20 @@ type OIDCAuthIntegration { name is the label assigned to reference the provider of the OIDC integration. """ name: String - clientID: String @auth(roles: [ADMIN]) - clientSecret: String @auth(roles: [ADMIN]) - authorizationURL: String @auth(roles: [ADMIN]) - tokenURL: String @auth(roles: [ADMIN]) - jwksURI: String @auth(roles: [ADMIN]) - issuer: String @auth(roles: [ADMIN]) + + """ + callbackURL is the URL that the user should be redirected to in order to start + an authentication flow with the given integration. This field is not stored, + and is instead computed from the Tenant. + """ + callbackURL: String! + + clientID: String! @auth(roles: [ADMIN]) + clientSecret: String! @auth(roles: [ADMIN]) + authorizationURL: String! @auth(roles: [ADMIN]) + tokenURL: String! @auth(roles: [ADMIN]) + jwksURI: String! @auth(roles: [ADMIN]) + issuer: String! @auth(roles: [ADMIN]) """ displayNameEnable when enabled, will allow Users to set and view their @@ -383,7 +399,7 @@ type FacebookAuthIntegration { type AuthIntegrations { local: LocalAuthIntegration! sso: SSOAuthIntegration! - oidc: OIDCAuthIntegration! + oidc: [OIDCAuthIntegration!]! google: GoogleAuthIntegration! facebook: FacebookAuthIntegration! } @@ -1349,33 +1365,33 @@ input SettingsSSOAuthIntegrationInput { displayNameEnable: Boolean } -input SettingsOIDCAuthIntegrationInput { - enabled: Boolean +# input SettingsOIDCAuthIntegrationInput { +# enabled: Boolean - """ - targetFilter will restrict where the authentication integration should be - displayed. If the value of targetFilter is null, then the authentication - integration should be displayed in all targets. - """ - targetFilter: SettingsAuthenticationTargetFilterInput +# """ +# targetFilter will restrict where the authentication integration should be +# displayed. If the value of targetFilter is null, then the authentication +# integration should be displayed in all targets. +# """ +# targetFilter: SettingsAuthenticationTargetFilterInput - """ - name is the label assigned to reference the provider of the OIDC integration. - """ - name: String - clientID: String - clientSecret: String - authorizationURL: String - tokenURL: String - jwksURI: String - issuer: String +# """ +# name is the label assigned to reference the provider of the OIDC integration. +# """ +# name: String +# clientID: String +# clientSecret: String +# authorizationURL: String +# tokenURL: String +# jwksURI: String +# issuer: String - """ - displayNameEnable when enabled, will allow Users to set and view their - displayName's. - """ - displayNameEnable: Boolean -} +# """ +# displayNameEnable when enabled, will allow Users to set and view their +# displayName's. +# """ +# displayNameEnable: Boolean +# } input SettingsGoogleAuthIntegrationInput { enabled: Boolean @@ -1408,7 +1424,6 @@ input SettingsFacebookAuthIntegrationInput { input SettingsAuthIntegrationsInput { local: SettingsLocalAuthIntegrationInput sso: SettingsSSOAuthIntegrationInput - oidc: SettingsOIDCAuthIntegrationInput google: SettingsGoogleAuthIntegrationInput facebook: SettingsFacebookAuthIntegrationInput } @@ -1917,6 +1932,7 @@ input DiscoverOIDCConfigurationInput { } type OIDCConfiguration { + issuer: String! authorizationURL: String tokenURL: String jwksURI: String @@ -1935,6 +1951,117 @@ type DiscoverOIDCConfigurationPayload { clientMutationId: String! } +################## +# createOIDCAuthIntegration +################## + +input CreateOIDCAuthIntegrationConfigurationInput { + name: String! + clientID: String! + clientSecret: String! + authorizationURL: String! + tokenURL: String! + jwksURI: String! + issuer: String! +} + +input CreateOIDCAuthIntegrationInput { + """ + configuration contains the configuration to be used to create the auth integration. + """ + configuration: CreateOIDCAuthIntegrationConfigurationInput! + + """ + clientMutationId is required for Relay support. + """ + clientMutationId: String! +} + +type CreateOIDCAuthIntegrationPayload { + """ + settings is the Settings that the OIDCAuthIntegration was created on. + """ + settings: Settings + + """ + clientMutationId is required for Relay support. + """ + clientMutationId: String! +} + +################## +# updateOIDCAuthIntegration +################## + +input UpdateOIDCAuthIntegrationConfigurationInput { + enabled: Boolean + name: String + clientID: String + clientSecret: String + authorizationURL: String + tokenURL: String + jwksURI: String + issuer: String +} + +input UpdateOIDCAuthIntegrationInput { + """ + id is the ID of the specific OpenID Connect integration that we are updating. + """ + id: ID! + + """ + configuration contains the configuration to be used to update the OpenID + Connect integration. + """ + configuration: UpdateOIDCAuthIntegrationConfigurationInput! + + """ + clientMutationId is required for Relay support. + """ + clientMutationId: String! +} + +type UpdateOIDCAuthIntegrationPayload { + """ + settings is the Settings that the OIDCAuthIntegration was updated on. + """ + settings: Settings + + """ + clientMutationId is required for Relay support. + """ + clientMutationId: String! +} + +################## +# deleteOIDCAuthIntegration +################## + +input DeleteOIDCAuthIntegrationInput { + """ + id is the ID of the specific OpenID Connect integration that we are deleting. + """ + id: ID! + + """ + clientMutationId is required for Relay support. + """ + clientMutationId: String! +} + +type DeleteOIDCAuthIntegrationPayload { + """ + settings is the Settings that the OIDCAuthIntegration was deleted on. + """ + settings: Settings + + """ + clientMutationId is required for Relay support. + """ + clientMutationId: String! +} + ################## ## Mutation ################## @@ -1964,10 +2091,36 @@ type Mutation { regenerateSSOKey(input: RegenerateSSOKeyInput!): RegenerateSSOKeyPayload @auth(roles: [ADMIN]) + """ + discoverOIDCConfiguration will discover the OpenID Connect configuration based + on the provided input. + """ discoverOIDCConfiguration( input: DiscoverOIDCConfigurationInput! ): DiscoverOIDCConfigurationPayload @auth(roles: [ADMIN]) + """ + createOIDCAuthIntegration will create a OpenID Connect auth integration. + """ + createOIDCAuthIntegration( + input: CreateOIDCAuthIntegrationInput! + ): CreateOIDCAuthIntegrationPayload @auth(roles: [ADMIN]) + + """ + updateOIDCAuthIntegration will update a given OpenID Connect auth integration. + """ + updateOIDCAuthIntegration( + input: UpdateOIDCAuthIntegrationInput! + ): UpdateOIDCAuthIntegrationPayload @auth(roles: [ADMIN]) + + """ + deleteOIDCAuthIntegration will delete the specified OpenID Connect auth + integration. + """ + deleteOIDCAuthIntegration( + input: DeleteOIDCAuthIntegrationInput! + ): DeleteOIDCAuthIntegrationPayload @auth(roles: [ADMIN]) + """ createCommentReaction will create a Reaction authored by the current logged in User on a Comment. diff --git a/src/core/server/models/tenant.ts b/src/core/server/models/tenant.ts index aa66d8229..e17fa1e5f 100644 --- a/src/core/server/models/tenant.ts +++ b/src/core/server/models/tenant.ts @@ -4,7 +4,10 @@ import uuid from "uuid"; import { DeepPartial, Omit, Sub } from "talk-common/types"; import { dotize } from "talk-common/utils/dotize"; -import { GQLMODERATION_MODE } from "talk-server/graph/tenant/schema/__generated__/types"; +import { + GQLMODERATION_MODE, + GQLOIDCAuthIntegration, +} from "talk-server/graph/tenant/schema/__generated__/types"; import { Settings } from "talk-server/models/settings"; function collection(db: Db) { @@ -87,9 +90,7 @@ export async function createTenant(mongo: Db, input: CreateTenantInput) { sso: { enabled: false, }, - oidc: { - enabled: false, - }, + oidc: [], google: { enabled: false, }, @@ -245,3 +246,95 @@ export async function regenerateTenantSSOKey(db: Db, id: string) { return result.value || null; } + +export type CreateTenantOIDCAuthIntegrationInput = Omit< + GQLOIDCAuthIntegration, + "id" | "callbackURL" +>; + +export async function createTenantOIDCAuthIntegration( + mongo: Db, + id: string, + input: CreateTenantOIDCAuthIntegrationInput +) { + // Add the ID to the integration. + const integration = { + id: uuid.v4(), + ...input, + }; + + const result = await collection(mongo).findOneAndUpdate( + { id }, + // Serialize the deep update into the Tenant. + { + $push: { "auth.integrations.oidc": integration }, + }, + // False to return the updated document instead of the original + // document. + { returnOriginal: false } + ); + + return result.value || null; +} + +export type UpdateTenantOIDCAuthIntegrationInput = Partial< + Omit +>; + +export async function updateTenantOIDCAuthIntegration( + mongo: Db, + id: string, + oidcID: string, + input: UpdateTenantOIDCAuthIntegrationInput +) { + const result = await collection(mongo).findOneAndUpdate( + { id }, + { + // $set: dotize({ + // "auth.integrations.oidc.$[oidc]": input, + // }), + // FIXME: replace with the above one once the types are updated. + $set: dotize({ + "auth.integrations.oidc.$[]": input, + }), + }, + { + // Add an ArrayFilter to only update one of the OpenID Connect + // integrations. + // arrayFilters: [{ "oidc.id": oidcID }], // FIXME: add back when we got the mongo fixes in place + // False to return the updated document instead of the original + // document. + returnOriginal: false, + } + ); + + return result.value || null; +} + +/** + * deleteTenantOIDCAuthIntegration will delete the specific OpenID Connect Auth + * Integration on the Tenant. + * + * @param mongo MongoDB Database handle + * @param id the id of the Tenant + * @param oidcID the id of the OpenID Connect Auth Integration we're deleting + */ +export async function deleteTenantOIDCAuthIntegration( + mongo: Db, + id: string, + oidcID: string +) { + const result = await collection(mongo).findOneAndUpdate( + { id }, + { + $pull: { "auth.integrations.oidc": { id: oidcID } }, + }, + { + // False to return the updated document instead of the original + // document. + returnOriginal: false, + } + ); + + return result.value || null; +} diff --git a/src/core/server/services/tenant/index.ts b/src/core/server/services/tenant/index.ts index 2d0f634a9..da73c651d 100644 --- a/src/core/server/services/tenant/index.ts +++ b/src/core/server/services/tenant/index.ts @@ -2,13 +2,20 @@ import { Redis } from "ioredis"; import { Db } from "mongodb"; import { URL } from "url"; -import { GQLSettingsInput } from "talk-server/graph/tenant/schema/__generated__/types"; +import { + GQLCreateOIDCAuthIntegrationConfigurationInput, + GQLSettingsInput, + GQLUpdateOIDCAuthIntegrationConfigurationInput, +} from "talk-server/graph/tenant/schema/__generated__/types"; import { createTenant, CreateTenantInput, + createTenantOIDCAuthIntegration, + deleteTenantOIDCAuthIntegration, regenerateTenantSSOKey, Tenant, updateTenant, + updateTenantOIDCAuthIntegration, } from "talk-server/models/tenant"; import { discover } from "talk-server/app/middleware/passport/strategies/oidc/discover"; @@ -116,3 +123,81 @@ export async function discoverOIDCConfiguration(issuerString: string) { // Discover the configuration. return discover(issuer); } + +export type CreateOIDCAuthIntegration = GQLCreateOIDCAuthIntegrationConfigurationInput; + +export async function createOIDCAuthIntegration( + mongo: Db, + redis: Redis, + cache: TenantCache, + tenant: Tenant, + input: CreateOIDCAuthIntegration +) { + // Create the integration. By default, the integration is disabled. + const updatedTenant = await createTenantOIDCAuthIntegration( + mongo, + tenant.id, + { + enabled: false, + ...input, + } + ); + if (!updatedTenant) { + return null; + } + + // Update the tenant cache. + await cache.update(redis, updatedTenant); + + return updatedTenant; +} + +export type UpdateOIDCAuthIntegration = GQLUpdateOIDCAuthIntegrationConfigurationInput; + +export async function updateOIDCAuthIntegration( + mongo: Db, + redis: Redis, + cache: TenantCache, + tenant: Tenant, + oidcID: string, + input: UpdateOIDCAuthIntegration +) { + // Update the integration. By default, the integration is disabled. + const updatedTenant = await updateTenantOIDCAuthIntegration( + mongo, + tenant.id, + oidcID, + input + ); + if (!updatedTenant) { + return null; + } + + // Update the tenant cache. + await cache.update(redis, updatedTenant); + + return updatedTenant; +} + +export async function deleteOIDCAuthIntegration( + mongo: Db, + redis: Redis, + cache: TenantCache, + tenant: Tenant, + oidcID: string +) { + // Delete the integration. By default, the integration is disabled. + const updatedTenant = await deleteTenantOIDCAuthIntegration( + mongo, + tenant.id, + oidcID + ); + if (!updatedTenant) { + return null; + } + + // Update the tenant cache. + await cache.update(redis, updatedTenant); + + return updatedTenant; +} From 977161db0a485b46ee4e50c909aa41aa625523dc Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Thu, 25 Oct 2018 08:58:58 -0600 Subject: [PATCH 05/18] fix: linting --- .../comments/moderation/phases/wordList.ts | 55 +++++++++++++++++++ .../comments/moderation/wordList.spec.ts | 46 ++++++++++++++++ .../services/comments/moderation/wordList.ts | 25 +++++++++ 3 files changed, 126 insertions(+) create mode 100755 src/core/server/services/comments/moderation/phases/wordList.ts create mode 100644 src/core/server/services/comments/moderation/wordList.spec.ts create mode 100644 src/core/server/services/comments/moderation/wordList.ts diff --git a/src/core/server/services/comments/moderation/phases/wordList.ts b/src/core/server/services/comments/moderation/phases/wordList.ts new file mode 100755 index 000000000..71f8eb860 --- /dev/null +++ b/src/core/server/services/comments/moderation/phases/wordList.ts @@ -0,0 +1,55 @@ +import { + GQLCOMMENT_FLAG_REASON, + GQLCOMMENT_STATUS, +} from "talk-server/graph/tenant/schema/__generated__/types"; +import { ACTION_TYPE } from "talk-server/models/action"; +import { + IntermediateModerationPhase, + IntermediatePhaseResult, +} from "talk-server/services/comments/moderation"; +import { containsMatchingPhrase } from "talk-server/services/comments/moderation/wordlist"; + +// This phase checks the comment against the wordList. +export const wordList: IntermediateModerationPhase = ({ + tenant, + comment, +}): IntermediatePhaseResult | void => { + // If there isn't a body, there can't be a bad word! + if (!comment.body) { + return; + } + + // Decide the status based on whether or not the current asset/settings + // has pre-mod enabled or not. If the comment was rejected based on the + // wordList, then reject it, otherwise if the moderation setting is + // premod, set it to `premod`. + if (containsMatchingPhrase(tenant.wordList.banned, comment.body)) { + // Add the flag related to Trust to the comment. + return { + status: GQLCOMMENT_STATUS.REJECTED, + actions: [ + { + action_type: ACTION_TYPE.FLAG, + reason: GQLCOMMENT_FLAG_REASON.COMMENT_DETECTED_BANNED_WORD, + }, + ], + }; + } + + // If the comment has a suspect word or a link, we need to add a + // flag to it to indicate that it needs to be looked at. + // Otherwise just return the new comment. + + // If the wordlist has matched the suspect word filter and we haven't disabled + // auto-flagging suspect words, then we should flag the comment! + if (containsMatchingPhrase(tenant.wordList.suspect, comment.body)) { + return { + actions: [ + { + action_type: ACTION_TYPE.FLAG, + reason: GQLCOMMENT_FLAG_REASON.COMMENT_DETECTED_SUSPECT_WORD, + }, + ], + }; + } +}; diff --git a/src/core/server/services/comments/moderation/wordList.spec.ts b/src/core/server/services/comments/moderation/wordList.spec.ts new file mode 100644 index 000000000..d23705ecf --- /dev/null +++ b/src/core/server/services/comments/moderation/wordList.spec.ts @@ -0,0 +1,46 @@ +import { containsMatchingPhrase } from "talk-server/services/comments/moderation/wordlist"; + +const phrases = [ + "cookies", + "how to do bad things", + "how to do really bad things", + "s h i t", + "$hit", + "p**ch", + "p*ch", +]; + +describe("containsMatchingPhrase", () => { + it("does match on a word in the list", () => { + [ + "how to do really bad things", + "what is cookies", + "cookies", + "COOKIES.", + "how to do bad things", + "How To do bad things!", + "This stuff is $hit!", + "That's a p**ch!", + ].forEach(word => { + expect(containsMatchingPhrase(phrases, word)).toEqual(true); + }); + }); + + it("does not match on a word not in the list", () => { + [ + "how to", + "cookie", + "how to be a great person?", + "how to not do really bad things?", + "i have $100 dollars.", + "I have bad $ hit lling", + "That's a p***ch!", + ].forEach(word => { + expect(containsMatchingPhrase(phrases, word)).toEqual(false); + }); + }); + + it("allows an empty list", () => { + expect(containsMatchingPhrase([], "test")).toEqual(false); + }); +}); diff --git a/src/core/server/services/comments/moderation/wordList.ts b/src/core/server/services/comments/moderation/wordList.ts new file mode 100644 index 000000000..30cc124ac --- /dev/null +++ b/src/core/server/services/comments/moderation/wordList.ts @@ -0,0 +1,25 @@ +/** + * Escape string for special regular expression characters. + */ +export function escapeRegExp(str: string) { + return str.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); // $& means the whole matched string +} + +/** + * Generate a regular expression that catches the `phrases`. + */ +export function generateRegExp(phrases: string[]) { + const inner = phrases + .map(phrase => + phrase + .split(/\s+/) + .map(word => escapeRegExp(word)) + .join('[\\s"?!.]+') + ) + .join("|"); + + return new RegExp(`(^|[^\\w])(${inner})(?=[^\\w]|$)`, "iu"); +} + +export const containsMatchingPhrase = (phrases: string[], testString: string) => + phrases.length > 0 ? generateRegExp(phrases).test(testString) : false; From e7131e15672fb6b7e7002931d2d26274ced57ef1 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Thu, 25 Oct 2018 10:31:43 -0600 Subject: [PATCH 06/18] fix: adjusted to include the openID --- src/core/server/app/middleware/passport/strategies/jwt.ts | 7 +++++++ .../app/middleware/passport/strategies/oidc/index.ts | 5 ++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/core/server/app/middleware/passport/strategies/jwt.ts b/src/core/server/app/middleware/passport/strategies/jwt.ts index 273bce5d3..59e27f227 100644 --- a/src/core/server/app/middleware/passport/strategies/jwt.ts +++ b/src/core/server/app/middleware/passport/strategies/jwt.ts @@ -75,6 +75,13 @@ export class JWTStrategy extends Strategy { throw new Error("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); 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 4405e3697..b9ff305a7 100644 --- a/src/core/server/app/middleware/passport/strategies/oidc/index.ts +++ b/src/core/server/app/middleware/passport/strategies/oidc/index.ts @@ -343,7 +343,10 @@ export default class OIDCStrategy extends Strategy { const { clientID, clientSecret, authorizationURL, tokenURL } = integration; // Construct the callbackURL from the request. - const callbackURL = reconstructURL(req, "/api/tenant/auth/oidc/callback"); + const callbackURL = reconstructURL( + req, + `/api/tenant/auth/oidc/${integration.id}/callback` + ); // Create a new OAuth2Strategy, where we pass the verify callback bound to // this OIDCStrategy instance. From eadf7bde438525a350530d58147ee1005250114a Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Thu, 25 Oct 2018 10:55:22 -0600 Subject: [PATCH 07/18] feat: added memoization to the regexp generation --- .../comments/moderation/phases/wordList.ts | 8 +-- .../comments/moderation/phases/wordlist.ts | 55 ------------------- .../services/comments/moderation/wordList.ts | 14 +++++ .../services/comments/moderation/wordlist.ts | 25 --------- 4 files changed, 18 insertions(+), 84 deletions(-) delete mode 100755 src/core/server/services/comments/moderation/phases/wordlist.ts delete mode 100644 src/core/server/services/comments/moderation/wordlist.ts diff --git a/src/core/server/services/comments/moderation/phases/wordList.ts b/src/core/server/services/comments/moderation/phases/wordList.ts index 71f8eb860..a014bff94 100755 --- a/src/core/server/services/comments/moderation/phases/wordList.ts +++ b/src/core/server/services/comments/moderation/phases/wordList.ts @@ -7,7 +7,7 @@ import { IntermediateModerationPhase, IntermediatePhaseResult, } from "talk-server/services/comments/moderation"; -import { containsMatchingPhrase } from "talk-server/services/comments/moderation/wordlist"; +import { containsMatchingPhraseMemoized } from "talk-server/services/comments/moderation/wordlist"; // This phase checks the comment against the wordList. export const wordList: IntermediateModerationPhase = ({ @@ -23,7 +23,7 @@ export const wordList: IntermediateModerationPhase = ({ // has pre-mod enabled or not. If the comment was rejected based on the // wordList, then reject it, otherwise if the moderation setting is // premod, set it to `premod`. - if (containsMatchingPhrase(tenant.wordList.banned, comment.body)) { + if (containsMatchingPhraseMemoized(tenant.wordList.banned, comment.body)) { // Add the flag related to Trust to the comment. return { status: GQLCOMMENT_STATUS.REJECTED, @@ -40,9 +40,9 @@ export const wordList: IntermediateModerationPhase = ({ // flag to it to indicate that it needs to be looked at. // Otherwise just return the new comment. - // If the wordlist has matched the suspect word filter and we haven't disabled + // If the wordList has matched the suspect word filter and we haven't disabled // auto-flagging suspect words, then we should flag the comment! - if (containsMatchingPhrase(tenant.wordList.suspect, comment.body)) { + if (containsMatchingPhraseMemoized(tenant.wordList.suspect, comment.body)) { return { actions: [ { diff --git a/src/core/server/services/comments/moderation/phases/wordlist.ts b/src/core/server/services/comments/moderation/phases/wordlist.ts deleted file mode 100755 index 71f8eb860..000000000 --- a/src/core/server/services/comments/moderation/phases/wordlist.ts +++ /dev/null @@ -1,55 +0,0 @@ -import { - GQLCOMMENT_FLAG_REASON, - GQLCOMMENT_STATUS, -} from "talk-server/graph/tenant/schema/__generated__/types"; -import { ACTION_TYPE } from "talk-server/models/action"; -import { - IntermediateModerationPhase, - IntermediatePhaseResult, -} from "talk-server/services/comments/moderation"; -import { containsMatchingPhrase } from "talk-server/services/comments/moderation/wordlist"; - -// This phase checks the comment against the wordList. -export const wordList: IntermediateModerationPhase = ({ - tenant, - comment, -}): IntermediatePhaseResult | void => { - // If there isn't a body, there can't be a bad word! - if (!comment.body) { - return; - } - - // Decide the status based on whether or not the current asset/settings - // has pre-mod enabled or not. If the comment was rejected based on the - // wordList, then reject it, otherwise if the moderation setting is - // premod, set it to `premod`. - if (containsMatchingPhrase(tenant.wordList.banned, comment.body)) { - // Add the flag related to Trust to the comment. - return { - status: GQLCOMMENT_STATUS.REJECTED, - actions: [ - { - action_type: ACTION_TYPE.FLAG, - reason: GQLCOMMENT_FLAG_REASON.COMMENT_DETECTED_BANNED_WORD, - }, - ], - }; - } - - // If the comment has a suspect word or a link, we need to add a - // flag to it to indicate that it needs to be looked at. - // Otherwise just return the new comment. - - // If the wordlist has matched the suspect word filter and we haven't disabled - // auto-flagging suspect words, then we should flag the comment! - if (containsMatchingPhrase(tenant.wordList.suspect, comment.body)) { - return { - actions: [ - { - action_type: ACTION_TYPE.FLAG, - reason: GQLCOMMENT_FLAG_REASON.COMMENT_DETECTED_SUSPECT_WORD, - }, - ], - }; - } -}; diff --git a/src/core/server/services/comments/moderation/wordList.ts b/src/core/server/services/comments/moderation/wordList.ts index 30cc124ac..47f50d09e 100644 --- a/src/core/server/services/comments/moderation/wordList.ts +++ b/src/core/server/services/comments/moderation/wordList.ts @@ -1,3 +1,9 @@ +import { memoize } from "lodash"; + +// TODO: reintroduce this when we have https://github.com/DefinitelyTyped/DefinitelyTyped/pull/30035 merged +// // Replace `memoize.Cache`. +// memoize.Cache = WeakMap; + /** * Escape string for special regular expression characters. */ @@ -21,5 +27,13 @@ export function generateRegExp(phrases: string[]) { return new RegExp(`(^|[^\\w])(${inner})(?=[^\\w]|$)`, "iu"); } +export const generateRegExpMemoized = memoize(generateRegExp); + export const containsMatchingPhrase = (phrases: string[], testString: string) => phrases.length > 0 ? generateRegExp(phrases).test(testString) : false; + +export const containsMatchingPhraseMemoized = ( + phrases: string[], + testString: string +) => + phrases.length > 0 ? generateRegExpMemoized(phrases).test(testString) : false; diff --git a/src/core/server/services/comments/moderation/wordlist.ts b/src/core/server/services/comments/moderation/wordlist.ts deleted file mode 100644 index 30cc124ac..000000000 --- a/src/core/server/services/comments/moderation/wordlist.ts +++ /dev/null @@ -1,25 +0,0 @@ -/** - * Escape string for special regular expression characters. - */ -export function escapeRegExp(str: string) { - return str.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); // $& means the whole matched string -} - -/** - * Generate a regular expression that catches the `phrases`. - */ -export function generateRegExp(phrases: string[]) { - const inner = phrases - .map(phrase => - phrase - .split(/\s+/) - .map(word => escapeRegExp(word)) - .join('[\\s"?!.]+') - ) - .join("|"); - - return new RegExp(`(^|[^\\w])(${inner})(?=[^\\w]|$)`, "iu"); -} - -export const containsMatchingPhrase = (phrases: string[], testString: string) => - phrases.length > 0 ? generateRegExp(phrases).test(testString) : false; From ffd0f90965f51341d40b8f9a04a02c3462ba3eaa Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Thu, 25 Oct 2018 11:29:10 -0600 Subject: [PATCH 08/18] fix: documentation + moved discover into query --- .../passport/strategies/oidc/discover.ts | 38 ++- src/core/server/graph/tenant/loaders/auth.ts | 14 + src/core/server/graph/tenant/loaders/index.ts | 2 + .../server/graph/tenant/mutators/settings.ts | 6 - .../server/graph/tenant/resolvers/mutation.ts | 4 - .../server/graph/tenant/resolvers/query.ts | 2 + .../server/graph/tenant/schema/schema.graphql | 260 +++++++++++++----- src/core/server/models/tenant.ts | 4 +- 8 files changed, 231 insertions(+), 99 deletions(-) create mode 100644 src/core/server/graph/tenant/loaders/auth.ts diff --git a/src/core/server/app/middleware/passport/strategies/oidc/discover.ts b/src/core/server/app/middleware/passport/strategies/oidc/discover.ts index ab12fa59c..3fe4679a6 100644 --- a/src/core/server/app/middleware/passport/strategies/oidc/discover.ts +++ b/src/core/server/app/middleware/passport/strategies/oidc/discover.ts @@ -6,9 +6,9 @@ import { URL } from "url"; */ export interface DiscoveryConfiguration { issuer: string; - authorizationURL?: string; + authorizationURL: string; tokenURL?: string; - jwksURI?: string; + jwksURI: string; } /** @@ -18,9 +18,9 @@ export interface DiscoveryConfiguration { */ interface DiscoveryRawConfiguration { issuer: string; - authorization_endpoint?: string; + authorization_endpoint: string; token_endpoint?: string; - jwks_uri?: string; + jwks_uri: string; } /** @@ -29,7 +29,9 @@ interface DiscoveryRawConfiguration { * @param issuer the Issuer URL that should be used to determine the * configuration */ -export async function discover(issuer: URL): Promise { +export async function discover( + issuer: URL +): Promise { // Any provider MUST provide a .well-known url that is JSON parsable based // on the issuer: https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfig const configurationURL = @@ -38,13 +40,23 @@ export async function discover(issuer: URL): Promise { "/.well-known/openid-configuration"; const res = await fetch(configurationURL); - // Parse the configuration - const meta: DiscoveryRawConfiguration = await res.json(); + // Ensure that it responds correctly. + if (res.status !== 200) { + return null; + } - return { - issuer: meta.issuer, - authorizationURL: meta.authorization_endpoint, - tokenURL: meta.token_endpoint, - jwksURI: meta.jwks_uri, - }; + try { + // Parse the configuration + const meta: DiscoveryRawConfiguration = await res.json(); + + return { + issuer: meta.issuer, + authorizationURL: meta.authorization_endpoint, + tokenURL: meta.token_endpoint, + jwksURI: meta.jwks_uri, + }; + } catch (err) { + // TODO: log the error + return null; + } } diff --git a/src/core/server/graph/tenant/loaders/auth.ts b/src/core/server/graph/tenant/loaders/auth.ts new file mode 100644 index 000000000..9b13a9cda --- /dev/null +++ b/src/core/server/graph/tenant/loaders/auth.ts @@ -0,0 +1,14 @@ +import DataLoader from "dataloader"; + +import TenantContext from "talk-server/graph/tenant/context"; +import { GQLDiscoveredOIDCConfiguration } from "talk-server/graph/tenant/schema/__generated__/types"; +import { discoverOIDCConfiguration } from "talk-server/services/tenant"; + +export default (ctx: TenantContext) => ({ + discoverOIDCConfiguration: new DataLoader< + string, + GQLDiscoveredOIDCConfiguration | null + >(issuers => + Promise.all(issuers.map(issuer => discoverOIDCConfiguration(issuer))) + ), +}); diff --git a/src/core/server/graph/tenant/loaders/index.ts b/src/core/server/graph/tenant/loaders/index.ts index d4a00cc28..1233b54c7 100644 --- a/src/core/server/graph/tenant/loaders/index.ts +++ b/src/core/server/graph/tenant/loaders/index.ts @@ -1,9 +1,11 @@ import Context from "talk-server/graph/tenant/context"; import Assets from "./assets"; +import Auth from "./auth"; import Comments from "./comments"; import Users from "./users"; export default (ctx: Context) => ({ + Auth: Auth(ctx), Assets: Assets(ctx), Comments: Comments(ctx), Users: Users(ctx), diff --git a/src/core/server/graph/tenant/mutators/settings.ts b/src/core/server/graph/tenant/mutators/settings.ts index d1ceaaef3..065e709d3 100644 --- a/src/core/server/graph/tenant/mutators/settings.ts +++ b/src/core/server/graph/tenant/mutators/settings.ts @@ -4,8 +4,6 @@ import TenantContext from "talk-server/graph/tenant/context"; import { GQLCreateOIDCAuthIntegrationInput, GQLDeleteOIDCAuthIntegrationInput, - GQLDiscoverOIDCConfigurationInput, - GQLOIDCConfiguration, GQLSettingsInput, GQLUpdateOIDCAuthIntegrationInput, } from "talk-server/graph/tenant/schema/__generated__/types"; @@ -13,7 +11,6 @@ import { Tenant } from "talk-server/models/tenant"; import { createOIDCAuthIntegration, deleteOIDCAuthIntegration, - discoverOIDCConfiguration, regenerateSSOKey, update, updateOIDCAuthIntegration, @@ -24,9 +21,6 @@ export default ({ mongo, redis, tenantCache, tenant }: TenantContext) => ({ update(mongo, redis, tenantCache, tenant, omitBy(input, isNull)), regenerateSSOKey: (): Promise => regenerateSSOKey(mongo, redis, tenantCache, tenant), - discoverOIDCConfiguration: ( - input: GQLDiscoverOIDCConfigurationInput - ): Promise => discoverOIDCConfiguration(input.issuer), createOIDCAuthIntegration: ( input: GQLCreateOIDCAuthIntegrationInput ): Promise => diff --git a/src/core/server/graph/tenant/resolvers/mutation.ts b/src/core/server/graph/tenant/resolvers/mutation.ts index 7781d7689..cea0de73d 100644 --- a/src/core/server/graph/tenant/resolvers/mutation.ts +++ b/src/core/server/graph/tenant/resolvers/mutation.ts @@ -51,10 +51,6 @@ const Mutation: GQLMutationTypeResolver = { settings: await ctx.mutators.Settings.regenerateSSOKey(), clientMutationId: input.clientMutationId, }), - discoverOIDCConfiguration: async (source, { input }, ctx) => ({ - configuration: await ctx.mutators.Settings.discoverOIDCConfiguration(input), - clientMutationId: input.clientMutationId, - }), createOIDCAuthIntegration: async (source, { input }, ctx) => ({ settings: await ctx.mutators.Settings.createOIDCAuthIntegration(input), clientMutationId: input.clientMutationId, diff --git a/src/core/server/graph/tenant/resolvers/query.ts b/src/core/server/graph/tenant/resolvers/query.ts index 295b6c459..73ed1d2e5 100644 --- a/src/core/server/graph/tenant/resolvers/query.ts +++ b/src/core/server/graph/tenant/resolvers/query.ts @@ -6,6 +6,8 @@ const Query: GQLQueryTypeResolver = { id ? ctx.loaders.Comments.comment.load(id) : null, settings: (source, args, ctx) => ctx.tenant, me: (source, args, ctx) => ctx.user, + discoverOIDCConfiguration: (source, { issuer }, ctx) => + ctx.loaders.Auth.discoverOIDCConfiguration.load(issuer), }; export default Query; diff --git a/src/core/server/graph/tenant/schema/schema.graphql b/src/core/server/graph/tenant/schema/schema.graphql index b29921796..2f88ed512 100644 --- a/src/core/server/graph/tenant/schema/schema.graphql +++ b/src/core/server/graph/tenant/schema/schema.graphql @@ -312,6 +312,47 @@ type SSOAuthIntegration { ## OIDCAuthIntegration ########################## +""" +DiscoveredOIDCConfiguration contains the discovered Provider Metadata as defined +in: + +https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata + +Discovery is not supported on all providers, and is described in the OpenID +Connect Discovery 1.0 incorporating errata set 1: + +https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfig +""" +type DiscoveredOIDCConfiguration { + """ + issuer is defined as the `issuer` in: + + https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata + """ + issuer: String! + + """ + authorizationURL is defined as the `authorization_endpoint` in: + + https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata + """ + authorizationURL: String! + + """ + tokenURL is defined as the `token_endpoint` in: + + https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata + """ + tokenURL: String + + """ + jwksURI is defined as the `jwks_uri` in: + + https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata + """ + jwksURI: String! +} + """ OIDCAuthIntegration provides a way to store Open ID Connect credentials. This will be used in the admin to provide staff logins for users. @@ -335,7 +376,9 @@ type OIDCAuthIntegration { targetFilter: AuthenticationTargetFilter """ - name is the label assigned to reference the provider of the OIDC integration. + name is the label assigned to reference the provider of the OIDC integration, + and will be used in situations where the name of the provider needs to be + displayed, like the login button. """ name: String @@ -346,11 +389,46 @@ type OIDCAuthIntegration { """ callbackURL: String! + """ + clientID is the Client Identifier as defined in: + + https://tools.ietf.org/html/rfc6749#section-2.2 + """ clientID: String! @auth(roles: [ADMIN]) + + """ + clientSecret is the Client Secret as defined in: + + https://tools.ietf.org/html/rfc6749#section-2.3.1 + """ clientSecret: String! @auth(roles: [ADMIN]) + + """ + authorizationURL is defined as the `authorization_endpoint` in: + + https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata + """ authorizationURL: String! @auth(roles: [ADMIN]) + + """ + tokenURL is defined as the `token_endpoint` in: + + https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata + """ tokenURL: String! @auth(roles: [ADMIN]) + + """ + jwksURI is defined as the `jwks_uri` in: + + https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata + """ jwksURI: String! @auth(roles: [ADMIN]) + + """ + issuer is defined as the `issuer` in: + + https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata + """ issuer: String! @auth(roles: [ADMIN]) """ @@ -1200,7 +1278,8 @@ type Query { asset(id: ID, url: String): Asset """ - me is the current logged in User. + me is the current logged in User. If no user is currently logged in, it will + return null. """ me: User @@ -1208,6 +1287,18 @@ type Query { settings is the Settings for a given Tenant. """ settings: Settings! + + """ + discoverOIDCConfiguration will discover the OpenID Connect configuration based + on the provided issuer. Discovery is not supported on all providers, and is + described in the OpenID Connect Discovery 1.0 incorporating errata set 1: + + https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfig + + If the provider does not support discovery, the response will be null. + """ + discoverOIDCConfiguration(issuer: String!): DiscoveredOIDCConfiguration + @auth(roles: [ADMIN]) } ################################################################################ @@ -1365,34 +1456,6 @@ input SettingsSSOAuthIntegrationInput { displayNameEnable: Boolean } -# input SettingsOIDCAuthIntegrationInput { -# enabled: Boolean - -# """ -# targetFilter will restrict where the authentication integration should be -# displayed. If the value of targetFilter is null, then the authentication -# integration should be displayed in all targets. -# """ -# targetFilter: SettingsAuthenticationTargetFilterInput - -# """ -# name is the label assigned to reference the provider of the OIDC integration. -# """ -# name: String -# clientID: String -# clientSecret: String -# authorizationURL: String -# tokenURL: String -# jwksURI: String -# issuer: String - -# """ -# displayNameEnable when enabled, will allow Users to set and view their -# displayName's. -# """ -# displayNameEnable: Boolean -# } - input SettingsGoogleAuthIntegrationInput { enabled: Boolean @@ -1915,53 +1978,58 @@ type RegenerateSSOKeyPayload { clientMutationId: String! } -################## -## discoverOIDCConfiguration -################## - -input DiscoverOIDCConfigurationInput { - """ - - """ - issuer: String! - - """ - clientMutationId is required for Relay support. - """ - clientMutationId: String! -} - -type OIDCConfiguration { - issuer: String! - authorizationURL: String - tokenURL: String - jwksURI: String -} - -type DiscoverOIDCConfigurationPayload { - """ - configuration was the discovered configuration for the OpenID Connect server - given the provided issuer. - """ - configuration: OIDCConfiguration - - """ - clientMutationId is required for Relay support. - """ - clientMutationId: String! -} - ################## # createOIDCAuthIntegration ################## input CreateOIDCAuthIntegrationConfigurationInput { + """ + name is the label assigned to reference the provider of the OIDC integration, + and will be used in situations where the name of the provider needs to be + displayed, like the login button. + """ name: String! + + """ + clientID is the Client Identifier as defined in: + + https://tools.ietf.org/html/rfc6749#section-2.2 + """ clientID: String! + + """ + clientSecret is the Client Secret as defined in: + + https://tools.ietf.org/html/rfc6749#section-2.3.1 + """ clientSecret: String! + + """ + authorizationURL is defined as the `authorization_endpoint` in: + + https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata + """ authorizationURL: String! + + """ + tokenURL is defined as the `token_endpoint` in: + + https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata + """ tokenURL: String! + + """ + jwksURI is defined as the `jwks_uri` in: + + https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata + """ jwksURI: String! + + """ + issuer is defined as the `issuer` in: + + https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata + """ issuer: String! } @@ -1994,13 +2062,65 @@ type CreateOIDCAuthIntegrationPayload { ################## input UpdateOIDCAuthIntegrationConfigurationInput { + """ + enabled, when true, allows the integration to be enabled. + """ enabled: Boolean + + """ + targetFilter will restrict where the authentication integration should be + displayed. If the value of targetFilter is null, then the authentication + integration should be displayed in all targets. + """ + targetFilter: SettingsAuthenticationTargetFilterInput + + """ + name is the label assigned to reference the provider of the OIDC integration, + and will be used in situations where the name of the provider needs to be + displayed, like the login button. + """ name: String + + """ + clientID is the Client Identifier as defined in: + + https://tools.ietf.org/html/rfc6749#section-2.2 + """ clientID: String + + """ + clientSecret is the Client Secret as defined in: + + https://tools.ietf.org/html/rfc6749#section-2.3.1 + """ clientSecret: String + + """ + authorizationURL is defined as the `authorization_endpoint` in: + + https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata + """ authorizationURL: String + + """ + tokenURL is defined as the `token_endpoint` in: + + https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata + """ tokenURL: String + + """ + jwksURI is defined as the `jwks_uri` in: + + https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata + """ jwksURI: String + + """ + issuer is defined as the `issuer` in: + + https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata + """ issuer: String } @@ -2091,14 +2211,6 @@ type Mutation { regenerateSSOKey(input: RegenerateSSOKeyInput!): RegenerateSSOKeyPayload @auth(roles: [ADMIN]) - """ - discoverOIDCConfiguration will discover the OpenID Connect configuration based - on the provided input. - """ - discoverOIDCConfiguration( - input: DiscoverOIDCConfigurationInput! - ): DiscoverOIDCConfigurationPayload @auth(roles: [ADMIN]) - """ createOIDCAuthIntegration will create a OpenID Connect auth integration. """ diff --git a/src/core/server/models/tenant.ts b/src/core/server/models/tenant.ts index e17fa1e5f..e6a12f8df 100644 --- a/src/core/server/models/tenant.ts +++ b/src/core/server/models/tenant.ts @@ -293,7 +293,7 @@ export async function updateTenantOIDCAuthIntegration( // $set: dotize({ // "auth.integrations.oidc.$[oidc]": input, // }), - // FIXME: replace with the above one once the types are updated. + // FIXME: uncomment when https://github.com/DefinitelyTyped/DefinitelyTyped/pull/29986 gets merged $set: dotize({ "auth.integrations.oidc.$[]": input, }), @@ -301,7 +301,7 @@ export async function updateTenantOIDCAuthIntegration( { // Add an ArrayFilter to only update one of the OpenID Connect // integrations. - // arrayFilters: [{ "oidc.id": oidcID }], // FIXME: add back when we got the mongo fixes in place + // arrayFilters: [{ "oidc.id": oidcID }], // FIXME: uncomment when https://github.com/DefinitelyTyped/DefinitelyTyped/pull/29986 gets merged // False to return the updated document instead of the original // document. returnOriginal: false, From 6dab5836c3f82a342c59bfb44f7cc3afcce7fbb3 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Thu, 25 Oct 2018 11:51:28 -0600 Subject: [PATCH 09/18] review: adapted graph responses for OIDC mutations --- .../server/graph/tenant/mutators/settings.ts | 12 +-- .../server/graph/tenant/resolvers/mutation.ts | 48 +++++++--- .../server/graph/tenant/schema/schema.graphql | 18 +++- src/core/server/models/tenant.ts | 89 +++++++++++++++++-- src/core/server/services/tenant/index.ts | 34 ++++--- 5 files changed, 151 insertions(+), 50 deletions(-) diff --git a/src/core/server/graph/tenant/mutators/settings.ts b/src/core/server/graph/tenant/mutators/settings.ts index 065e709d3..16c3ac6c0 100644 --- a/src/core/server/graph/tenant/mutators/settings.ts +++ b/src/core/server/graph/tenant/mutators/settings.ts @@ -21,9 +21,7 @@ export default ({ mongo, redis, tenantCache, tenant }: TenantContext) => ({ update(mongo, redis, tenantCache, tenant, omitBy(input, isNull)), regenerateSSOKey: (): Promise => regenerateSSOKey(mongo, redis, tenantCache, tenant), - createOIDCAuthIntegration: ( - input: GQLCreateOIDCAuthIntegrationInput - ): Promise => + createOIDCAuthIntegration: (input: GQLCreateOIDCAuthIntegrationInput) => createOIDCAuthIntegration( mongo, redis, @@ -31,9 +29,7 @@ export default ({ mongo, redis, tenantCache, tenant }: TenantContext) => ({ tenant, input.configuration ), - updateOIDCAuthIntegration: ( - input: GQLUpdateOIDCAuthIntegrationInput - ): Promise => + updateOIDCAuthIntegration: (input: GQLUpdateOIDCAuthIntegrationInput) => updateOIDCAuthIntegration( mongo, redis, @@ -42,8 +38,6 @@ export default ({ mongo, redis, tenantCache, tenant }: TenantContext) => ({ input.id, input.configuration ), - deleteOIDCAuthIntegration: ( - input: GQLDeleteOIDCAuthIntegrationInput - ): Promise => + deleteOIDCAuthIntegration: (input: GQLDeleteOIDCAuthIntegrationInput) => deleteOIDCAuthIntegration(mongo, redis, tenantCache, tenant, input.id), }); diff --git a/src/core/server/graph/tenant/resolvers/mutation.ts b/src/core/server/graph/tenant/resolvers/mutation.ts index cea0de73d..32425a67d 100644 --- a/src/core/server/graph/tenant/resolvers/mutation.ts +++ b/src/core/server/graph/tenant/resolvers/mutation.ts @@ -51,18 +51,42 @@ const Mutation: GQLMutationTypeResolver = { settings: await ctx.mutators.Settings.regenerateSSOKey(), clientMutationId: input.clientMutationId, }), - createOIDCAuthIntegration: async (source, { input }, ctx) => ({ - settings: await ctx.mutators.Settings.createOIDCAuthIntegration(input), - clientMutationId: input.clientMutationId, - }), - updateOIDCAuthIntegration: async (source, { input }, ctx) => ({ - settings: await ctx.mutators.Settings.updateOIDCAuthIntegration(input), - clientMutationId: input.clientMutationId, - }), - deleteOIDCAuthIntegration: async (source, { input }, ctx) => ({ - settings: await ctx.mutators.Settings.deleteOIDCAuthIntegration(input), - clientMutationId: input.clientMutationId, - }), + createOIDCAuthIntegration: async (source, { input }, ctx) => { + const result = await ctx.mutators.Settings.createOIDCAuthIntegration(input); + if (!result) { + return { clientMutationId: input.clientMutationId }; + } + + return { + integration: result.integration, + settings: result.tenant, + clientMutationId: input.clientMutationId, + }; + }, + updateOIDCAuthIntegration: async (source, { input }, ctx) => { + const result = await ctx.mutators.Settings.updateOIDCAuthIntegration(input); + if (!result) { + return { clientMutationId: input.clientMutationId }; + } + + return { + integration: result.integration, + settings: result.tenant, + clientMutationId: input.clientMutationId, + }; + }, + deleteOIDCAuthIntegration: async (source, { input }, ctx) => { + const result = await ctx.mutators.Settings.deleteOIDCAuthIntegration(input); + if (!result) { + return { clientMutationId: input.clientMutationId }; + } + + return { + integration: result.integration, + settings: result.tenant, + clientMutationId: input.clientMutationId, + }; + }, }; export default Mutation; diff --git a/src/core/server/graph/tenant/schema/schema.graphql b/src/core/server/graph/tenant/schema/schema.graphql index 2f88ed512..197530ce5 100644 --- a/src/core/server/graph/tenant/schema/schema.graphql +++ b/src/core/server/graph/tenant/schema/schema.graphql @@ -2046,6 +2046,11 @@ input CreateOIDCAuthIntegrationInput { } type CreateOIDCAuthIntegrationPayload { + """ + integration is the OIDCAuthIntegration we just created. + """ + integration: OIDCAuthIntegration + """ settings is the Settings that the OIDCAuthIntegration was created on. """ @@ -2143,6 +2148,11 @@ input UpdateOIDCAuthIntegrationInput { } type UpdateOIDCAuthIntegrationPayload { + """ + integration is the OIDCAuthIntegration we just updated. + """ + integration: OIDCAuthIntegration + """ settings is the Settings that the OIDCAuthIntegration was updated on. """ @@ -2172,7 +2182,13 @@ input DeleteOIDCAuthIntegrationInput { type DeleteOIDCAuthIntegrationPayload { """ - settings is the Settings that the OIDCAuthIntegration was deleted on. + integration is the OIDCAuthIntegration we just deleted. + """ + integration: OIDCAuthIntegration + + """ + settings is the Settings that the OIDCAuthIntegration was deleted on with the + OIDCAuthIntegration removed from it. """ settings: Settings diff --git a/src/core/server/models/tenant.ts b/src/core/server/models/tenant.ts index e6a12f8df..559cf7178 100644 --- a/src/core/server/models/tenant.ts +++ b/src/core/server/models/tenant.ts @@ -252,11 +252,17 @@ export type CreateTenantOIDCAuthIntegrationInput = Omit< "id" | "callbackURL" >; +export interface CreateTenantOIDCAuthIntegrationResultObject { + tenant?: Tenant; + integration?: Omit; + wasCreated: boolean; +} + export async function createTenantOIDCAuthIntegration( mongo: Db, id: string, input: CreateTenantOIDCAuthIntegrationInput -) { +): Promise { // Add the ID to the integration. const integration = { id: uuid.v4(), @@ -273,20 +279,40 @@ export async function createTenantOIDCAuthIntegration( // document. { returnOriginal: false } ); + if (!result.value) { + return { + wasCreated: false, + }; + } - return result.value || null; + const wasCreated = + result.value.auth.integrations.oidc.findIndex( + ({ id: integrationID }) => integrationID === integration.id + ) !== -1; + + return { + tenant: result.value, + integration, + wasCreated, + }; } export type UpdateTenantOIDCAuthIntegrationInput = Partial< Omit >; +export interface UpdateTenantOIDCAuthIntegrationResultObject { + tenant?: Tenant; + integration?: Omit; + wasUpdated: boolean; +} + export async function updateTenantOIDCAuthIntegration( mongo: Db, id: string, oidcID: string, input: UpdateTenantOIDCAuthIntegrationInput -) { +): Promise { const result = await collection(mongo).findOneAndUpdate( { id }, { @@ -307,8 +333,29 @@ export async function updateTenantOIDCAuthIntegration( returnOriginal: false, } ); + if (!result.value) { + return { + wasUpdated: false, + }; + } - return result.value || null; + const integration = result.value.auth.integrations.oidc.find( + ({ id: integrationID }) => integrationID === oidcID + ); + + const wasUpdated = Boolean(integration); + + return { + tenant: result.value, + integration, + wasUpdated, + }; +} + +export interface DeleteTenantOIDCAuthIntegrationResultObject { + tenant?: Tenant; + integration?: Omit; + wasDeleted: boolean; } /** @@ -323,18 +370,42 @@ export async function deleteTenantOIDCAuthIntegration( mongo: Db, id: string, oidcID: string -) { +): Promise { const result = await collection(mongo).findOneAndUpdate( { id }, { $pull: { "auth.integrations.oidc": { id: oidcID } }, }, { - // False to return the updated document instead of the original - // document. - returnOriginal: false, + // True to return the document before we modified it. This gives us the + // opportunity to return the original document and asertain if the + // integration was/could be removed. + returnOriginal: true, } ); + if (!result.value) { + return { wasDeleted: false }; + } - return result.value || null; + // Find the integration that we wanted to delete. + const integration = result.value.auth.integrations.oidc.find( + ({ id: integrationID }) => integrationID === oidcID + ); + if (!integration) { + // The integration was not in the original document, so we could not have + // possibly deleted it! + return { wasDeleted: false }; + } + + // The integration was found, we should pull that integration out of the + // resulting Tenant. + result.value.auth.integrations.oidc.filter( + ({ id: integrationID }) => integrationID !== integration.id + ); + + return { + tenant: result.value, + integration, + wasDeleted: true, + }; } diff --git a/src/core/server/services/tenant/index.ts b/src/core/server/services/tenant/index.ts index da73c651d..c484f3c83 100644 --- a/src/core/server/services/tenant/index.ts +++ b/src/core/server/services/tenant/index.ts @@ -134,22 +134,18 @@ export async function createOIDCAuthIntegration( input: CreateOIDCAuthIntegration ) { // Create the integration. By default, the integration is disabled. - const updatedTenant = await createTenantOIDCAuthIntegration( - mongo, - tenant.id, - { - enabled: false, - ...input, - } - ); - if (!updatedTenant) { + const result = await createTenantOIDCAuthIntegration(mongo, tenant.id, { + enabled: false, + ...input, + }); + if (!result.wasCreated || !result.tenant) { return null; } // Update the tenant cache. - await cache.update(redis, updatedTenant); + await cache.update(redis, result.tenant); - return updatedTenant; + return result; } export type UpdateOIDCAuthIntegration = GQLUpdateOIDCAuthIntegrationConfigurationInput; @@ -163,20 +159,20 @@ export async function updateOIDCAuthIntegration( input: UpdateOIDCAuthIntegration ) { // Update the integration. By default, the integration is disabled. - const updatedTenant = await updateTenantOIDCAuthIntegration( + const result = await updateTenantOIDCAuthIntegration( mongo, tenant.id, oidcID, input ); - if (!updatedTenant) { + if (!result.wasUpdated || !result.tenant) { return null; } // Update the tenant cache. - await cache.update(redis, updatedTenant); + await cache.update(redis, result.tenant); - return updatedTenant; + return result; } export async function deleteOIDCAuthIntegration( @@ -187,17 +183,17 @@ export async function deleteOIDCAuthIntegration( oidcID: string ) { // Delete the integration. By default, the integration is disabled. - const updatedTenant = await deleteTenantOIDCAuthIntegration( + const result = await deleteTenantOIDCAuthIntegration( mongo, tenant.id, oidcID ); - if (!updatedTenant) { + if (!result.wasDeleted || !result.tenant) { return null; } // Update the tenant cache. - await cache.update(redis, updatedTenant); + await cache.update(redis, result.tenant); - return updatedTenant; + return result; } From d1e6d297f38fc899ae121bf15d8a3b349a521020 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Thu, 25 Oct 2018 12:03:28 -0600 Subject: [PATCH 10/18] fix: fixed linting --- .../comments/moderation/phases/wordList.ts | 2 +- .../comments/moderation/wordList.spec.ts | 2 +- .../comments/moderation/wordlist.spec.ts | 46 ------------------- 3 files changed, 2 insertions(+), 48 deletions(-) delete mode 100644 src/core/server/services/comments/moderation/wordlist.spec.ts diff --git a/src/core/server/services/comments/moderation/phases/wordList.ts b/src/core/server/services/comments/moderation/phases/wordList.ts index a014bff94..8a9d2eab0 100755 --- a/src/core/server/services/comments/moderation/phases/wordList.ts +++ b/src/core/server/services/comments/moderation/phases/wordList.ts @@ -7,7 +7,7 @@ import { IntermediateModerationPhase, IntermediatePhaseResult, } from "talk-server/services/comments/moderation"; -import { containsMatchingPhraseMemoized } from "talk-server/services/comments/moderation/wordlist"; +import { containsMatchingPhraseMemoized } from "talk-server/services/comments/moderation/wordList"; // This phase checks the comment against the wordList. export const wordList: IntermediateModerationPhase = ({ diff --git a/src/core/server/services/comments/moderation/wordList.spec.ts b/src/core/server/services/comments/moderation/wordList.spec.ts index d23705ecf..5e68398b1 100644 --- a/src/core/server/services/comments/moderation/wordList.spec.ts +++ b/src/core/server/services/comments/moderation/wordList.spec.ts @@ -1,4 +1,4 @@ -import { containsMatchingPhrase } from "talk-server/services/comments/moderation/wordlist"; +import { containsMatchingPhrase } from "talk-server/services/comments/moderation/wordList"; const phrases = [ "cookies", diff --git a/src/core/server/services/comments/moderation/wordlist.spec.ts b/src/core/server/services/comments/moderation/wordlist.spec.ts deleted file mode 100644 index d23705ecf..000000000 --- a/src/core/server/services/comments/moderation/wordlist.spec.ts +++ /dev/null @@ -1,46 +0,0 @@ -import { containsMatchingPhrase } from "talk-server/services/comments/moderation/wordlist"; - -const phrases = [ - "cookies", - "how to do bad things", - "how to do really bad things", - "s h i t", - "$hit", - "p**ch", - "p*ch", -]; - -describe("containsMatchingPhrase", () => { - it("does match on a word in the list", () => { - [ - "how to do really bad things", - "what is cookies", - "cookies", - "COOKIES.", - "how to do bad things", - "How To do bad things!", - "This stuff is $hit!", - "That's a p**ch!", - ].forEach(word => { - expect(containsMatchingPhrase(phrases, word)).toEqual(true); - }); - }); - - it("does not match on a word not in the list", () => { - [ - "how to", - "cookie", - "how to be a great person?", - "how to not do really bad things?", - "i have $100 dollars.", - "I have bad $ hit lling", - "That's a p***ch!", - ].forEach(word => { - expect(containsMatchingPhrase(phrases, word)).toEqual(false); - }); - }); - - it("allows an empty list", () => { - expect(containsMatchingPhrase([], "test")).toEqual(false); - }); -}); From 3c613754f041d37e71ba5bb0945745075f6001ed Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Thu, 25 Oct 2018 12:57:20 -0600 Subject: [PATCH 11/18] feat: added allowRegistration option --- .../app/handlers/api/tenant/auth/local.ts | 5 + .../passport/strategies/oidc/index.ts | 23 ++--- .../passport/strategies/oidc/oidc.spec.ts | 13 +-- .../passport/strategies/verifiers/sso.spec.ts | 9 +- .../passport/strategies/verifiers/sso.ts | 24 +++-- .../server/graph/tenant/schema/schema.graphql | 99 +++++++++++++++---- src/core/server/models/tenant.ts | 4 + src/core/server/services/tenant/index.ts | 1 + 8 files changed, 121 insertions(+), 57 deletions(-) diff --git a/src/core/server/app/handlers/api/tenant/auth/local.ts b/src/core/server/app/handlers/api/tenant/auth/local.ts index 181148b87..94f8fc7f9 100644 --- a/src/core/server/app/handlers/api/tenant/auth/local.ts +++ b/src/core/server/app/handlers/api/tenant/auth/local.ts @@ -51,6 +51,11 @@ export const signupHandler = (options: SignupOptions): RequestHandler => async ( return next(new Error("integration is disabled")); } + if (!tenant.auth.integrations.local.allowRegistration) { + // TODO: replace with better error. + return next(new Error("registration is disabled")); + } + // Get the fields from the body. Validate will throw an error if the body // does not conform to the specification. const { username, password, email }: SignupBody = validate( 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 b9ff305a7..1d792f5f1 100644 --- a/src/core/server/app/middleware/passport/strategies/oidc/index.ts +++ b/src/core/server/app/middleware/passport/strategies/oidc/index.ts @@ -136,13 +136,10 @@ export const OIDCIDTokenSchema = Joi.object() email: Joi.string(), email_verified: Joi.boolean().default(false), picture: Joi.string().default(undefined), + name: Joi.string().default(undefined), + nickname: Joi.string().default(undefined), }) - .optionalKeys(["picture", "email_verified"]); - -export const OIDCDisplayNameIDTokenSchema = OIDCIDTokenSchema.keys({ - name: Joi.string().default(undefined), - nickname: Joi.string().default(undefined), -}).optionalKeys(["name", "nickname"]); + .optionalKeys(["picture", "email_verified", "name", "nickname"]); export async function findOrCreateOIDCUser( db: Db, @@ -160,12 +157,7 @@ export async function findOrCreateOIDCUser( picture, name, nickname, - }: OIDCIDToken = validate( - integration.displayNameEnable - ? OIDCDisplayNameIDTokenSchema - : OIDCIDTokenSchema, - token - ); + }: OIDCIDToken = validate(OIDCIDTokenSchema, token); // Construct the profile that will be used to query for the user. const profile: OIDCProfile = { @@ -178,10 +170,13 @@ export async function findOrCreateOIDCUser( // Try to lookup user given their id provided in the `sub` claim. let user = await retrieveUserWithProfile(db, tenant.id, profile); if (!user) { + if (!integration.allowRegistration) { + // Registration is disabled, so we can't create the user user here. + return; + } + // FIXME: implement rules. - // Default the displayName. When it is disabled, Joi will strip the - // displayName fields from the token, so it will fallback to undefined. const displayName = nickname || name || undefined; // Create the new user, as one didn't exist before! diff --git a/src/core/server/app/middleware/passport/strategies/oidc/oidc.spec.ts b/src/core/server/app/middleware/passport/strategies/oidc/oidc.spec.ts index 141b145ba..b8afbffad 100644 --- a/src/core/server/app/middleware/passport/strategies/oidc/oidc.spec.ts +++ b/src/core/server/app/middleware/passport/strategies/oidc/oidc.spec.ts @@ -1,7 +1,4 @@ -import { - OIDCDisplayNameIDTokenSchema, - OIDCIDTokenSchema, -} from "talk-server/app/middleware/passport/strategies/oidc"; +import { OIDCIDTokenSchema } from "talk-server/app/middleware/passport/strategies/oidc"; import { validate } from "talk-server/app/request/body"; describe("OIDCIDTokenSchema", () => { @@ -42,9 +39,7 @@ describe("OIDCIDTokenSchema", () => { expect(validate(OIDCIDTokenSchema, token)).toEqual(token); }); -}); -describe("OIDCDisplayNameIDTokenSchema", () => { it("allows a valid payload", () => { const token = { sub: "sub", @@ -56,7 +51,7 @@ describe("OIDCDisplayNameIDTokenSchema", () => { nickname: "nickname", }; - expect(validate(OIDCDisplayNameIDTokenSchema, token)).toEqual(token); + expect(validate(OIDCIDTokenSchema, token)).toEqual(token); }); it("allows an empty name", () => { @@ -69,7 +64,7 @@ describe("OIDCDisplayNameIDTokenSchema", () => { nickname: "nickname", }; - expect(validate(OIDCDisplayNameIDTokenSchema, token)).toEqual(token); + expect(validate(OIDCIDTokenSchema, token)).toEqual(token); }); it("allows an empty nickname", () => { @@ -82,6 +77,6 @@ describe("OIDCDisplayNameIDTokenSchema", () => { name: "name", }; - expect(validate(OIDCDisplayNameIDTokenSchema, token)).toEqual(token); + expect(validate(OIDCIDTokenSchema, token)).toEqual(token); }); }); diff --git a/src/core/server/app/middleware/passport/strategies/verifiers/sso.spec.ts b/src/core/server/app/middleware/passport/strategies/verifiers/sso.spec.ts index 7f9816541..049fe45e9 100644 --- a/src/core/server/app/middleware/passport/strategies/verifiers/sso.spec.ts +++ b/src/core/server/app/middleware/passport/strategies/verifiers/sso.spec.ts @@ -1,6 +1,5 @@ import { isSSOToken, - SSODisplayNameUserProfileSchema, SSOUserProfileSchema, } from "talk-server/app/middleware/passport/strategies/verifiers/sso"; import { validate } from "talk-server/app/request/body"; @@ -44,9 +43,7 @@ describe("SSOUserProfileSchema", () => { expect(validate(SSOUserProfileSchema, profile)).toEqual(profile); }); -}); -describe("SSODisplayNameUserProfileSchema", () => { it("allows a valid payload", () => { const profile = { id: "id", @@ -56,7 +53,7 @@ describe("SSODisplayNameUserProfileSchema", () => { displayName: "displayName", }; - expect(validate(SSODisplayNameUserProfileSchema, profile)).toEqual(profile); + expect(validate(SSOUserProfileSchema, profile)).toEqual(profile); }); it("allows an empty avatar", () => { @@ -67,7 +64,7 @@ describe("SSODisplayNameUserProfileSchema", () => { displayName: "displayName", }; - expect(validate(SSODisplayNameUserProfileSchema, profile)).toEqual(profile); + expect(validate(SSOUserProfileSchema, profile)).toEqual(profile); }); it("allows an empty displayName", () => { @@ -78,6 +75,6 @@ describe("SSODisplayNameUserProfileSchema", () => { avatar: "avatar", }; - expect(validate(SSODisplayNameUserProfileSchema, profile)).toEqual(profile); + expect(validate(SSOUserProfileSchema, profile)).toEqual(profile); }); }); 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 1ff9ff44f..60c606b53 100644 --- a/src/core/server/app/middleware/passport/strategies/verifiers/sso.ts +++ b/src/core/server/app/middleware/passport/strategies/verifiers/sso.ts @@ -3,7 +3,10 @@ import jwt from "jsonwebtoken"; import { Db } from "mongodb"; import { validate } from "talk-server/app/request/body"; -import { GQLUSER_ROLE } from "talk-server/graph/tenant/schema/__generated__/types"; +import { + GQLSSOAuthIntegration, + GQLUSER_ROLE, +} 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"; @@ -30,16 +33,14 @@ export const SSOUserProfileSchema = Joi.object() email: Joi.string(), username: Joi.string(), avatar: Joi.string().default(undefined), + displayName: Joi.string().default(undefined), }) - .optionalKeys(["avatar"]); - -export const SSODisplayNameUserProfileSchema = SSOUserProfileSchema.keys({ - displayName: Joi.string().default(undefined), -}).optionalKeys(["displayName"]); + .optionalKeys(["avatar", "displayName"]); export async function findOrCreateSSOUser( db: Db, tenant: Tenant, + integration: GQLSSOAuthIntegration, token: SSOToken ) { if (!token.user) { @@ -49,9 +50,7 @@ export async function findOrCreateSSOUser( // Unpack/validate the token content. const { id, email, username, displayName, avatar }: SSOUserProfile = validate( - tenant.auth.integrations.sso!.displayNameEnable - ? SSODisplayNameUserProfileSchema - : SSOUserProfileSchema, + SSOUserProfileSchema, token.user ); @@ -63,6 +62,11 @@ export async function findOrCreateSSOUser( // Try to lookup user given their id provided in the `sub` claim. let user = await retrieveUserWithProfile(db, tenant.id, profile); if (!user) { + if (!integration.allowRegistration) { + // Registration is disabled, so we can't create the user user here. + return; + } + // 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! @@ -138,6 +142,6 @@ export class SSOVerifier { algorithms: ["HS256"], // TODO: (wyattjoh) investigate replacing algorithm. }); - return findOrCreateSSOUser(this.mongo, tenant, token); + return findOrCreateSSOUser(this.mongo, tenant, integration, token); } } diff --git a/src/core/server/graph/tenant/schema/schema.graphql b/src/core/server/graph/tenant/schema/schema.graphql index 197530ce5..e845393bd 100644 --- a/src/core/server/graph/tenant/schema/schema.graphql +++ b/src/core/server/graph/tenant/schema/schema.graphql @@ -264,6 +264,12 @@ type AuthenticationTargetFilter { type LocalAuthIntegration { enabled: Boolean! + """ + allowRegistration when true will allow users that have not signed up + before with this authentication integration to sign up. + """ + allowRegistration: Boolean! + """ targetFilter will restrict where the authentication integration should be displayed. If the value of targetFilter is null, then the authentication @@ -284,6 +290,12 @@ embed to allow single sign on. type SSOAuthIntegration { enabled: Boolean! + """ + allowRegistration when true will allow users that have not signed up + before with this authentication integration to sign up. + """ + allowRegistration: Boolean! + """ targetFilter will restrict where the authentication integration should be displayed. If the value of targetFilter is null, then the authentication @@ -300,12 +312,6 @@ type SSOAuthIntegration { keyGeneratedAt is the Time that the key was effective from. """ keyGeneratedAt: Time @auth(roles: [ADMIN]) - - """ - displayNameEnable when enabled, will allow Users to set and view their - displayName's. - """ - displayNameEnable: Boolean @auth(roles: [ADMIN]) } ########################## @@ -368,6 +374,12 @@ type OIDCAuthIntegration { """ enabled: Boolean! + """ + allowRegistration when true will allow users that have not signed up + before with this authentication integration to sign up. + """ + allowRegistration: Boolean! + """ targetFilter will restrict where the authentication integration should be displayed. If the value of targetFilter is null, then the authentication @@ -430,12 +442,6 @@ type OIDCAuthIntegration { https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata """ issuer: String! @auth(roles: [ADMIN]) - - """ - displayNameEnable when enabled, will allow Users to set and view their - displayName's. - """ - displayNameEnable: Boolean @auth(roles: [ADMIN]) } ########################## @@ -445,6 +451,12 @@ type OIDCAuthIntegration { type GoogleAuthIntegration { enabled: Boolean! + """ + allowRegistration when true will allow users that have not signed up + before with this authentication integration to sign up. + """ + allowRegistration: Boolean! + """ targetFilter will restrict where the authentication integration should be displayed. If the value of targetFilter is null, then the authentication @@ -463,6 +475,12 @@ type GoogleAuthIntegration { type FacebookAuthIntegration { enabled: Boolean! + """ + allowRegistration when true will allow users that have not signed up + before with this authentication integration to sign up. + """ + allowRegistration: Boolean! + """ targetFilter will restrict where the authentication integration should be displayed. If the value of targetFilter is null, then the authentication @@ -474,6 +492,10 @@ type FacebookAuthIntegration { clientSecret: String @auth(roles: [ADMIN]) } +########################## +## Auth +########################## + type AuthIntegrations { local: LocalAuthIntegration! sso: SSOAuthIntegration! @@ -482,6 +504,17 @@ type AuthIntegrations { facebook: FacebookAuthIntegration! } +""" +AuthDisplayNameConfiguration allows configuration related to Display Names. +""" +type AuthDisplayNameConfiguration { + """ + enabled when true will allow the display name to be used by other + AuthIntegrations. + """ + enabled: Boolean! +} + """ Auth contains all the settings related to authentication and authorization. @@ -492,6 +525,12 @@ type Auth { authentication solutions. """ integrations: AuthIntegrations! + + """ + displayName contains configuration related to the use of Display Names across + AuthIntegrations. + """ + displayName: AuthDisplayNameConfiguration @auth(roles: [ADMIN]) } ################################################################################ @@ -1431,6 +1470,12 @@ input SettingsAuthenticationTargetFilterInput { input SettingsLocalAuthIntegrationInput { enabled: Boolean + """ + allowRegistration when true will allow users that have not signed up + before with this authentication integration to sign up. + """ + allowRegistration: Boolean + """ targetFilter will restrict where the authentication integration should be displayed. If the value of targetFilter is null, then the authentication @@ -1442,23 +1487,29 @@ input SettingsLocalAuthIntegrationInput { input SettingsSSOAuthIntegrationInput { enabled: Boolean + """ + allowRegistration when true will allow users that have not signed up + before with this authentication integration to sign up. + """ + allowRegistration: Boolean + """ targetFilter will restrict where the authentication integration should be displayed. If the value of targetFilter is null, then the authentication integration should be displayed in all targets. """ targetFilter: SettingsAuthenticationTargetFilterInput - - """ - displayNameEnable when enabled, will allow Users to set and view their - displayName's. - """ - displayNameEnable: Boolean } input SettingsGoogleAuthIntegrationInput { enabled: Boolean + """ + allowRegistration when true will allow users that have not signed up + before with this authentication integration to sign up. + """ + allowRegistration: Boolean + """ targetFilter will restrict where the authentication integration should be displayed. If the value of targetFilter is null, then the authentication @@ -1473,6 +1524,12 @@ input SettingsGoogleAuthIntegrationInput { input SettingsFacebookAuthIntegrationInput { enabled: Boolean + """ + allowRegistration when true will allow users that have not signed up + before with this authentication integration to sign up. + """ + allowRegistration: Boolean + """ targetFilter will restrict where the authentication integration should be displayed. If the value of targetFilter is null, then the authentication @@ -2072,6 +2129,12 @@ input UpdateOIDCAuthIntegrationConfigurationInput { """ enabled: Boolean + """ + allowRegistration when true will allow users that have not signed up + before with this authentication integration to sign up. + """ + allowRegistration: Boolean + """ targetFilter will restrict where the authentication integration should be displayed. If the value of targetFilter is null, then the authentication diff --git a/src/core/server/models/tenant.ts b/src/core/server/models/tenant.ts index 559cf7178..ad2cc9652 100644 --- a/src/core/server/models/tenant.ts +++ b/src/core/server/models/tenant.ts @@ -86,16 +86,20 @@ export async function createTenant(mongo: Db, input: CreateTenantInput) { integrations: { local: { enabled: true, + allowRegistration: true, }, sso: { enabled: false, + allowRegistration: false, }, oidc: [], google: { enabled: false, + allowRegistration: false, }, facebook: { enabled: false, + allowRegistration: false, }, }, }, diff --git a/src/core/server/services/tenant/index.ts b/src/core/server/services/tenant/index.ts index c484f3c83..0fa525616 100644 --- a/src/core/server/services/tenant/index.ts +++ b/src/core/server/services/tenant/index.ts @@ -136,6 +136,7 @@ export async function createOIDCAuthIntegration( // Create the integration. By default, the integration is disabled. const result = await createTenantOIDCAuthIntegration(mongo, tenant.id, { enabled: false, + allowRegistration: false, ...input, }); if (!result.wasCreated || !result.tenant) { From 33ecbaecdaecd86a6d08bae6c31a6573b324967a Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Fri, 26 Oct 2018 12:17:24 -0600 Subject: [PATCH 12/18] fix: linting --- .../server/app/middleware/passport/strategies/verifiers/sso.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 60c606b53..e48fdd1b7 100644 --- a/src/core/server/app/middleware/passport/strategies/verifiers/sso.ts +++ b/src/core/server/app/middleware/passport/strategies/verifiers/sso.ts @@ -64,7 +64,7 @@ export async function findOrCreateSSOUser( if (!user) { if (!integration.allowRegistration) { // Registration is disabled, so we can't create the user user here. - return; + return null; } // FIXME: (wyattjoh) implement rules! Not all users should be able to create an account via this method. From 2571f048add478b51231baf9fa62fe6bf527c68e Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Mon, 29 Oct 2018 14:31:35 -0600 Subject: [PATCH 13/18] feat: support development url's --- src/core/server/app/middleware/context/tenant.ts | 4 ++++ src/core/server/app/router/api/tenant.ts | 6 +----- src/core/server/app/url.ts | 7 ++++++- src/core/server/graph/common/context.ts | 6 +++++- src/core/server/graph/management/context.ts | 6 ++++-- src/core/server/graph/management/middleware.ts | 2 +- src/core/server/graph/tenant/context.ts | 5 ++++- .../server/graph/tenant/resolvers/oidc_auth_integration.ts | 2 +- 8 files changed, 26 insertions(+), 12 deletions(-) diff --git a/src/core/server/app/middleware/context/tenant.ts b/src/core/server/app/middleware/context/tenant.ts index c52772b6b..549e70266 100644 --- a/src/core/server/app/middleware/context/tenant.ts +++ b/src/core/server/app/middleware/context/tenant.ts @@ -2,6 +2,7 @@ import { RequestHandler } from "express-jwt"; import { Redis } from "ioredis"; import { Db } from "mongodb"; +import { Config } from "talk-common/config"; import TenantContext from "talk-server/graph/tenant/context"; import { TaskQueue } from "talk-server/services/queue"; import { Request } from "talk-server/types/express"; @@ -10,12 +11,14 @@ export interface TenantContextMiddlewareOptions { mongo: Db; redis: Redis; queue: TaskQueue; + config: Config; } export const tenantContext = ({ mongo, redis, queue, + config, }: TenantContextMiddlewareOptions): RequestHandler => ( req: Request, res, @@ -38,6 +41,7 @@ export const tenantContext = ({ req.talk.context = { tenant: new TenantContext({ req, + config, mongo, redis, tenant, diff --git a/src/core/server/app/router/api/tenant.ts b/src/core/server/app/router/api/tenant.ts index 23567d770..2f7e73d4f 100644 --- a/src/core/server/app/router/api/tenant.ts +++ b/src/core/server/app/router/api/tenant.ts @@ -42,11 +42,7 @@ export async function createTenantRouter( // Any users may submit their GraphQL requests with authentication, this // middleware will unpack their user into the request. options.passport.authenticate("jwt", { session: false }), - tenantContext({ - mongo: app.mongo, - redis: app.redis, - queue: app.queue, - }), + tenantContext(app), await tenantGraphMiddleware({ schema: app.schemas.tenant, config: app.config, diff --git a/src/core/server/app/url.ts b/src/core/server/app/url.ts index a37def2c5..d9b7da56d 100644 --- a/src/core/server/app/url.ts +++ b/src/core/server/app/url.ts @@ -1,3 +1,4 @@ +import { Config } from "talk-common/config"; import { Tenant } from "talk-server/models/tenant"; import { Request } from "talk-server/types/express"; import { URL } from "url"; @@ -16,10 +17,14 @@ export function reconstructURL(req: Request, path: string = "/"): string { * reconstructTenantURL will reconstruct a URL based off of the Tenant's domain. */ export function reconstructTenantURL( + config: Config, tenant: Pick, path: string = "/" ): string { - const url = new URL(path, `https://${tenant.domain}`); + let url: URL = new URL(path, `https://${tenant.domain}`); + if (config.get("env") === "development") { + url = new URL(path, `http://${tenant.domain}:${config.get("port")}`); + } return url.href; } diff --git a/src/core/server/graph/common/context.ts b/src/core/server/graph/common/context.ts index 7e191e3f8..016aa37d6 100644 --- a/src/core/server/graph/common/context.ts +++ b/src/core/server/graph/common/context.ts @@ -1,17 +1,21 @@ +import { Config } from "talk-common/config"; import { User } from "talk-server/models/user"; import { Request } from "talk-server/types/express"; export interface CommonContextOptions { user?: User; req?: Request; + config: Config; } export default class CommonContext { public user?: User; public req?: Request; + public config: Config; - constructor({ user, req }: CommonContextOptions) { + constructor({ user, req, config }: CommonContextOptions) { this.user = user; this.req = req; + this.config = config; } } diff --git a/src/core/server/graph/management/context.ts b/src/core/server/graph/management/context.ts index cdb25dbe3..41e59e094 100644 --- a/src/core/server/graph/management/context.ts +++ b/src/core/server/graph/management/context.ts @@ -1,18 +1,20 @@ import { Db } from "mongodb"; +import { Config } from "talk-common/config"; import CommonContext from "talk-server/graph/common/context"; import { Request } from "talk-server/types/express"; export interface ManagementContextOptions { mongo: Db; + config: Config; req?: Request; } export default class ManagementContext extends CommonContext { public mongo: Db; - constructor({ req, mongo }: ManagementContextOptions) { - super({ req }); + constructor({ req, mongo, config }: ManagementContextOptions) { + super({ req, config }); this.mongo = mongo; } diff --git a/src/core/server/graph/management/middleware.ts b/src/core/server/graph/management/middleware.ts index 6dd15e5da..5e96a6a36 100644 --- a/src/core/server/graph/management/middleware.ts +++ b/src/core/server/graph/management/middleware.ts @@ -10,5 +10,5 @@ import ManagementContext from "./context"; export default (schema: GraphQLSchema, config: Config, mongo: Db) => graphqlMiddleware(config, async (req: Request) => ({ schema, - context: new ManagementContext({ req, mongo }), + context: new ManagementContext({ req, mongo, config }), })); diff --git a/src/core/server/graph/tenant/context.ts b/src/core/server/graph/tenant/context.ts index b6facbb30..3456e8029 100644 --- a/src/core/server/graph/tenant/context.ts +++ b/src/core/server/graph/tenant/context.ts @@ -8,6 +8,7 @@ import { TaskQueue } from "talk-server/services/queue"; import TenantCache from "talk-server/services/tenant/cache"; import { Request } from "talk-server/types/express"; +import { Config } from "talk-common/config"; import loaders from "./loaders"; import mutators from "./mutators"; @@ -17,6 +18,7 @@ export interface TenantContextOptions { tenant: Tenant; tenantCache: TenantCache; queue: TaskQueue; + config: Config; req?: Request; user?: User; } @@ -37,10 +39,11 @@ export default class TenantContext extends CommonContext { tenant, mongo, redis, + config, tenantCache, queue, }: TenantContextOptions) { - super({ user, req }); + super({ user, req, config }); this.tenant = tenant; this.tenantCache = tenantCache; diff --git a/src/core/server/graph/tenant/resolvers/oidc_auth_integration.ts b/src/core/server/graph/tenant/resolvers/oidc_auth_integration.ts index 9e2890222..44e28b6ec 100644 --- a/src/core/server/graph/tenant/resolvers/oidc_auth_integration.ts +++ b/src/core/server/graph/tenant/resolvers/oidc_auth_integration.ts @@ -19,7 +19,7 @@ const OIDCAuthIntegration: GQLOIDCAuthIntegrationTypeResolver< // Note that when constructing the callback url with the tenant, the port // information is lost. - return reconstructTenantURL(ctx.tenant, path); + return reconstructTenantURL(ctx.config, ctx.tenant, path); }, }; From c47032f257a2f9f9b011cbc9b703ad073d52276e Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Mon, 29 Oct 2018 14:31:47 -0600 Subject: [PATCH 14/18] feat: support facebook auth --- package-lock.json | 18 ++ package.json | 2 + .../server/app/middleware/passport/index.ts | 4 + .../passport/strategies/facebook.ts | 203 ++++++++++++++++++ src/core/server/app/router/api/auth.ts | 10 + src/core/server/models/user.ts | 7 +- 6 files changed, 243 insertions(+), 1 deletion(-) create mode 100644 src/core/server/app/middleware/passport/strategies/facebook.ts diff --git a/package-lock.json b/package-lock.json index ea0df219f..d3cdd2206 100644 --- a/package-lock.json +++ b/package-lock.json @@ -2080,6 +2080,16 @@ "@types/express": "*" } }, + "@types/passport-facebook": { + "version": "2.1.8", + "resolved": "http://registry.npmjs.org/@types/passport-facebook/-/passport-facebook-2.1.8.tgz", + "integrity": "sha512-5FGF6zNN0ZELetEdIDjVjfHSJfXSehNWeRLv9/8JD6Des4Z9A7sthhyXVRQUXeUxv0SmQ/i+IHZjR8R/G61wIg==", + "dev": true, + "requires": { + "@types/express": "*", + "@types/passport": "*" + } + }, "@types/passport-local": { "version": "1.0.33", "resolved": "https://registry.npmjs.org/@types/passport-local/-/passport-local-1.0.33.tgz", @@ -18709,6 +18719,14 @@ "pause": "0.0.1" } }, + "passport-facebook": { + "version": "2.1.1", + "resolved": "https://registry.npmjs.org/passport-facebook/-/passport-facebook-2.1.1.tgz", + "integrity": "sha1-w50LUq5NWRYyRaTiGnubYyEwMxE=", + "requires": { + "passport-oauth2": "1.x.x" + } + }, "passport-local": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/passport-local/-/passport-local-1.0.0.tgz", diff --git a/package.json b/package.json index d03269e3d..73a7d187d 100644 --- a/package.json +++ b/package.json @@ -90,6 +90,7 @@ "nodemailer": "^4.6.7", "nunjucks": "^3.1.3", "passport": "^0.4.0", + "passport-facebook": "^2.1.1", "passport-local": "^1.0.0", "passport-oauth2": "^1.4.0", "passport-strategy": "^1.0.0", @@ -149,6 +150,7 @@ "@types/nodemailer": "^4.6.2", "@types/nunjucks": "^3.0.0", "@types/passport": "^0.4.6", + "@types/passport-facebook": "^2.1.8", "@types/passport-local": "^1.0.33", "@types/passport-oauth2": "^1.4.5", "@types/passport-strategy": "^0.2.33", diff --git a/src/core/server/app/middleware/passport/index.ts b/src/core/server/app/middleware/passport/index.ts index 207fb1e92..5763eebc9 100644 --- a/src/core/server/app/middleware/passport/index.ts +++ b/src/core/server/app/middleware/passport/index.ts @@ -6,6 +6,7 @@ import { Db } from "mongodb"; import passport, { Authenticator } from "passport"; import { Config } from "talk-common/config"; +import FacebookStrategy from "talk-server/app/middleware/passport/strategies/facebook"; import { JWTStrategy } from "talk-server/app/middleware/passport/strategies/jwt"; import { createLocalStrategy } from "talk-server/app/middleware/passport/strategies/local"; import OIDCStrategy from "talk-server/app/middleware/passport/strategies/oidc"; @@ -50,6 +51,9 @@ export function createPassport( // Use the SSOStrategy. auth.use(new JWTStrategy(options)); + // Use the FacebookStrategy. + auth.use(new FacebookStrategy(options)); + return auth; } diff --git a/src/core/server/app/middleware/passport/strategies/facebook.ts b/src/core/server/app/middleware/passport/strategies/facebook.ts new file mode 100644 index 000000000..7fd628635 --- /dev/null +++ b/src/core/server/app/middleware/passport/strategies/facebook.ts @@ -0,0 +1,203 @@ +import { Db } from "mongodb"; +import { + Profile, + Strategy as FacebookPassportStrategy, +} from "passport-facebook"; +import { VerifyCallback } from "passport-oauth2"; +import { Strategy } from "passport-strategy"; + +import { Config } from "talk-common/config"; +import { reconstructTenantURL } from "talk-server/app/url"; +import { + GQLFacebookAuthIntegration, + GQLUSER_ROLE, +} from "talk-server/graph/tenant/schema/__generated__/types"; +import { Tenant } from "talk-server/models/tenant"; +import { + FacebookProfile, + retrieveUserWithProfile, +} 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 { Request } from "talk-server/types/express"; + +async function findOrCreateFacebookUser( + mongo: Db, + tenant: Tenant, + integration: GQLFacebookAuthIntegration, + { id, photos, emails, displayName }: Profile +) { + // Create the user profile that will be used to lookup the User. + const profile: FacebookProfile = { + type: "facebook", + id, + }; + + 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; + } + + // FIXME: implement rules. + + // Try to get the avatar. + let avatar: string | undefined; + if (photos && photos.length > 0) { + avatar = photos[0].value; + } + + // Try to get the email address. + let email: string | undefined; + let emailVerified: boolean | undefined; + if (emails && emails.length > 0) { + email = emails[0].value; + emailVerified = false; + } + + user = await upsert(mongo, tenant, { + username: null, + displayName, + role: GQLUSER_ROLE.COMMENTER, + email, + email_verified: emailVerified, + avatar, + profiles: [profile], + }); + } + + return user; +} + +function getEnabledIntegration( + tenant: Tenant +): Required { + const integration = tenant.auth.integrations.facebook; + + // Handle when the integration is enabled/disabled. + if (!integration.enabled) { + // TODO: return a better error. + throw new Error("integration not enabled"); + } + if (!integration.enabled) { + // TODO: return a better error. + throw new Error("integration not enabled"); + } + + if (!integration.clientID) { + throw new Error("clientID is missing in configuration"); + } + + if (!integration.clientSecret) { + throw new Error("clientSecret is missing in configuration"); + } + + // TODO: (wyattjoh) for some reason, type guards above to not allow coercion to this required type. + return integration as Required; +} + +export interface FacebookStrategyOptions { + config: Config; + mongo: Db; + tenantCache: TenantCache; +} + +export default class FacebookStrategy extends Strategy { + public name = "facebook"; + + private config: Config; + private mongo: Db; + private cache: TenantCacheAdapter; + + constructor({ config, mongo, tenantCache }: FacebookStrategyOptions) { + super(); + + this.config = config; + this.mongo = mongo; + this.cache = new TenantCacheAdapter(tenantCache); + } + + private userAuthenticatedCallback = async ( + req: Request, + accessToken: string, + refreshToken: string, + profile: Profile, + done: VerifyCallback + ) => { + const { tenant } = req.talk!; + if (!tenant) { + // TODO: return a better error. + throw new Error("tenant not found"); + } + + // Get the integration from the tenant. If needed, it will be used to create + // a new strategy. + let integration: Required; + try { + integration = getEnabledIntegration(tenant); + } catch (err) { + // TODO: wrap error? + return done(err); + } + + try { + const user = await findOrCreateFacebookUser( + this.mongo, + tenant, + integration, + profile + ); + + return done(null, user); + } catch (err) { + return done(err); + } + }; + + public authenticate(req: Request) { + try { + const { tenant } = req.talk!; + if (!tenant) { + // TODO: return a better error. + throw new Error("tenant not found"); + } + + // Get the integration (it will throw if the integration is not enabled). + const integration = getEnabledIntegration(tenant); + + let strategy = this.cache.get(tenant.id); + if (!strategy) { + strategy = new FacebookPassportStrategy( + { + clientID: integration.clientID, + clientSecret: integration.clientSecret, + callbackURL: reconstructTenantURL( + this.config, + tenant, + "/api/tenant/auth/facebook/callback" + ), + profileFields: ["id", "displayName", "photos", "email"], + enableProof: true, + passReqToCallback: true, + }, + this.userAuthenticatedCallback + ); + + this.cache.set(tenant.id, strategy); + } + + // Augment the strategy with the request method bindings. + strategy.error = this.error.bind(this); + strategy.fail = this.fail.bind(this); + strategy.pass = this.pass.bind(this); + strategy.redirect = this.redirect.bind(this); + strategy.success = this.success.bind(this); + + strategy.authenticate(req, { session: false }); + } catch (err) { + return this.error(err); + } + } +} diff --git a/src/core/server/app/router/api/auth.ts b/src/core/server/app/router/api/auth.ts index 37a1ddfff..0bc04a590 100644 --- a/src/core/server/app/router/api/auth.ts +++ b/src/core/server/app/router/api/auth.ts @@ -29,6 +29,16 @@ export function createNewAuthRouter(app: AppOptions, options: RouterOptions) { signupHandler({ db: app.mongo, signingConfig: app.signingConfig }) ); + router.get( + "/facebook", + wrapAuthn(options.passport, app.signingConfig, "facebook") + ); + + router.get( + "/facebook/callback", + wrapAuthn(options.passport, app.signingConfig, "facebook") + ); + router.get( "/oidc/:oidcID", wrapAuthn(options.passport, app.signingConfig, "oidc") diff --git a/src/core/server/models/user.ts b/src/core/server/models/user.ts index a89d3b42a..e29a54134 100644 --- a/src/core/server/models/user.ts +++ b/src/core/server/models/user.ts @@ -32,7 +32,12 @@ export interface SSOProfile { id: string; } -export type Profile = LocalProfile | OIDCProfile | SSOProfile; +export interface FacebookProfile { + type: "facebook"; + id: string; +} + +export type Profile = LocalProfile | OIDCProfile | SSOProfile | FacebookProfile; export interface Token { readonly id: string; From 58e51b299dd0ee339c0fc331c66fccd5bb343e28 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Mon, 29 Oct 2018 15:13:10 -0600 Subject: [PATCH 15/18] feat: created oauth2 abstract class --- .../passport/strategies/facebook.ts | 246 ++++++------------ .../middleware/passport/strategies/oauth2.ts | 122 +++++++++ 2 files changed, 195 insertions(+), 173 deletions(-) create mode 100644 src/core/server/app/middleware/passport/strategies/oauth2.ts diff --git a/src/core/server/app/middleware/passport/strategies/facebook.ts b/src/core/server/app/middleware/passport/strategies/facebook.ts index 7fd628635..3ae0a7f55 100644 --- a/src/core/server/app/middleware/passport/strategies/facebook.ts +++ b/src/core/server/app/middleware/passport/strategies/facebook.ts @@ -1,14 +1,11 @@ import { Db } from "mongodb"; -import { - Profile, - Strategy as FacebookPassportStrategy, -} from "passport-facebook"; -import { VerifyCallback } from "passport-oauth2"; -import { Strategy } from "passport-strategy"; +import { Profile, Strategy } from "passport-facebook"; import { Config } from "talk-common/config"; +import OAuth2Strategy from "talk-server/app/middleware/passport/strategies/oauth2"; import { reconstructTenantURL } from "talk-server/app/url"; import { + GQLAuthIntegrations, GQLFacebookAuthIntegration, GQLUSER_ROLE, } from "talk-server/graph/tenant/schema/__generated__/types"; @@ -18,85 +15,7 @@ import { retrieveUserWithProfile, } 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 { Request } from "talk-server/types/express"; - -async function findOrCreateFacebookUser( - mongo: Db, - tenant: Tenant, - integration: GQLFacebookAuthIntegration, - { id, photos, emails, displayName }: Profile -) { - // Create the user profile that will be used to lookup the User. - const profile: FacebookProfile = { - type: "facebook", - id, - }; - - 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; - } - - // FIXME: implement rules. - - // Try to get the avatar. - let avatar: string | undefined; - if (photos && photos.length > 0) { - avatar = photos[0].value; - } - - // Try to get the email address. - let email: string | undefined; - let emailVerified: boolean | undefined; - if (emails && emails.length > 0) { - email = emails[0].value; - emailVerified = false; - } - - user = await upsert(mongo, tenant, { - username: null, - displayName, - role: GQLUSER_ROLE.COMMENTER, - email, - email_verified: emailVerified, - avatar, - profiles: [profile], - }); - } - - return user; -} - -function getEnabledIntegration( - tenant: Tenant -): Required { - const integration = tenant.auth.integrations.facebook; - - // Handle when the integration is enabled/disabled. - if (!integration.enabled) { - // TODO: return a better error. - throw new Error("integration not enabled"); - } - if (!integration.enabled) { - // TODO: return a better error. - throw new Error("integration not enabled"); - } - - if (!integration.clientID) { - throw new Error("clientID is missing in configuration"); - } - - if (!integration.clientSecret) { - throw new Error("clientSecret is missing in configuration"); - } - - // TODO: (wyattjoh) for some reason, type guards above to not allow coercion to this required type. - return integration as Required; -} export interface FacebookStrategyOptions { config: Config; @@ -104,100 +23,81 @@ export interface FacebookStrategyOptions { tenantCache: TenantCache; } -export default class FacebookStrategy extends Strategy { +export default class FacebookStrategy extends OAuth2Strategy< + GQLFacebookAuthIntegration, + Strategy +> { public name = "facebook"; - private config: Config; - private mongo: Db; - private cache: TenantCacheAdapter; + protected getIntegration = (integrations: GQLAuthIntegrations) => + integrations.facebook; - constructor({ config, mongo, tenantCache }: FacebookStrategyOptions) { - super(); + protected async findOrCreateUser( + tenant: Tenant, + integration: Required, + { id, photos, emails, displayName }: Profile + ) { + // Create the user profile that will be used to lookup the User. + const profile: FacebookProfile = { + type: "facebook", + id, + }; - this.config = config; - this.mongo = mongo; - this.cache = new TenantCacheAdapter(tenantCache); + let user = await retrieveUserWithProfile(this.mongo, tenant.id, profile); + if (!user) { + if (!integration.allowRegistration) { + // Registration is disabled, so we can't create the user user here. + return; + } + + // FIXME: implement rules. + + // Try to get the avatar. + let avatar: string | undefined; + if (photos && photos.length > 0) { + avatar = photos[0].value; + } + + // Try to get the email address. + let email: string | undefined; + let emailVerified: boolean | undefined; + if (emails && emails.length > 0) { + email = emails[0].value; + emailVerified = false; + } + + user = await upsert(this.mongo, tenant, { + username: null, + displayName, + role: GQLUSER_ROLE.COMMENTER, + email, + email_verified: emailVerified, + avatar, + profiles: [profile], + }); + } + + return user; } - private userAuthenticatedCallback = async ( - req: Request, - accessToken: string, - refreshToken: string, - profile: Profile, - done: VerifyCallback - ) => { - const { tenant } = req.talk!; - if (!tenant) { - // TODO: return a better error. - throw new Error("tenant not found"); - } - - // Get the integration from the tenant. If needed, it will be used to create - // a new strategy. - let integration: Required; - try { - integration = getEnabledIntegration(tenant); - } catch (err) { - // TODO: wrap error? - return done(err); - } - - try { - const user = await findOrCreateFacebookUser( - this.mongo, - tenant, - integration, - profile - ); - - return done(null, user); - } catch (err) { - return done(err); - } - }; - - public authenticate(req: Request) { - try { - const { tenant } = req.talk!; - if (!tenant) { - // TODO: return a better error. - throw new Error("tenant not found"); - } - - // Get the integration (it will throw if the integration is not enabled). - const integration = getEnabledIntegration(tenant); - - let strategy = this.cache.get(tenant.id); - if (!strategy) { - strategy = new FacebookPassportStrategy( - { - clientID: integration.clientID, - clientSecret: integration.clientSecret, - callbackURL: reconstructTenantURL( - this.config, - tenant, - "/api/tenant/auth/facebook/callback" - ), - profileFields: ["id", "displayName", "photos", "email"], - enableProof: true, - passReqToCallback: true, - }, - this.userAuthenticatedCallback - ); - - this.cache.set(tenant.id, strategy); - } - - // Augment the strategy with the request method bindings. - strategy.error = this.error.bind(this); - strategy.fail = this.fail.bind(this); - strategy.pass = this.pass.bind(this); - strategy.redirect = this.redirect.bind(this); - strategy.success = this.success.bind(this); - - strategy.authenticate(req, { session: false }); - } catch (err) { - return this.error(err); - } + protected createStrategy( + tenant: Tenant, + integration: Required + ) { + return new Strategy( + { + clientID: integration.clientID, + clientSecret: integration.clientSecret, + callbackURL: reconstructTenantURL( + this.config, + tenant, + "/api/tenant/auth/facebook/callback" + ), + profileFields: ["id", "displayName", "photos", "email"], + enableProof: true, + passReqToCallback: true, + }, + this.verifyCallback + ); } } diff --git a/src/core/server/app/middleware/passport/strategies/oauth2.ts b/src/core/server/app/middleware/passport/strategies/oauth2.ts new file mode 100644 index 000000000..e7f74c685 --- /dev/null +++ b/src/core/server/app/middleware/passport/strategies/oauth2.ts @@ -0,0 +1,122 @@ +import { Db } from "mongodb"; +import { Strategy } from "passport-strategy"; + +import { Profile } from "passport"; +import { VerifyCallback } from "passport-oauth2"; +import { Config } from "talk-common/config"; +import { GQLAuthIntegrations } from "talk-server/graph/tenant/schema/__generated__/types"; +import { Tenant } from "talk-server/models/tenant"; +import { User } from "talk-server/models/user"; +import TenantCache from "talk-server/services/tenant/cache"; +import { TenantCacheAdapter } from "talk-server/services/tenant/cache/adapter"; +import { Request } from "talk-server/types/express"; + +interface OAuth2Integration { + enabled: boolean; + clientID?: string; + clientSecret?: string; +} + +export interface OAuth2StrategyOptions { + config: Config; + mongo: Db; + tenantCache: TenantCache; +} + +export default abstract class OAuth2Strategy< + T extends OAuth2Integration, + U extends Strategy +> extends Strategy { + protected config: Config; + protected mongo: Db; + protected cache: TenantCacheAdapter; + + constructor({ config, mongo, tenantCache }: OAuth2StrategyOptions) { + super(); + + this.config = config; + this.mongo = mongo; + this.cache = new TenantCacheAdapter(tenantCache); + } + + protected abstract getIntegration(integrations: GQLAuthIntegrations): T; + + protected abstract createStrategy( + tenant: Tenant, + integration: Required + ): U; + + protected abstract findOrCreateUser( + tenant: Tenant, + integration: Required, + profile: Profile + ): Promise; + + protected verifyCallback = async ( + req: Request, + accessToken: string, + refreshToken: string, + profile: Profile, + done: VerifyCallback + ) => { + try { + // Talk is defined at this point. + const tenant = req.talk!.tenant!; + + // Get the integration. + const integration = this.getIntegration(tenant.auth.integrations); + + // Get the user. + const user = await this.findOrCreateUser( + tenant, + integration as Required, + profile + ); + + return done(null, user); + } catch (err) { + return done(err); + } + }; + + public authenticate(req: Request) { + try { + // Talk is defined at this point. + const tenant = req.talk!.tenant!; + + // Get the integration. + const integration = this.getIntegration(tenant.auth.integrations); + + // Check to see if the integration is enabled. + if (!integration.enabled) { + // TODO: return a better error. + throw new Error("integration not enabled"); + } + + if (!integration.clientID) { + throw new Error("clientID is missing in configuration"); + } + + if (!integration.clientSecret) { + throw new Error("clientSecret is missing in configuration"); + } + + let strategy = this.cache.get(tenant.id); + if (!strategy) { + strategy = this.createStrategy(tenant, integration as Required); + this.cache.set(tenant.id, strategy); + } + + // Augment the strategy with the request method bindings. + strategy.error = this.error.bind(this); + strategy.fail = this.fail.bind(this); + strategy.pass = this.pass.bind(this); + strategy.redirect = this.redirect.bind(this); + strategy.success = this.success.bind(this); + + strategy.authenticate(req, { session: false }); + } catch (err) { + return this.error(err); + } + } +} From 93b91a05739d04ea46fd0354149dccaf8a412410 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Mon, 29 Oct 2018 17:36:15 -0600 Subject: [PATCH 16/18] feat: added google support --- package-lock.json | 8 ++ package.json | 1 + .../server/app/middleware/passport/index.ts | 4 + .../passport/strategies/facebook.ts | 2 + .../middleware/passport/strategies/google.ts | 116 ++++++++++++++++++ .../middleware/passport/strategies/oauth2.ts | 10 +- src/core/server/app/router/api/auth.ts | 38 +++--- src/core/server/models/user.ts | 12 +- src/types/passport-google-oauth2.d.ts | 38 ++++++ 9 files changed, 207 insertions(+), 22 deletions(-) create mode 100644 src/core/server/app/middleware/passport/strategies/google.ts create mode 100644 src/types/passport-google-oauth2.d.ts diff --git a/package-lock.json b/package-lock.json index d3cdd2206..8b5dbf36b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -18727,6 +18727,14 @@ "passport-oauth2": "1.x.x" } }, + "passport-google-oauth2": { + "version": "0.1.6", + "resolved": "https://registry.npmjs.org/passport-google-oauth2/-/passport-google-oauth2-0.1.6.tgz", + "integrity": "sha1-39cBasdEn+J8/rJSrpdK/CMleg0=", + "requires": { + "passport-oauth2": "^1.1.2" + } + }, "passport-local": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/passport-local/-/passport-local-1.0.0.tgz", diff --git a/package.json b/package.json index 73a7d187d..127a62e0d 100644 --- a/package.json +++ b/package.json @@ -91,6 +91,7 @@ "nunjucks": "^3.1.3", "passport": "^0.4.0", "passport-facebook": "^2.1.1", + "passport-google-oauth2": "^0.1.6", "passport-local": "^1.0.0", "passport-oauth2": "^1.4.0", "passport-strategy": "^1.0.0", diff --git a/src/core/server/app/middleware/passport/index.ts b/src/core/server/app/middleware/passport/index.ts index 5763eebc9..85cf5c8fa 100644 --- a/src/core/server/app/middleware/passport/index.ts +++ b/src/core/server/app/middleware/passport/index.ts @@ -7,6 +7,7 @@ import passport, { Authenticator } from "passport"; import { Config } from "talk-common/config"; import FacebookStrategy from "talk-server/app/middleware/passport/strategies/facebook"; +import GoogleStrategy from "talk-server/app/middleware/passport/strategies/google"; import { JWTStrategy } from "talk-server/app/middleware/passport/strategies/jwt"; import { createLocalStrategy } from "talk-server/app/middleware/passport/strategies/local"; import OIDCStrategy from "talk-server/app/middleware/passport/strategies/oidc"; @@ -54,6 +55,9 @@ export function createPassport( // Use the FacebookStrategy. auth.use(new FacebookStrategy(options)); + // Use the GoogleStrategy. + auth.use(new GoogleStrategy(options)); + return auth; } diff --git a/src/core/server/app/middleware/passport/strategies/facebook.ts b/src/core/server/app/middleware/passport/strategies/facebook.ts index 3ae0a7f55..c0f202737 100644 --- a/src/core/server/app/middleware/passport/strategies/facebook.ts +++ b/src/core/server/app/middleware/passport/strategies/facebook.ts @@ -77,6 +77,8 @@ export default class FacebookStrategy extends OAuth2Strategy< }); } + // TODO: maybe update user details? + return user; } diff --git a/src/core/server/app/middleware/passport/strategies/google.ts b/src/core/server/app/middleware/passport/strategies/google.ts new file mode 100644 index 000000000..9e793e722 --- /dev/null +++ b/src/core/server/app/middleware/passport/strategies/google.ts @@ -0,0 +1,116 @@ +import { Db } from "mongodb"; +import { Profile, Strategy } from "passport-google-oauth2"; + +import { Config } from "talk-common/config"; +import OAuth2Strategy from "talk-server/app/middleware/passport/strategies/oauth2"; +import { reconstructTenantURL } from "talk-server/app/url"; +import { + GQLAuthIntegrations, + GQLGoogleAuthIntegration, + GQLUSER_ROLE, +} from "talk-server/graph/tenant/schema/__generated__/types"; +import { Tenant } from "talk-server/models/tenant"; +import { + GoogleProfile, + retrieveUserWithProfile, +} from "talk-server/models/user"; +import TenantCache from "talk-server/services/tenant/cache"; +import { upsert } from "talk-server/services/users"; + +export interface GoogleStrategyOptions { + config: Config; + mongo: Db; + tenantCache: TenantCache; +} + +export interface GoogleStrategyOptions { + config: Config; + mongo: Db; + tenantCache: TenantCache; +} + +export default class GoogleStrategy extends OAuth2Strategy< + GQLGoogleAuthIntegration, + Strategy +> { + public name = "google"; + + constructor(options: GoogleStrategyOptions) { + super({ + ...options, + scope: ["profile"], + }); + } + + protected getIntegration = (integrations: GQLAuthIntegrations) => + integrations.google; + + protected async findOrCreateUser( + tenant: Tenant, + integration: Required, + { id, photos, emails, displayName }: Profile + ) { + // Create the user profile that will be used to lookup the User. + const profile: GoogleProfile = { + type: "google", + id, + }; + + let user = await retrieveUserWithProfile(this.mongo, tenant.id, profile); + if (!user) { + if (!integration.allowRegistration) { + // Registration is disabled, so we can't create the user user here. + return; + } + + // FIXME: implement rules. + + // Try to get the avatar. + let avatar: string | undefined; + if (photos && photos.length > 0) { + avatar = photos[0].value; + } + + // Try to get the email address. + let email: string | undefined; + let emailVerified: boolean | undefined; + if (emails && emails.length > 0) { + email = emails[0].value; + emailVerified = false; + } + + user = await upsert(this.mongo, tenant, { + username: null, + displayName, + role: GQLUSER_ROLE.COMMENTER, + email, + email_verified: emailVerified, + avatar, + profiles: [profile], + }); + } + + // TODO: maybe update user details? + + return user; + } + + protected createStrategy( + tenant: Tenant, + integration: Required + ) { + return new Strategy( + { + clientID: integration.clientID, + clientSecret: integration.clientSecret, + callbackURL: reconstructTenantURL( + this.config, + tenant, + "/api/tenant/auth/google/callback" + ), + passReqToCallback: true, + }, + this.verifyCallback + ); + } +} diff --git a/src/core/server/app/middleware/passport/strategies/oauth2.ts b/src/core/server/app/middleware/passport/strategies/oauth2.ts index e7f74c685..dbcdcfe89 100644 --- a/src/core/server/app/middleware/passport/strategies/oauth2.ts +++ b/src/core/server/app/middleware/passport/strategies/oauth2.ts @@ -21,6 +21,7 @@ export interface OAuth2StrategyOptions { config: Config; mongo: Db; tenantCache: TenantCache; + scope?: string[]; } export default abstract class OAuth2Strategy< @@ -30,13 +31,15 @@ export default abstract class OAuth2Strategy< protected config: Config; protected mongo: Db; protected cache: TenantCacheAdapter; + private scope?: string[]; - constructor({ config, mongo, tenantCache }: OAuth2StrategyOptions) { + constructor({ config, mongo, tenantCache, scope }: OAuth2StrategyOptions) { super(); this.config = config; this.mongo = mongo; this.cache = new TenantCacheAdapter(tenantCache); + this.scope = scope; } protected abstract getIntegration(integrations: GQLAuthIntegrations): T; @@ -114,7 +117,10 @@ export default abstract class OAuth2Strategy< strategy.redirect = this.redirect.bind(this); strategy.success = this.success.bind(this); - strategy.authenticate(req, { session: false }); + strategy.authenticate(req, { + session: false, + scope: this.scope, + }); } catch (err) { return this.error(err); } diff --git a/src/core/server/app/router/api/auth.ts b/src/core/server/app/router/api/auth.ts index 0bc04a590..e29748e5a 100644 --- a/src/core/server/app/router/api/auth.ts +++ b/src/core/server/app/router/api/auth.ts @@ -8,16 +8,30 @@ import { import { wrapAuthn } from "talk-server/app/middleware/passport"; import { RouterOptions } from "talk-server/app/router/types"; +function wrapPath( + app: AppOptions, + options: RouterOptions, + router: express.Router, + strategy: string, + path: string = `/${strategy}` +) { + const handler = wrapAuthn(options.passport, app.signingConfig, strategy); + + router.get(path, handler); + router.get(path + "/callback", handler); +} + export function createNewAuthRouter(app: AppOptions, options: RouterOptions) { const router = express.Router(); - // Mount the passport routes. + // Mount the logout handler. router.delete( "/", options.passport.authenticate("jwt", { session: false }), logoutHandler({ redis: app.redis }) ); + // Mount the Local Authentication handlers. router.post( "/local", express.json(), @@ -29,24 +43,10 @@ export function createNewAuthRouter(app: AppOptions, options: RouterOptions) { signupHandler({ db: app.mongo, signingConfig: app.signingConfig }) ); - router.get( - "/facebook", - wrapAuthn(options.passport, app.signingConfig, "facebook") - ); - - router.get( - "/facebook/callback", - wrapAuthn(options.passport, app.signingConfig, "facebook") - ); - - router.get( - "/oidc/:oidcID", - wrapAuthn(options.passport, app.signingConfig, "oidc") - ); - router.get( - "/oidc/:oidcID/callback", - wrapAuthn(options.passport, app.signingConfig, "oidc") - ); + // Mount the external auth integrations with middleware/handle wrappers. + wrapPath(app, options, router, "facebook"); + wrapPath(app, options, router, "google"); + wrapPath(app, options, router, "oidc", "/oidc/:oidc"); return router; } diff --git a/src/core/server/models/user.ts b/src/core/server/models/user.ts index e29a54134..52cfc74c1 100644 --- a/src/core/server/models/user.ts +++ b/src/core/server/models/user.ts @@ -37,7 +37,17 @@ export interface FacebookProfile { id: string; } -export type Profile = LocalProfile | OIDCProfile | SSOProfile | FacebookProfile; +export interface GoogleProfile { + type: "google"; + id: string; +} + +export type Profile = + | LocalProfile + | OIDCProfile + | SSOProfile + | FacebookProfile + | GoogleProfile; export interface Token { readonly id: string; diff --git a/src/types/passport-google-oauth2.d.ts b/src/types/passport-google-oauth2.d.ts new file mode 100644 index 000000000..fb0ad8af1 --- /dev/null +++ b/src/types/passport-google-oauth2.d.ts @@ -0,0 +1,38 @@ +declare module "passport-google-oauth2" { + import express from "express"; + import passport from "passport"; + + export interface Profile extends passport.Profile { + id: string; + displayName: string; + } + + export interface AuthenticateOptions extends passport.AuthenticateOptions { + authType?: string; + } + + export interface StrategyOptionWithRequest { + clientID: string; + clientSecret: string; + callbackURL: string; + passReqToCallback: true; + } + + export type VerifyFunctionWithRequest = ( + req: express.Request, + accessToken: string, + refreshToken: string, + profile: Profile, + done: (error: any, user?: any, info?: any) => void + ) => void; + + export class Strategy extends passport.Strategy { + constructor( + options: StrategyOptionWithRequest, + verify: VerifyFunctionWithRequest + ); + + public name: string; + public authenticate(req: express.Request, options?: object): void; + } +} From f39da1f76b7facf66851c9809fc1becb6d2de0ac Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Wed, 31 Oct 2018 17:57:21 -0600 Subject: [PATCH 17/18] review: fixed function name --- .../server/app/middleware/passport/strategies/facebook.ts | 4 ++-- src/core/server/app/middleware/passport/strategies/google.ts | 4 ++-- src/core/server/app/url.ts | 4 ++-- .../server/graph/tenant/resolvers/oidc_auth_integration.ts | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/core/server/app/middleware/passport/strategies/facebook.ts b/src/core/server/app/middleware/passport/strategies/facebook.ts index c0f202737..54859d0b6 100644 --- a/src/core/server/app/middleware/passport/strategies/facebook.ts +++ b/src/core/server/app/middleware/passport/strategies/facebook.ts @@ -3,7 +3,7 @@ import { Profile, Strategy } from "passport-facebook"; import { Config } from "talk-common/config"; import OAuth2Strategy from "talk-server/app/middleware/passport/strategies/oauth2"; -import { reconstructTenantURL } from "talk-server/app/url"; +import { constructTenantURL } from "talk-server/app/url"; import { GQLAuthIntegrations, GQLFacebookAuthIntegration, @@ -90,7 +90,7 @@ export default class FacebookStrategy extends OAuth2Strategy< { clientID: integration.clientID, clientSecret: integration.clientSecret, - callbackURL: reconstructTenantURL( + callbackURL: constructTenantURL( this.config, tenant, "/api/tenant/auth/facebook/callback" diff --git a/src/core/server/app/middleware/passport/strategies/google.ts b/src/core/server/app/middleware/passport/strategies/google.ts index 9e793e722..88fb4d7f7 100644 --- a/src/core/server/app/middleware/passport/strategies/google.ts +++ b/src/core/server/app/middleware/passport/strategies/google.ts @@ -3,7 +3,7 @@ import { Profile, Strategy } from "passport-google-oauth2"; import { Config } from "talk-common/config"; import OAuth2Strategy from "talk-server/app/middleware/passport/strategies/oauth2"; -import { reconstructTenantURL } from "talk-server/app/url"; +import { constructTenantURL } from "talk-server/app/url"; import { GQLAuthIntegrations, GQLGoogleAuthIntegration, @@ -103,7 +103,7 @@ export default class GoogleStrategy extends OAuth2Strategy< { clientID: integration.clientID, clientSecret: integration.clientSecret, - callbackURL: reconstructTenantURL( + callbackURL: constructTenantURL( this.config, tenant, "/api/tenant/auth/google/callback" diff --git a/src/core/server/app/url.ts b/src/core/server/app/url.ts index d9b7da56d..de4686e24 100644 --- a/src/core/server/app/url.ts +++ b/src/core/server/app/url.ts @@ -14,9 +14,9 @@ export function reconstructURL(req: Request, path: string = "/"): string { } /** - * reconstructTenantURL will reconstruct a URL based off of the Tenant's domain. + * constructTenantURL will construct a URL based off of the Tenant's domain. */ -export function reconstructTenantURL( +export function constructTenantURL( config: Config, tenant: Pick, path: string = "/" diff --git a/src/core/server/graph/tenant/resolvers/oidc_auth_integration.ts b/src/core/server/graph/tenant/resolvers/oidc_auth_integration.ts index 44e28b6ec..3bd4aab70 100644 --- a/src/core/server/graph/tenant/resolvers/oidc_auth_integration.ts +++ b/src/core/server/graph/tenant/resolvers/oidc_auth_integration.ts @@ -1,4 +1,4 @@ -import { reconstructTenantURL, reconstructURL } from "talk-server/app/url"; +import { constructTenantURL, reconstructURL } from "talk-server/app/url"; import { GQLOIDCAuthIntegration, GQLOIDCAuthIntegrationTypeResolver, @@ -19,7 +19,7 @@ const OIDCAuthIntegration: GQLOIDCAuthIntegrationTypeResolver< // Note that when constructing the callback url with the tenant, the port // information is lost. - return reconstructTenantURL(ctx.config, ctx.tenant, path); + return constructTenantURL(ctx.config, ctx.tenant, path); }, }; From f0753214e188cf13385b36c33aade95d8072dac9 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Wed, 31 Oct 2018 18:00:30 -0600 Subject: [PATCH 18/18] review: upgraded merged upstream @types/mongodb --- package-lock.json | 7 +++---- package.json | 2 +- src/core/server/models/tenant.ts | 8 ++------ 3 files changed, 6 insertions(+), 11 deletions(-) diff --git a/package-lock.json b/package-lock.json index 8b5dbf36b..22605c63f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -2016,13 +2016,12 @@ } }, "@types/mongodb": { - "version": "3.1.8", - "resolved": "https://registry.npmjs.org/@types/mongodb/-/mongodb-3.1.8.tgz", - "integrity": "sha512-5higsHdPx63XKIh5hjr5GGrCCErBqEbpZZiNsUcqk97mMDpCBH9R4dRi/T8bcMrQItCdL+wecagdAj3JPKkuVg==", + "version": "3.1.14", + "resolved": "https://registry.npmjs.org/@types/mongodb/-/mongodb-3.1.14.tgz", + "integrity": "sha512-Hc9nhu9Z33Gq8SP2CZluNlhwbXBCEGAzLMQPEceZG0wUt/ZTzIGaeo8RXu7FWNXfUd6JHh6KHl0YjQlu6TgncQ==", "dev": true, "requires": { "@types/bson": "*", - "@types/events": "*", "@types/node": "*" } }, diff --git a/package.json b/package.json index 127a62e0d..4a4f4d4c0 100644 --- a/package.json +++ b/package.json @@ -144,7 +144,7 @@ "@types/lodash": "^4.14.111", "@types/luxon": "^0.5.3", "@types/mini-css-extract-plugin": "^0.2.0", - "@types/mongodb": "^3.1.8", + "@types/mongodb": "^3.1.14", "@types/ms": "^0.7.30", "@types/node": "^10.5.2", "@types/node-fetch": "^2.1.2", diff --git a/src/core/server/models/tenant.ts b/src/core/server/models/tenant.ts index 5e7a07061..56a425f04 100644 --- a/src/core/server/models/tenant.ts +++ b/src/core/server/models/tenant.ts @@ -320,18 +320,14 @@ export async function updateTenantOIDCAuthIntegration( const result = await collection(mongo).findOneAndUpdate( { id }, { - // $set: dotize({ - // "auth.integrations.oidc.$[oidc]": input, - // }), - // FIXME: uncomment when https://github.com/DefinitelyTyped/DefinitelyTyped/pull/29986 gets merged $set: dotize({ - "auth.integrations.oidc.$[]": input, + "auth.integrations.oidc.$[oidc]": input, }), }, { // Add an ArrayFilter to only update one of the OpenID Connect // integrations. - // arrayFilters: [{ "oidc.id": oidcID }], // FIXME: uncomment when https://github.com/DefinitelyTyped/DefinitelyTyped/pull/29986 gets merged + arrayFilters: [{ "oidc.id": oidcID }], // False to return the updated document instead of the original // document. returnOriginal: false,