From 348105f59ff0d66998f0c12bb2a29ec25492df79 Mon Sep 17 00:00:00 2001 From: aditya-mitra <55396651+aditya-mitra@users.noreply.github.com> Date: Wed, 11 Oct 2023 12:14:31 +0530 Subject: [PATCH 01/10] refactor(identity-provider): use service hooks and resolvers --- .../identity-provider.class.ts | 245 +----------------- .../identity-provider.hooks.ts | 112 +++++++- .../identity-provider.resolvers.ts | 4 - .../identity-provider/identity-provider.ts | 2 +- 4 files changed, 110 insertions(+), 253 deletions(-) diff --git a/packages/server-core/src/user/identity-provider/identity-provider.class.ts b/packages/server-core/src/user/identity-provider/identity-provider.class.ts index 6db6cfd44a..cc11617b84 100755 --- a/packages/server-core/src/user/identity-provider/identity-provider.class.ts +++ b/packages/server-core/src/user/identity-provider/identity-provider.class.ts @@ -23,9 +23,8 @@ All portions of the code written by the Ethereal Engine team are Copyright © 20 Ethereal Engine. All Rights Reserved. */ -import type { Id, NullableId, Params } from '@feathersjs/feathers' -import type { KnexAdapterOptions } from '@feathersjs/knex' -import { KnexAdapter } from '@feathersjs/knex' +import type { Params } from '@feathersjs/feathers' +import { KnexService } from '@feathersjs/knex' import { IdentityProviderData, @@ -33,21 +32,8 @@ import { IdentityProviderQuery, IdentityProviderType } from '@etherealengine/engine/src/schemas/user/identity-provider.schema' -import { Paginated } from '@feathersjs/feathers' -import { random } from 'lodash' -import { v1 as uuidv1 } from 'uuid' - -import { isDev } from '@etherealengine/common/src/config' -import { avatarPath, AvatarType } from '@etherealengine/engine/src/schemas/user/avatar.schema' - -import { scopePath, ScopeType } from '@etherealengine/engine/src/schemas/scope/scope.schema' -import { userPath, UserType } from '@etherealengine/engine/src/schemas/user/user.schema' -import { Application } from '../../../declarations' import { RootParams } from '../../api/root-params' -import appConfig from '../../appconfig' -import { scopeTypeSeed } from '../../scope/scope-type/scope-type.seed' -import getFreeInviteCode from '../../util/get-free-invite-code' export interface IdentityProviderParams extends RootParams { authentication?: any @@ -60,229 +46,4 @@ export interface IdentityProviderParams extends RootParams extends KnexAdapter { - app: Application - - constructor(options: KnexAdapterOptions, app: Application) { - super(options) - this.app = app - } - - async create(data: IdentityProviderData, params?: IdentityProviderParams) { - if (!params) params = {} - let { token, type } = data - let user - let authResult - - if (params?.authentication) { - authResult = await (this.app.service('authentication') as any).strategies.jwt.authenticate( - { accessToken: params?.authentication.accessToken }, - {} - ) - if (authResult[appConfig.authentication.entity]?.userId) { - user = await this.app.service(userPath).get(authResult[appConfig.authentication.entity]?.userId) - } - } - if ( - (!user || !user.scopes || !user.scopes.find((scope) => scope.type === 'admin:admin')) && - params?.provider && - type !== 'password' && - type !== 'email' && - type !== 'sms' - ) - type = 'guest' //Non-password/magiclink create requests must always be for guests - - let userId = data.userId || (authResult ? authResult[appConfig.authentication.entity]?.userId : null) - let identityProvider: IdentityProviderData = { ...data } - - switch (type) { - case 'email': - identityProvider = { - ...identityProvider, - token, - type - } - break - case 'sms': - identityProvider = { - ...identityProvider, - token, - type - } - break - case 'password': - identityProvider = { - ...identityProvider, - token, - type - } - break - case 'github': - identityProvider = { - ...identityProvider, - token: token, - type - } - break - case 'facebook': - identityProvider = { - ...identityProvider, - token: token, - type - } - break - case 'google': - identityProvider = { - ...identityProvider, - token: token, - type - } - break - case 'twitter': - identityProvider = { - ...identityProvider, - token: token, - type - } - break - case 'linkedin': - identityProvider = { - ...identityProvider, - token: token, - type - } - break - case 'discord': - identityProvider = { - ...identityProvider, - token: token, - type - } - break - case 'guest': - identityProvider = { - ...identityProvider, - token: token, - type: type - } - break - case 'auth0': - break - } - - // if userId is not defined, then generate userId - if (!userId) { - userId = uuidv1() - } - - // check if there is a user with userId - let foundUser - try { - foundUser = await this.app.service(userPath).get(userId) - } catch (err) { - // - } - - if (foundUser != null) { - // if there is the user with userId, then we add the identity provider to the user - return await super._create( - { - ...identityProvider, - userId - }, - params - ) - } - - const code = await getFreeInviteCode(this.app) - // if there is no user with userId, then we create a user and a identity provider. - const adminCount = (await this.app.service(scopePath).find({ - query: { - type: 'admin:admin' - } - })) as Paginated - - const avatars = (await this.app - .service(avatarPath) - .find({ isInternal: true, query: { $limit: 1000 } })) as Paginated - - const isGuest = type === 'guest' - - if (adminCount.data.length === 0) { - // in dev mode make the first guest an admin - // otherwise make the first logged in user an admin - if (isDev || !isGuest) { - type = 'admin' - } - } - - let result: IdentityProviderType - try { - const newUser = (await this.app.service(userPath).create({ - id: userId, - isGuest, - inviteCode: type === 'guest' ? '' : code, - avatarId: avatars.data[random(avatars.data.length - 1)].id - })) as UserType - - result = await super._create( - { - ...identityProvider, - userId: newUser.id - }, - params - ) - } catch (err) { - console.error(err) - await this.app.service(userPath).remove(userId) - throw err - } - // DRC - - if (type === 'guest') { - if (appConfig.scopes.guest.length) { - const data = appConfig.scopes.guest.map((el) => { - return { - type: el, - userId - } - }) - await this.app.service(scopePath).create(data) - } - - result.accessToken = await this.app - .service('authentication') - .createAccessToken({}, { subject: result.id.toString() }) - } else if (isDev && type === 'admin') { - // in dev mode, add all scopes to the first user made an admin - const data = scopeTypeSeed.map(({ type }) => { - return { userId, type } - }) - await this.app.service(scopePath).create(data) - - result.accessToken = await this.app - .service('authentication') - .createAccessToken({}, { subject: result.id.toString() }) - } - - return result - } - - async find(params?: IdentityProviderParams) { - const loggedInUser = params!.user as UserType - if (params!.provider) params!.query!.userId = loggedInUser.id - return super._find(params) - } - - async get(id: Id, params?: IdentityProviderParams) { - return super._get(id, params) - } - - async patch(id: Id, data: IdentityProviderData, params?: IdentityProviderParams) { - return super._patch(id, data, params) - } - - async remove(id: NullableId, params?: IdentityProviderParams) { - return super._remove(id, params) - } -} +> extends KnexService {} diff --git a/packages/server-core/src/user/identity-provider/identity-provider.hooks.ts b/packages/server-core/src/user/identity-provider/identity-provider.hooks.ts index e47b856f2b..fe1e713a18 100755 --- a/packages/server-core/src/user/identity-provider/identity-provider.hooks.ts +++ b/packages/server-core/src/user/identity-provider/identity-provider.hooks.ts @@ -23,9 +23,6 @@ All portions of the code written by the Ethereal Engine team are Copyright © 20 Ethereal Engine. All Rights Reserved. */ -import { hooks as schemaHooks } from '@feathersjs/schema' -import { iff, isProvider } from 'feathers-hooks-common' - import { IdentityProviderType, identityProviderDataValidator, @@ -35,9 +32,20 @@ import { } from '@etherealengine/engine/src/schemas/user/identity-provider.schema' import { Forbidden, MethodNotAllowed, NotFound } from '@feathersjs/errors' import { HookContext } from '@feathersjs/feathers' +import { hooks as schemaHooks } from '@feathersjs/schema' +import { iff, isProvider } from 'feathers-hooks-common' +import appConfig from '../../appconfig' import authenticate from '../../hooks/authenticate' +import { isDev } from '@etherealengine/common/src/config' +import { checkScope } from '@etherealengine/engine/src/common/functions/checkScope' +import { scopePath } from '@etherealengine/engine/src/schemas/scope/scope.schema' +import { avatarPath } from '@etherealengine/engine/src/schemas/user/avatar.schema' +import { userPath } from '@etherealengine/engine/src/schemas/user/user.schema' +import { random } from 'lodash' +import { Application } from '../../../declarations' +import { scopeTypeSeed } from '../../scope/scope-type/scope-type.seed' import { identityProviderDataResolver, identityProviderExternalResolver, @@ -99,6 +107,94 @@ const checkOnlyIdentityProvider = () => { } } +function addUserId(key: 'data' | 'query') { + return (context: HookContext) => { + if (key === 'data') { + context.data.userId = context.existingUser!.id + } else { + if (context.params.provider) context.params.query.userId = context.params.user!.id + } + } +} + +/** (BEFORE) CREATE HOOKS **/ + +// async function validateAuthParams(context:HookContext) { +async function validateAuthParams(context: HookContext) { + let userId = context.data.userId + + if (context.params.authentication) { + const authResult = await (context.app.service('authentication') as any).strategies.jwt.authenticate( + { accessToken: context.params.authentication.accessToken }, + {} + ) + userId = userId || authResult[appConfig.authentication.entity]?.userId + } + + if (!userId) { + return + } + + const user = await context.app.service(userPath).get(userId) + + context.existingUser = user +} + +async function addIdentityProviderType(context: HookContext) { + const isAdmin = context.existingUser && (await checkScope(context.existingUser, 'admin', 'admin')) + if (!isAdmin && context.params!.provider && !['password', 'email', 'sms'].includes(context.data.type)) { + context.data.type = 'guest' + } + + const adminScopes = await context.app.service(scopePath).find({ + query: { + type: 'admin:admin' + } + }) + + if (adminScopes.total === 0 && isDev && context.data.type !== 'guest') { + // doubt here - it should be the first if condition + context.data.type = 'admin' + } +} + +async function createNewUser(context: HookContext) { + const isGuest = context.data.type === 'guest' + const avatars = await context.app.service(avatarPath).find({ isInternal: true, query: { $limit: 1000 } } as any) + + const newUser = await context.app.service(userPath).create({ + isGuest, + avatarId: avatars.data[random(avatars.data.length - 1)].id + }) + + context.existingUser = newUser +} + +/** (AFTER) CREATE HOOKS **/ + +async function addScopes(context: HookContext) { + if (context.data.type === 'guest' && appConfig.scopes.guest.length) { + const data = appConfig.scopes.guest.map((el) => ({ + type: el, + userId: context.existingUser!.id + })) + await context.app.service(scopePath).create(data) + } + + if (isDev && context.data.type === 'admin') { + const data = scopeTypeSeed.map(({ type }) => ({ userId: context.existingUser!.id, type })) + await context.app.service(scopePath).create(data) + } +} + +async function createAccessToken(context: HookContext) { + if (!context.result!.accessToken) { + context.result.accessToken = await (context.app as Application) + .service('authentication') + .createAccessToken({}, { subject: context.result.id.toString() }) + } +} + export default { around: { all: [ @@ -112,11 +208,15 @@ export default { () => schemaHooks.validateQuery(identityProviderQueryValidator), schemaHooks.resolveQuery(identityProviderQueryResolver) ], - find: [iff(isProvider('external'), authenticate() as any)], + find: [iff(isProvider('external'), authenticate() as any), addUserId('query')], get: [iff(isProvider('external'), authenticate() as any, checkIdentityProvider())], create: [ () => schemaHooks.validateData(identityProviderDataValidator), - schemaHooks.resolveData(identityProviderDataResolver) + schemaHooks.resolveData(identityProviderDataResolver), + validateAuthParams, + addIdentityProviderType, + iff((context: HookContext) => !context.existingUser, createNewUser), + addUserId('data') ], update: [iff(isProvider('external'), authenticate() as any, checkIdentityProvider())], patch: [ @@ -130,7 +230,7 @@ export default { all: [], find: [], get: [], - create: [], + create: [addScopes, createAccessToken], update: [], patch: [], remove: [] diff --git a/packages/server-core/src/user/identity-provider/identity-provider.resolvers.ts b/packages/server-core/src/user/identity-provider/identity-provider.resolvers.ts index 1993ece654..2e9bf82c16 100644 --- a/packages/server-core/src/user/identity-provider/identity-provider.resolvers.ts +++ b/packages/server-core/src/user/identity-provider/identity-provider.resolvers.ts @@ -33,7 +33,6 @@ import { } from '@etherealengine/engine/src/schemas/user/identity-provider.schema' import type { HookContext } from '@etherealengine/server-core/declarations' -import { UserID } from '@etherealengine/engine/src/schemas/user/user.schema' import { fromDateTimeSql, getDateTimeSql } from '../../util/datetime-sql' export const identityProviderResolver = resolve({ @@ -47,9 +46,6 @@ export const identityProviderDataResolver = resolve { return v4() }, - userId: async (userId) => { - return userId || (v4() as UserID) - }, createdAt: getDateTimeSql, updatedAt: getDateTimeSql }) diff --git a/packages/server-core/src/user/identity-provider/identity-provider.ts b/packages/server-core/src/user/identity-provider/identity-provider.ts index d5ac5ce058..9cb685c108 100755 --- a/packages/server-core/src/user/identity-provider/identity-provider.ts +++ b/packages/server-core/src/user/identity-provider/identity-provider.ts @@ -47,7 +47,7 @@ export default (app: Application): void => { multi: true } - app.use(identityProviderPath, new IdentityProviderService(options, app), { + app.use(identityProviderPath, new IdentityProviderService(options), { // A list of all methods this service exposes externally methods: identityProviderMethods, // You can add additional custom events to be sent to clients here From 55ffa33341863afe327a243c19764b9e09134dad Mon Sep 17 00:00:00 2001 From: aditya-mitra <55396651+aditya-mitra@users.noreply.github.com> Date: Wed, 11 Oct 2023 12:14:58 +0530 Subject: [PATCH 02/10] fix: do not throw error if no scopes are available --- packages/engine/src/common/functions/checkScope.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/engine/src/common/functions/checkScope.ts b/packages/engine/src/common/functions/checkScope.ts index 62d7f910b2..feba74933b 100644 --- a/packages/engine/src/common/functions/checkScope.ts +++ b/packages/engine/src/common/functions/checkScope.ts @@ -23,7 +23,6 @@ All portions of the code written by the Ethereal Engine team are Copyright © 20 Ethereal Engine. All Rights Reserved. */ -import { NotFound } from '@feathersjs/errors' import { Engine } from '../../ecs/classes/Engine' import { ScopeType, scopePath } from '../../schemas/scope/scope.schema' import { UserType } from '../../schemas/user/user.schema' @@ -36,7 +35,9 @@ export const checkScope = async (user: UserType, currentType: string, scopeToVer } })) as any as ScopeType[] - if (!scopes || scopes.length === 0) throw new NotFound('No scope available for the current user.') + if (!scopes || scopes.length === 0) { + return false + } const currentScopes = scopes.reduce((result, sc) => { if (sc.type.split(':')[0] === currentType) result.push(sc.type.split(':')[1]) From 8cf5194a967f2fe5609529ce43621b318d6159a9 Mon Sep 17 00:00:00 2001 From: aditya-mitra <55396651+aditya-mitra@users.noreply.github.com> Date: Wed, 11 Oct 2023 12:15:23 +0530 Subject: [PATCH 03/10] tests: test more result properties --- .../identity-provider.test.ts | 101 ++++++++---------- 1 file changed, 45 insertions(+), 56 deletions(-) diff --git a/packages/server-core/src/user/identity-provider/identity-provider.test.ts b/packages/server-core/src/user/identity-provider/identity-provider.test.ts index 39654e5a5d..1b2f20e466 100755 --- a/packages/server-core/src/user/identity-provider/identity-provider.test.ts +++ b/packages/server-core/src/user/identity-provider/identity-provider.test.ts @@ -34,13 +34,12 @@ import { } from '@etherealengine/engine/src/schemas/user/identity-provider.schema' import { UserID } from '@etherealengine/engine/src/schemas/user/user.schema' -import { Paginated } from '@feathersjs/feathers' import { Application } from '../../../declarations' import { createFeathersKoaApp } from '../../createApp' let userId: UserID -describe('identity-provider service', () => { +describe('identity-provider.service', () => { let app: Application let providers: IdentityProviderType[] = [] @@ -53,94 +52,84 @@ describe('identity-provider service', () => { return destroyEngine() }) - it('registered the service', async () => { - const service = await app.service(identityProviderPath) - assert.ok(service, 'Registered the service') - }) - it('should create an identity provider for guest', async () => { const type = 'guest' const token = v1() - const item = await app.service(identityProviderPath).create( - { - type, - token, - userId: '' as UserID - }, - {} - ) + const createdIdentityProvider = await app.service(identityProviderPath).create({ + type, + token, + userId: '' as UserID + }) - providers.push(item) + providers.push(createdIdentityProvider) - userId = item.userId + userId = createdIdentityProvider.userId - assert.equal(item.type, type) - assert.equal(item.token, token) - assert.ok(item.userId) + assert.equal(createdIdentityProvider.type, type) + assert.equal(createdIdentityProvider.token, token) + assert.ok(createdIdentityProvider.accessToken) + assert.ok(createdIdentityProvider.userId) }) it('should create an identity provider for email', async () => { const type = 'email' const token = v1() - const item = await app.service(identityProviderPath).create( - { - type, - token, - userId - }, - {} - ) + const createdIdentityProvider = await app.service(identityProviderPath).create({ + type, + token, + userId + }) - providers.push(item) + providers.push(createdIdentityProvider) - assert.equal(item.type, type) - assert.equal(item.token, token) - assert.ok(item.userId) + assert.equal(createdIdentityProvider.type, type) + assert.equal(createdIdentityProvider.token, token) + assert.ok(createdIdentityProvider.accessToken) + assert.ok(createdIdentityProvider.userId) }) it('should create an identity provider for password', async () => { const type = 'password' const token = v1() - const item = await app.service(identityProviderPath).create( - { - type, - token, - userId - }, - {} - ) + const createdIdentityProvider = await app.service(identityProviderPath).create({ + type, + token, + userId + }) - providers.push(item) + providers.push(createdIdentityProvider) - assert.equal(item.type, type) - assert.equal(item.token, token) - assert.ok(item.userId) + assert.equal(createdIdentityProvider.type, type) + assert.equal(createdIdentityProvider.token, token) + assert.ok(createdIdentityProvider.accessToken) + assert.ok(createdIdentityProvider.userId) }) it('should find identity providers', async () => { - const item = (await app.service(identityProviderPath).find({ + const foundIdentityProviders = await app.service(identityProviderPath).find({ query: { userId - } - })) as Paginated + }, + isInternal: true + }) - assert.ok(item, 'Identity provider item is found') - assert.equal(item.total, providers.length) + assert.ok(foundIdentityProviders) + assert.equal(foundIdentityProviders.total, providers.length) }) it('should remove an identity provider by id', async () => { await app.service(identityProviderPath).remove(providers[0].id) - const item = (await app.service(identityProviderPath).find({ + const foundIdentityProviders = await app.service(identityProviderPath).find({ query: { id: providers[0].id } - })) as Paginated + }) - assert.equal(item.total, 0) + assert.equal(foundIdentityProviders.total, 0) }) it('should not be able to remove identity providers by user id', async () => { @@ -161,7 +150,7 @@ describe('identity-provider service', () => { const type = 'guest' const token = v1() - const item = await app.service(identityProviderPath).create( + const foundIdentityProvider = await app.service(identityProviderPath).create( { type, token, @@ -170,14 +159,14 @@ describe('identity-provider service', () => { {} ) - assert.ok(() => app.service(identityProviderPath).remove(item.id)) + assert.ok(() => app.service(identityProviderPath).remove(foundIdentityProvider.id)) }) it('should not be able to remove the only identity provider as a user', async () => { const type = 'user' const token = v1() - const item = await app.service(identityProviderPath).create( + const foundIdentityProvider = await app.service(identityProviderPath).create( { type, token, @@ -186,7 +175,7 @@ describe('identity-provider service', () => { {} ) - assert.rejects(() => app.service(identityProviderPath).remove(item.id), { + assert.rejects(() => app.service(identityProviderPath).remove(foundIdentityProvider.id), { name: 'MethodNotAllowed' }) }) From 2549a4dfaa47d0c80f6aeaf29d93e982f6093c1e Mon Sep 17 00:00:00 2001 From: aditya-mitra <55396651+aditya-mitra@users.noreply.github.com> Date: Wed, 11 Oct 2023 12:15:52 +0530 Subject: [PATCH 04/10] improve: use getFreeInviteCode in user resolvers --- packages/server-core/src/user/user/user.resolvers.ts | 5 +++-- packages/server-core/src/util/get-free-invite-code.ts | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/server-core/src/user/user/user.resolvers.ts b/packages/server-core/src/user/user/user.resolvers.ts index 5693a0dd13..fc743e0281 100644 --- a/packages/server-core/src/user/user/user.resolvers.ts +++ b/packages/server-core/src/user/user/user.resolvers.ts @@ -47,6 +47,7 @@ import { import { UserApiKeyType, userApiKeyPath } from '@etherealengine/engine/src/schemas/user/user-api-key.schema' import { UserSettingType, userSettingPath } from '@etherealengine/engine/src/schemas/user/user-setting.schema' import { fromDateTimeSql, getDateTimeSql } from '../../util/datetime-sql' +import getFreeInviteCode from '../../util/get-free-invite-code' export const userResolver = resolve({ identityProviders: virtual(async (user, context) => { @@ -142,8 +143,8 @@ export const userDataResolver = resolve({ name: async (name) => { return name || 'Guest #' + Math.floor(Math.random() * (999 - 100 + 1) + 100) }, - inviteCode: async (inviteCode) => { - return inviteCode || Math.random().toString(36).slice(2) + inviteCode: async (inviteCode, _, context) => { + return inviteCode || (await getFreeInviteCode(context.app)) }, avatarId: async (avatarId) => { return avatarId || undefined diff --git a/packages/server-core/src/util/get-free-invite-code.ts b/packages/server-core/src/util/get-free-invite-code.ts index 957b593cfd..c6f1f21823 100755 --- a/packages/server-core/src/util/get-free-invite-code.ts +++ b/packages/server-core/src/util/get-free-invite-code.ts @@ -25,8 +25,9 @@ Ethereal Engine. All Rights Reserved. import { userPath } from '@etherealengine/engine/src/schemas/user/user.schema' import crypto from 'crypto' +import { Application } from '../../declarations' -const getFreeInviteCode = async (app): Promise => { +const getFreeInviteCode = async (app: Application): Promise => { const code = crypto.randomBytes(4).toString('hex') const users = await app.service(userPath).find({ query: { From 0ff62e22e1ae77e96a0885d85e6a0a9cdf0a1736 Mon Sep 17 00:00:00 2001 From: aditya-mitra <55396651+aditya-mitra@users.noreply.github.com> Date: Wed, 11 Oct 2023 12:23:07 +0530 Subject: [PATCH 05/10] improve: identity provider hooks --- .../identity-provider.hooks.ts | 88 +++++++++---------- 1 file changed, 41 insertions(+), 47 deletions(-) diff --git a/packages/server-core/src/user/identity-provider/identity-provider.hooks.ts b/packages/server-core/src/user/identity-provider/identity-provider.hooks.ts index 7e65a000b6..7f4f549fd9 100755 --- a/packages/server-core/src/user/identity-provider/identity-provider.hooks.ts +++ b/packages/server-core/src/user/identity-provider/identity-provider.hooks.ts @@ -52,57 +52,51 @@ import { identityProviderResolver } from './identity-provider.resolvers' -const checkIdentityProvider = (): any => { - return async (context: HookContext): Promise => { - if (context.id) { - // If trying to CRUD a specific identity-provider, throw 404 if the user doesn't own it - const thisIdentityProvider = (await context.app - .service(identityProviderPath) - .get(context.id)) as IdentityProviderType - if ( - !context.params.user || - !thisIdentityProvider || - (context.params.user && thisIdentityProvider && context.params.user.id !== thisIdentityProvider.userId) - ) - throw new NotFound() - } else { - // If trying to CRUD multiple identity-providers, e.g. patch all IP's belonging to a user, make params.query.userId - // the ID of the calling user, so no one can alter anyone else's IPs. - const userId = context.params[identityProviderPath]?.userId - if (!userId) throw new NotFound() - if (!context.params.query) context.params.query = {} - context.params.query.userId = userId - } - if (context.data) context.data = { password: context.data.password } //If patching externally, should only be able to change password - return context - } -} - -const checkOnlyIdentityProvider = () => { - return async (context: HookContext): Promise => { - if (!context.id) { - // do not allow to remove identity providers in bulk - throw new MethodNotAllowed('Cannot remove multiple providers together') - } +async function checkIdentityProvider(context: HookContext): Promise { + if (context.id) { + // If trying to CRUD a specific identity-provider, throw 404 if the user doesn't own it const thisIdentityProvider = (await context.app .service(identityProviderPath) .get(context.id)) as IdentityProviderType + if ( + !context.params.user || + !thisIdentityProvider || + (context.params.user && thisIdentityProvider && context.params.user.id !== thisIdentityProvider.userId) + ) + throw new NotFound() + } else { + // If trying to CRUD multiple identity-providers, e.g. patch all IP's belonging to a user, make params.query.userId + // the ID of the calling user, so no one can alter anyone else's IPs. + const userId = context.params[identityProviderPath]?.userId + if (!userId) throw new NotFound() + if (!context.params.query) context.params.query = {} + context.params.query.userId = userId + } + if (context.data) context.data = { password: context.data.password } //If patching externally, should only be able to change password + return context +} + +async function checkOnlyIdentityProvider(context: HookContext): Promise { + if (!context.id) { + // do not allow to remove identity providers in bulk + throw new MethodNotAllowed('Cannot remove multiple providers together') + } + const thisIdentityProvider = (await context.app.service(identityProviderPath).get(context.id)) as IdentityProviderType - if (!thisIdentityProvider) throw new Forbidden('You do not have any identity provider') + if (!thisIdentityProvider) throw new Forbidden('You do not have any identity provider') - // we only want to disallow removing the last identity provider if it is not a guest - // since the guest user will be destroyed once they log in - if (thisIdentityProvider.type === 'guest') return context + // we only want to disallow removing the last identity provider if it is not a guest + // since the guest user will be destroyed once they log in + if (thisIdentityProvider.type === 'guest') return context - const providers = await context.app - .service(identityProviderPath) - .find({ query: { userId: thisIdentityProvider.userId } }) + const providers = await context.app + .service(identityProviderPath) + .find({ query: { userId: thisIdentityProvider.userId } }) - if (providers.total <= 1) { - throw new MethodNotAllowed('Cannot remove the only identity provider on a user') - } - return context + if (providers.total <= 1) { + throw new MethodNotAllowed('Cannot remove the only identity provider on a user') } + return context } function addUserId(key: 'data' | 'query') { @@ -207,7 +201,7 @@ export default { schemaHooks.resolveQuery(identityProviderQueryResolver) ], find: [addUserId('query')], - get: [iff(isProvider('external'), checkIdentityProvider())], + get: [iff(isProvider('external'), checkIdentityProvider)], create: [ () => schemaHooks.validateData(identityProviderDataValidator), schemaHooks.resolveData(identityProviderDataResolver), @@ -216,13 +210,13 @@ export default { iff((context: HookContext) => !context.existingUser, createNewUser), addUserId('data') ], - update: [iff(isProvider('external'), checkIdentityProvider())], + update: [iff(isProvider('external'), checkIdentityProvider)], patch: [ - iff(isProvider('external'), checkIdentityProvider()), + iff(isProvider('external'), checkIdentityProvider), () => schemaHooks.validateData(identityProviderPatchValidator), schemaHooks.resolveData(identityProviderPatchResolver) ], - remove: [iff(isProvider('external'), checkIdentityProvider()), checkOnlyIdentityProvider()] + remove: [iff(isProvider('external'), checkIdentityProvider), checkOnlyIdentityProvider] }, after: { all: [], From f02c021e1bfd9fa9a1c0401e1c1cf94f7e0a6fc3 Mon Sep 17 00:00:00 2001 From: aditya-mitra <55396651+aditya-mitra@users.noreply.github.com> Date: Mon, 16 Oct 2023 14:02:39 +0530 Subject: [PATCH 06/10] fix: use existing set-logged-in-user hook --- .../identity-provider.hooks.ts | 23 +++---------------- 1 file changed, 3 insertions(+), 20 deletions(-) diff --git a/packages/server-core/src/user/identity-provider/identity-provider.hooks.ts b/packages/server-core/src/user/identity-provider/identity-provider.hooks.ts index 7f4f549fd9..2e9ecc2abe 100755 --- a/packages/server-core/src/user/identity-provider/identity-provider.hooks.ts +++ b/packages/server-core/src/user/identity-provider/identity-provider.hooks.ts @@ -43,6 +43,7 @@ import { avatarPath } from '@etherealengine/engine/src/schemas/user/avatar.schem import { userPath } from '@etherealengine/engine/src/schemas/user/user.schema' import { random } from 'lodash' import { Application } from '../../../declarations' +import setLoggedinUserInQuery from '../../hooks/set-loggedin-user-in-query' import { scopeTypeSeed } from '../../scope/scope-type/scope-type.seed' import { identityProviderDataResolver, @@ -99,16 +100,6 @@ async function checkOnlyIdentityProvider(context: HookContext): Promise { - if (key === 'data') { - context.data.userId = context.existingUser!.id - } else { - if (context.params.provider) context.params.query.userId = context.params.user!.id - } - } -} - /** (BEFORE) CREATE HOOKS **/ // async function validateAuthParams(context:HookContext) { @@ -165,14 +156,6 @@ async function createNewUser(context: HookContext) { /** (AFTER) CREATE HOOKS **/ async function addScopes(context: HookContext) { - if (context.data.type === 'guest' && appConfig.scopes.guest.length) { - const data = appConfig.scopes.guest.map((el) => ({ - type: el, - userId: context.existingUser!.id - })) - await context.app.service(scopePath).create(data) - } - if (isDev && context.data.type === 'admin') { const data = scopeTypeSeed.map(({ type }) => ({ userId: context.existingUser!.id, type })) await context.app.service(scopePath).create(data) @@ -200,7 +183,7 @@ export default { () => schemaHooks.validateQuery(identityProviderQueryValidator), schemaHooks.resolveQuery(identityProviderQueryResolver) ], - find: [addUserId('query')], + find: [setLoggedinUserInQuery('userId')], get: [iff(isProvider('external'), checkIdentityProvider)], create: [ () => schemaHooks.validateData(identityProviderDataValidator), @@ -208,7 +191,7 @@ export default { validateAuthParams, addIdentityProviderType, iff((context: HookContext) => !context.existingUser, createNewUser), - addUserId('data') + (context: HookContext) => (context.data.userId = context.existingUser!.id) ], update: [iff(isProvider('external'), checkIdentityProvider)], patch: [ From 6bd0d7282249f20dd295c91499d3129d28bfb078 Mon Sep 17 00:00:00 2001 From: aditya-mitra <55396651+aditya-mitra@users.noreply.github.com> Date: Mon, 16 Oct 2023 14:41:08 +0530 Subject: [PATCH 07/10] fix: add the correct typing in HookContext --- .../identity-provider.class.ts | 4 - .../identity-provider.hooks.ts | 85 ++++++++++--------- 2 files changed, 46 insertions(+), 43 deletions(-) diff --git a/packages/server-core/src/user/identity-provider/identity-provider.class.ts b/packages/server-core/src/user/identity-provider/identity-provider.class.ts index 2e9940fb43..a8a76dcee4 100755 --- a/packages/server-core/src/user/identity-provider/identity-provider.class.ts +++ b/packages/server-core/src/user/identity-provider/identity-provider.class.ts @@ -38,10 +38,6 @@ import { KnexAdapterParams } from '@feathersjs/knex' export interface IdentityProviderParams extends KnexAdapterParams { authentication?: any } - -/** - * A class for IdentityProvider service - */ export class IdentityProviderService< T = IdentityProviderType, ServiceParams extends Params = IdentityProviderParams diff --git a/packages/server-core/src/user/identity-provider/identity-provider.hooks.ts b/packages/server-core/src/user/identity-provider/identity-provider.hooks.ts index 2e9ecc2abe..b3209ab95e 100755 --- a/packages/server-core/src/user/identity-provider/identity-provider.hooks.ts +++ b/packages/server-core/src/user/identity-provider/identity-provider.hooks.ts @@ -24,14 +24,14 @@ Ethereal Engine. All Rights Reserved. */ import { + IdentityProviderData, IdentityProviderType, identityProviderDataValidator, identityProviderPatchValidator, identityProviderPath, identityProviderQueryValidator } from '@etherealengine/engine/src/schemas/user/identity-provider.schema' -import { Forbidden, MethodNotAllowed, NotFound } from '@feathersjs/errors' -import { HookContext } from '@feathersjs/feathers' +import { BadRequest, Forbidden, MethodNotAllowed, NotFound } from '@feathersjs/errors' import { hooks as schemaHooks } from '@feathersjs/schema' import { iff, isProvider } from 'feathers-hooks-common' import appConfig from '../../appconfig' @@ -42,9 +42,10 @@ import { scopePath } from '@etherealengine/engine/src/schemas/scope/scope.schema import { avatarPath } from '@etherealengine/engine/src/schemas/user/avatar.schema' import { userPath } from '@etherealengine/engine/src/schemas/user/user.schema' import { random } from 'lodash' -import { Application } from '../../../declarations' +import { HookContext } from '../../../declarations' import setLoggedinUserInQuery from '../../hooks/set-loggedin-user-in-query' import { scopeTypeSeed } from '../../scope/scope-type/scope-type.seed' +import { IdentityProviderService } from './identity-provider.class' import { identityProviderDataResolver, identityProviderExternalResolver, @@ -53,41 +54,40 @@ import { identityProviderResolver } from './identity-provider.resolvers' -async function checkIdentityProvider(context: HookContext): Promise { +/** + * If trying to CRUD multiple identity-providers (e.g. patch all IP's belonging to a user), + * make `params.query.userId` the ID of the calling user, so no one can alter anyone else's IPs. + */ +async function checkIdentityProvider(context: HookContext): Promise { if (context.id) { - // If trying to CRUD a specific identity-provider, throw 404 if the user doesn't own it - const thisIdentityProvider = (await context.app - .service(identityProviderPath) - .get(context.id)) as IdentityProviderType + const thisIdentityProvider = await context.app.service(identityProviderPath).get(context.id) if ( !context.params.user || !thisIdentityProvider || (context.params.user && thisIdentityProvider && context.params.user.id !== thisIdentityProvider.userId) ) - throw new NotFound() + throw new MethodNotAllowed('authenticated user is not owner of this identity provider') } else { - // If trying to CRUD multiple identity-providers, e.g. patch all IP's belonging to a user, make params.query.userId - // the ID of the calling user, so no one can alter anyone else's IPs. const userId = context.params[identityProviderPath]?.userId if (!userId) throw new NotFound() if (!context.params.query) context.params.query = {} context.params.query.userId = userId } - if (context.data) context.data = { password: context.data.password } //If patching externally, should only be able to change password return context } -async function checkOnlyIdentityProvider(context: HookContext): Promise { +/** + * do not allow to remove the identity providers in bulk + * and we want to disallow removing the last identity provider for non-guest users + */ +async function checkOnlyIdentityProvider(context: HookContext) { if (!context.id) { - // do not allow to remove identity providers in bulk throw new MethodNotAllowed('Cannot remove multiple providers together') } - const thisIdentityProvider = (await context.app.service(identityProviderPath).get(context.id)) as IdentityProviderType + const thisIdentityProvider = await context.app.service(identityProviderPath).get(context.id) if (!thisIdentityProvider) throw new Forbidden('You do not have any identity provider') - // we only want to disallow removing the last identity provider if it is not a guest - // since the guest user will be destroyed once they log in if (thisIdentityProvider.type === 'guest') return context const providers = await context.app @@ -102,12 +102,15 @@ async function checkOnlyIdentityProvider(context: HookContext): Promise) { -async function validateAuthParams(context: HookContext) { - let userId = context.data.userId +async function validateAuthParams(context: HookContext) { + if (Array.isArray(context.data)) { + throw new MethodNotAllowed('identity-provider create works only with singular entries') + } + + let userId = context.data!.userId if (context.params.authentication) { - const authResult = await (context.app.service('authentication') as any).strategies.jwt.authenticate( + const authResult = await context.app.service('authentication').strategies.jwt.authenticate!( { accessToken: context.params.authentication.accessToken }, {} ) @@ -115,7 +118,7 @@ async function validateAuthParams(context: HookContext) { } if (!userId) { - return + throw new BadRequest('userId not found') } const user = await context.app.service(userPath).get(userId) @@ -123,10 +126,14 @@ async function validateAuthParams(context: HookContext) { context.existingUser = user } -async function addIdentityProviderType(context: HookContext) { +async function addIdentityProviderType(context: HookContext) { const isAdmin = context.existingUser && (await checkScope(context.existingUser, 'admin', 'admin')) - if (!isAdmin && context.params!.provider && !['password', 'email', 'sms'].includes(context.data.type)) { - context.data.type = 'guest' + if ( + !isAdmin && + context.params!.provider && + !['password', 'email', 'sms'].includes((context!.data as IdentityProviderData).type) + ) { + ;(context.data as IdentityProviderData).type = 'guest' } const adminScopes = await context.app.service(scopePath).find({ @@ -135,15 +142,14 @@ async function addIdentityProviderType(context: HookContext) { } }) - if (adminScopes.total === 0 && isDev && context.data.type !== 'guest') { - // doubt here - it should be the first if condition - context.data.type = 'admin' + if (adminScopes.total === 0 && isDev && (context.data as IdentityProviderData).type !== 'guest') { + ;(context.data as IdentityProviderData).type = 'admin' } } -async function createNewUser(context: HookContext) { - const isGuest = context.data.type === 'guest' - const avatars = await context.app.service(avatarPath).find({ isInternal: true, query: { $limit: 1000 } } as any) +async function createNewUser(context: HookContext) { + const isGuest = (context.data as IdentityProviderType).type === 'guest' + const avatars = await context.app.service(avatarPath).find({ isInternal: true, query: { $limit: 1000 } }) const newUser = await context.app.service(userPath).create({ isGuest, @@ -155,18 +161,18 @@ async function createNewUser(context: HookContext) { /** (AFTER) CREATE HOOKS **/ -async function addScopes(context: HookContext) { - if (isDev && context.data.type === 'admin') { +async function addScopes(context: HookContext) { + if (isDev && (context.data as IdentityProviderType).type === 'admin') { const data = scopeTypeSeed.map(({ type }) => ({ userId: context.existingUser!.id, type })) await context.app.service(scopePath).create(data) } } -async function createAccessToken(context: HookContext) { - if (!context.result!.accessToken) { - context.result.accessToken = await (context.app as Application) +async function createAccessToken(context: HookContext) { + if (!(context.result as IdentityProviderType).accessToken) { + ;(context.result as IdentityProviderType).accessToken = await context.app .service('authentication') - .createAccessToken({}, { subject: context.result.id.toString() }) + .createAccessToken({}, { subject: (context.result as IdentityProviderType).id.toString() }) } } @@ -190,8 +196,9 @@ export default { schemaHooks.resolveData(identityProviderDataResolver), validateAuthParams, addIdentityProviderType, - iff((context: HookContext) => !context.existingUser, createNewUser), - (context: HookContext) => (context.data.userId = context.existingUser!.id) + iff((context: HookContext) => !context.existingUser, createNewUser), + (context: HookContext) => + ((context.data as IdentityProviderData).userId = context.existingUser!.id) ], update: [iff(isProvider('external'), checkIdentityProvider)], patch: [ From 550db6e4d8f16c4993cae1c26d1515d7effb9190 Mon Sep 17 00:00:00 2001 From: Kyle Baran Date: Mon, 16 Oct 2023 14:57:15 -0700 Subject: [PATCH 08/10] Replaced internal method calls Changed order of identity-provider create hooks. validateAuthParams needs to occur after createNewUser, or else it won't have a userId to act on. --- packages/client-core/src/user/services/AuthService.ts | 2 +- packages/instanceserver/src/channels.ts | 2 +- packages/server-core/src/projects/project/project-helper.ts | 2 +- packages/server-core/src/social/invite/invite.class.ts | 2 +- packages/server-core/src/social/invite/invite.test.ts | 2 +- .../src/user/identity-provider/identity-provider.hooks.ts | 6 +++--- packages/server-core/src/user/login/login.class.ts | 2 +- packages/server-core/src/user/strategies/discord.ts | 6 +++--- packages/server-core/src/user/strategies/facebook.ts | 6 +++--- packages/server-core/src/user/strategies/google.ts | 6 +++--- packages/server-core/src/user/strategies/linkedin.ts | 6 +++--- packages/server-core/src/user/strategies/twitter.ts | 6 +++--- 12 files changed, 24 insertions(+), 24 deletions(-) diff --git a/packages/client-core/src/user/services/AuthService.ts b/packages/client-core/src/user/services/AuthService.ts index 7e4a4eb26e..b377e9c769 100755 --- a/packages/client-core/src/user/services/AuthService.ts +++ b/packages/client-core/src/user/services/AuthService.ts @@ -581,7 +581,7 @@ export const AuthService = { async removeConnection(identityProviderId: number, userId: UserID) { getMutableState(AuthState).merge({ isProcessing: true, error: '' }) try { - await Engine.instance.api.service(identityProviderPath)._remove(identityProviderId) + await Engine.instance.api.service(identityProviderPath).remove(identityProviderId) return AuthService.loadUserData(userId) } catch (err) { NotificationService.dispatchNotify(err.message, { variant: 'error' }) diff --git a/packages/instanceserver/src/channels.ts b/packages/instanceserver/src/channels.ts index 004570bbb6..3e921ef50c 100755 --- a/packages/instanceserver/src/channels.ts +++ b/packages/instanceserver/src/channels.ts @@ -662,7 +662,7 @@ const onDisconnection = (app: Application) => async (connection: PrimusConnectio } catch (err) { if (err.code === 401 && err.data.name === 'TokenExpiredError') { const jwtDecoded = decode(token)! - const idProvider = await app.service(identityProviderPath)._get(jwtDecoded.sub as string) + const idProvider = await app.service(identityProviderPath).get(jwtDecoded.sub as string) authResult = { [identityProviderPath]: idProvider } diff --git a/packages/server-core/src/projects/project/project-helper.ts b/packages/server-core/src/projects/project/project-helper.ts index fec5d5cab9..188f29114f 100644 --- a/packages/server-core/src/projects/project/project-helper.ts +++ b/packages/server-core/src/projects/project/project-helper.ts @@ -1446,7 +1446,7 @@ export const updateProject = async ( const userId = params!.user?.id || project?.updateUserId if (!userId) throw new BadRequest('No user ID from call or existing project owner') - const githubIdentityProvider = (await app.service(identityProviderPath)._find({ + const githubIdentityProvider = (await app.service(identityProviderPath).find({ query: { userId: userId, type: 'github', diff --git a/packages/server-core/src/social/invite/invite.class.ts b/packages/server-core/src/social/invite/invite.class.ts index 47a459c8f1..b2ac9c80bf 100755 --- a/packages/server-core/src/social/invite/invite.class.ts +++ b/packages/server-core/src/social/invite/invite.class.ts @@ -84,7 +84,7 @@ const afterInviteFind = async (app: Application, result: Paginated) } export const inviteReceived = async (app: Application, query) => { - const identityProviders = (await app.service(identityProviderPath)._find({ + const identityProviders = (await app.service(identityProviderPath).find({ query: { userId: query.userId } diff --git a/packages/server-core/src/social/invite/invite.test.ts b/packages/server-core/src/social/invite/invite.test.ts index ce5ee6725f..f17112bef5 100755 --- a/packages/server-core/src/social/invite/invite.test.ts +++ b/packages/server-core/src/social/invite/invite.test.ts @@ -64,7 +64,7 @@ describe.skip('invite service', () => { after(async () => { // Remove test user - await app.service(identityProviderPath)._remove(null, { + await app.service(identityProviderPath).remove(null, { query: { userId: user.userId } diff --git a/packages/server-core/src/user/identity-provider/identity-provider.hooks.ts b/packages/server-core/src/user/identity-provider/identity-provider.hooks.ts index b3209ab95e..503fbdd4d2 100755 --- a/packages/server-core/src/user/identity-provider/identity-provider.hooks.ts +++ b/packages/server-core/src/user/identity-provider/identity-provider.hooks.ts @@ -194,11 +194,11 @@ export default { create: [ () => schemaHooks.validateData(identityProviderDataValidator), schemaHooks.resolveData(identityProviderDataResolver), - validateAuthParams, - addIdentityProviderType, iff((context: HookContext) => !context.existingUser, createNewUser), (context: HookContext) => - ((context.data as IdentityProviderData).userId = context.existingUser!.id) + ((context.data as IdentityProviderData).userId = context.existingUser!.id), + validateAuthParams, + addIdentityProviderType ], update: [iff(isProvider('external'), checkIdentityProvider)], patch: [ diff --git a/packages/server-core/src/user/login/login.class.ts b/packages/server-core/src/user/login/login.class.ts index 84523fab3c..01ebbc907e 100755 --- a/packages/server-core/src/user/login/login.class.ts +++ b/packages/server-core/src/user/login/login.class.ts @@ -78,7 +78,7 @@ export class LoginService implements ServiceInterface { logger.info('Login Token has expired') return { error: 'Login link has expired' } } - const identityProvider = await this.app.service(identityProviderPath)._get(result.data[0].identityProviderId) + const identityProvider = await this.app.service(identityProviderPath).get(result.data[0].identityProviderId) await makeInitialAdmin(this.app, identityProvider.userId) const apiKey = (await this.app.service(userApiKeyPath).find({ query: { diff --git a/packages/server-core/src/user/strategies/discord.ts b/packages/server-core/src/user/strategies/discord.ts index 815a61e47a..6636b14c4b 100755 --- a/packages/server-core/src/user/strategies/discord.ts +++ b/packages/server-core/src/user/strategies/discord.ts @@ -79,7 +79,7 @@ export class DiscordStrategy extends CustomOAuthStrategy { scopes: [] }) entity.userId = newUser.id - await this.app.service(identityProviderPath)._patch(entity.id, { + await this.app.service(identityProviderPath).patch(entity.id, { userId: newUser.id }) } @@ -100,7 +100,7 @@ export class DiscordStrategy extends CustomOAuthStrategy { userId: entity.userId }) if (entity.type !== 'guest' && identityProvider.type === 'guest') { - await this.app.service(identityProviderPath)._remove(identityProvider.id) + await this.app.service(identityProviderPath).remove(identityProvider.id) await this.app.service(userPath).remove(identityProvider.userId) return super.updateEntity(entity, profile, params) } @@ -108,7 +108,7 @@ export class DiscordStrategy extends CustomOAuthStrategy { if (!existingEntity) { profile.userId = user.id const newIP = await super.createEntity(profile, params) - if (entity.type === 'guest') await this.app.service(identityProviderPath)._remove(entity.id) + if (entity.type === 'guest') await this.app.service(identityProviderPath).remove(entity.id) return newIP } else if (existingEntity.userId === identityProvider.userId) return existingEntity else { diff --git a/packages/server-core/src/user/strategies/facebook.ts b/packages/server-core/src/user/strategies/facebook.ts index b151640306..e449d89b5d 100755 --- a/packages/server-core/src/user/strategies/facebook.ts +++ b/packages/server-core/src/user/strategies/facebook.ts @@ -79,7 +79,7 @@ export class FacebookStrategy extends CustomOAuthStrategy { scopes: [] }) entity.userId = newUser.id - await this.app.service(identityProviderPath)._patch(entity.id, { + await this.app.service(identityProviderPath).patch(entity.id, { userId: newUser.id }) } @@ -100,7 +100,7 @@ export class FacebookStrategy extends CustomOAuthStrategy { userId: entity.userId }) if (entity.type !== 'guest' && identityProvider.type === 'guest') { - await this.app.service(identityProviderPath)._remove(identityProvider.id) + await this.app.service(identityProviderPath).remove(identityProvider.id) await this.app.service(userPath).remove(identityProvider.userId) return super.updateEntity(entity, profile, params) } @@ -108,7 +108,7 @@ export class FacebookStrategy extends CustomOAuthStrategy { if (!existingEntity) { profile.userId = user.id const newIP = await super.createEntity(profile, params) - if (entity.type === 'guest') await this.app.service(identityProviderPath)._remove(entity.id) + if (entity.type === 'guest') await this.app.service(identityProviderPath).remove(entity.id) return newIP } else if (existingEntity.userId === identityProvider.userId) return existingEntity else { diff --git a/packages/server-core/src/user/strategies/google.ts b/packages/server-core/src/user/strategies/google.ts index 01717ee551..8b69ab93e3 100755 --- a/packages/server-core/src/user/strategies/google.ts +++ b/packages/server-core/src/user/strategies/google.ts @@ -79,7 +79,7 @@ export class Googlestrategy extends CustomOAuthStrategy { scopes: [] }) entity.userId = newUser.id - await this.app.service(identityProviderPath)._patch(entity.id, { + await this.app.service(identityProviderPath).patch(entity.id, { userId: newUser.id }) } @@ -100,7 +100,7 @@ export class Googlestrategy extends CustomOAuthStrategy { userId: entity.userId }) if (entity.type !== 'guest' && identityProvider.type === 'guest') { - await this.app.service(identityProviderPath)._remove(identityProvider.id) + await this.app.service(identityProviderPath).remove(identityProvider.id) await this.app.service(userPath).remove(identityProvider.userId) return super.updateEntity(entity, profile, params) } @@ -108,7 +108,7 @@ export class Googlestrategy extends CustomOAuthStrategy { if (!existingEntity) { profile.userId = user.id const newIP = await super.createEntity(profile, params) - if (entity.type === 'guest') await this.app.service(identityProviderPath)._remove(entity.id) + if (entity.type === 'guest') await this.app.service(identityProviderPath).remove(entity.id) return newIP } else if (existingEntity.userId === identityProvider.userId) return existingEntity else { diff --git a/packages/server-core/src/user/strategies/linkedin.ts b/packages/server-core/src/user/strategies/linkedin.ts index 230717b202..4820a39fae 100755 --- a/packages/server-core/src/user/strategies/linkedin.ts +++ b/packages/server-core/src/user/strategies/linkedin.ts @@ -79,7 +79,7 @@ export class LinkedInStrategy extends CustomOAuthStrategy { scopes: [] }) entity.userId = newUser.id - await this.app.service(identityProviderPath)._patch(entity.id, { + await this.app.service(identityProviderPath).patch(entity.id, { userId: newUser.id }) } @@ -100,7 +100,7 @@ export class LinkedInStrategy extends CustomOAuthStrategy { userId: entity.userId }) if (entity.type !== 'guest' && identityProvider.type === 'guest') { - await this.app.service(identityProviderPath)._remove(identityProvider.id) + await this.app.service(identityProviderPath).remove(identityProvider.id) await this.app.service(userPath).remove(identityProvider.userId) return super.updateEntity(entity, profile, params) } @@ -108,7 +108,7 @@ export class LinkedInStrategy extends CustomOAuthStrategy { if (!existingEntity) { profile.userId = user.id const newIP = await super.createEntity(profile, params) - if (entity.type === 'guest') await this.app.service(identityProviderPath)._remove(entity.id) + if (entity.type === 'guest') await this.app.service(identityProviderPath).remove(entity.id) return newIP } else if (existingEntity.userId === identityProvider.userId) return existingEntity else { diff --git a/packages/server-core/src/user/strategies/twitter.ts b/packages/server-core/src/user/strategies/twitter.ts index eeda1a8042..85ef4e4f77 100755 --- a/packages/server-core/src/user/strategies/twitter.ts +++ b/packages/server-core/src/user/strategies/twitter.ts @@ -80,7 +80,7 @@ export class TwitterStrategy extends CustomOAuthStrategy { scopes: [] }) entity.userId = newUser.id - await this.app.service(identityProviderPath)._patch(entity.id, { + await this.app.service(identityProviderPath).patch(entity.id, { userId: newUser.id }) } @@ -101,7 +101,7 @@ export class TwitterStrategy extends CustomOAuthStrategy { userId: entity.userId }) if (entity.type !== 'guest' && identityProvider.type === 'guest') { - await this.app.service(identityProviderPath)._remove(identityProvider.id) + await this.app.service(identityProviderPath).remove(identityProvider.id) await this.app.service(userPath).remove(identityProvider.userId) return super.updateEntity(entity, profile, params) } @@ -109,7 +109,7 @@ export class TwitterStrategy extends CustomOAuthStrategy { if (!existingEntity) { profile.userId = user.id const newIP = await super.createEntity(profile, params) - if (entity.type === 'guest') await this.app.service(identityProviderPath)._remove(entity.id) + if (entity.type === 'guest') await this.app.service(identityProviderPath).remove(entity.id) return newIP } else if (existingEntity.userId === identityProvider.userId) return existingEntity else { From b8252e91d6041a1ebd5f5fc9e747c59fa2f5aced Mon Sep 17 00:00:00 2001 From: Kyle Baran Date: Mon, 16 Oct 2023 18:16:23 -0700 Subject: [PATCH 09/10] Reversed rearrangement, changed tests --- .../identity-provider/identity-provider.hooks.ts | 12 +++++------- .../user/identity-provider/identity-provider.test.ts | 6 +++--- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/packages/server-core/src/user/identity-provider/identity-provider.hooks.ts b/packages/server-core/src/user/identity-provider/identity-provider.hooks.ts index 503fbdd4d2..770e6a5d06 100755 --- a/packages/server-core/src/user/identity-provider/identity-provider.hooks.ts +++ b/packages/server-core/src/user/identity-provider/identity-provider.hooks.ts @@ -121,9 +121,7 @@ async function validateAuthParams(context: HookContext) throw new BadRequest('userId not found') } - const user = await context.app.service(userPath).get(userId) - - context.existingUser = user + context.existingUser = await context.app.service(userPath).get(userId) } async function addIdentityProviderType(context: HookContext) { @@ -189,16 +187,16 @@ export default { () => schemaHooks.validateQuery(identityProviderQueryValidator), schemaHooks.resolveQuery(identityProviderQueryResolver) ], - find: [setLoggedinUserInQuery('userId')], + find: [iff(isProvider('external'), setLoggedinUserInQuery('userId'))], get: [iff(isProvider('external'), checkIdentityProvider)], create: [ () => schemaHooks.validateData(identityProviderDataValidator), schemaHooks.resolveData(identityProviderDataResolver), + validateAuthParams, + addIdentityProviderType, iff((context: HookContext) => !context.existingUser, createNewUser), (context: HookContext) => - ((context.data as IdentityProviderData).userId = context.existingUser!.id), - validateAuthParams, - addIdentityProviderType + ((context.data as IdentityProviderData).userId = context.existingUser!.id) ], update: [iff(isProvider('external'), checkIdentityProvider)], patch: [ diff --git a/packages/server-core/src/user/identity-provider/identity-provider.test.ts b/packages/server-core/src/user/identity-provider/identity-provider.test.ts index 1b2f20e466..9abc154d1b 100755 --- a/packages/server-core/src/user/identity-provider/identity-provider.test.ts +++ b/packages/server-core/src/user/identity-provider/identity-provider.test.ts @@ -69,7 +69,7 @@ describe('identity-provider.service', () => { assert.equal(createdIdentityProvider.type, type) assert.equal(createdIdentityProvider.token, token) assert.ok(createdIdentityProvider.accessToken) - assert.ok(createdIdentityProvider.userId) + assert.equal(createdIdentityProvider.userId, userId) }) it('should create an identity provider for email', async () => { @@ -87,7 +87,7 @@ describe('identity-provider.service', () => { assert.equal(createdIdentityProvider.type, type) assert.equal(createdIdentityProvider.token, token) assert.ok(createdIdentityProvider.accessToken) - assert.ok(createdIdentityProvider.userId) + assert.equal(createdIdentityProvider.userId, userId) }) it('should create an identity provider for password', async () => { @@ -105,7 +105,7 @@ describe('identity-provider.service', () => { assert.equal(createdIdentityProvider.type, type) assert.equal(createdIdentityProvider.token, token) assert.ok(createdIdentityProvider.accessToken) - assert.ok(createdIdentityProvider.userId) + assert.equal(createdIdentityProvider.userId, userId) }) it('should find identity providers', async () => { From 7025cc6ad0f0eb2a1ef65a56671a4f426d4a04db Mon Sep 17 00:00:00 2001 From: aditya-mitra <55396651+aditya-mitra@users.noreply.github.com> Date: Wed, 18 Oct 2023 12:11:24 +0530 Subject: [PATCH 10/10] fix: allow guest to be created using idenity provider --- .../identity-provider.hooks.ts | 19 ++++++++++++------- .../identity-provider.test.ts | 3 +-- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/packages/server-core/src/user/identity-provider/identity-provider.hooks.ts b/packages/server-core/src/user/identity-provider/identity-provider.hooks.ts index 770e6a5d06..c97baed91a 100755 --- a/packages/server-core/src/user/identity-provider/identity-provider.hooks.ts +++ b/packages/server-core/src/user/identity-provider/identity-provider.hooks.ts @@ -100,14 +100,10 @@ async function checkOnlyIdentityProvider(context: HookContext) { - if (Array.isArray(context.data)) { - throw new MethodNotAllowed('identity-provider create works only with singular entries') - } - - let userId = context.data!.userId + let userId = (context.data as IdentityProviderData).userId if (context.params.authentication) { const authResult = await context.app.service('authentication').strategies.jwt.authenticate!( @@ -118,6 +114,9 @@ async function validateAuthParams(context: HookContext) } if (!userId) { + if ((context.data as IdentityProviderData).type === 'guest') { + return + } throw new BadRequest('userId not found') } @@ -157,7 +156,7 @@ async function createNewUser(context: HookContext) { context.existingUser = newUser } -/** (AFTER) CREATE HOOKS **/ +/* (AFTER) CREATE HOOKS */ async function addScopes(context: HookContext) { if (isDev && (context.data as IdentityProviderType).type === 'admin') { @@ -190,6 +189,12 @@ export default { find: [iff(isProvider('external'), setLoggedinUserInQuery('userId'))], get: [iff(isProvider('external'), checkIdentityProvider)], create: [ + iff( + (context: HookContext) => Array.isArray(context.data), + () => { + throw new MethodNotAllowed('identity-provider create works only with singular entries') + } + ), () => schemaHooks.validateData(identityProviderDataValidator), schemaHooks.resolveData(identityProviderDataResolver), validateAuthParams, diff --git a/packages/server-core/src/user/identity-provider/identity-provider.test.ts b/packages/server-core/src/user/identity-provider/identity-provider.test.ts index 9abc154d1b..56e9287be0 100755 --- a/packages/server-core/src/user/identity-provider/identity-provider.test.ts +++ b/packages/server-core/src/user/identity-provider/identity-provider.test.ts @@ -37,9 +37,8 @@ import { UserID } from '@etherealengine/engine/src/schemas/user/user.schema' import { Application } from '../../../declarations' import { createFeathersKoaApp } from '../../createApp' -let userId: UserID - describe('identity-provider.service', () => { + let userId: UserID let app: Application let providers: IdentityProviderType[] = []