From 3703d24cb67444e1586d9339eeceb78af4fbb9d5 Mon Sep 17 00:00:00 2001 From: Michael Uray Date: Sun, 18 Oct 2026 03:48:28 +0000 Subject: [PATCH] feat(auth): sync avatars from OIDC/OAuth providers The OIDC, Google and GitHub login handlers never read the avatar the identity provider returns (the OIDC picture claim, the passport profile photos), and the global person in the account database had no place to keep it, so workspace persons were always created with a plain color avatar. The provider handler now extracts the avatar URL from the profile and passes it to loginOrSignUpWithProvider/joinWithProvider, which store it on the global person (new nullable person.avatar_url column, account DB migration v28). Only absolute http(s) URLs without credentials and of reasonable length are accepted (normalizeExternalAvatarUrl in core); the URL is never fetched by the server. When a user connects to a workspace, ensureEmployee uses that URL as an external avatar for a newly created person, and refreshes it for an existing person whose avatar is still the default color avatar or a previously synced external one. Uploaded images, Gravatar and an explicitly chosen color are never overwritten. External avatars did not render at all so far: getAvatarProviderId() mapped AvatarType.EXTERNAL to the Image provider, which only reads the stored blob, and the model registered the External provider under the Color provider id. EXTERNAL now has its own provider id (contact.avatarProvider.External) that resolves to GetExternalUrl, which also makes existing GitHub-synced avatars visible. Push notifications use the external URL as sender icon as well. Stored external URLs are re-checked when rendered (getExternalAvatarUrl), so only http(s) URLs are ever used as an image source. The provider avatar URL may contain signed query parameters, so it is not part of the profile returned by getUserProfile; only the person's own client reads it through getPerson to sync the workspace avatar. Fixes #8120 Fixes #9026 Signed-off-by: Michael Uray --- .../src/__tests__/externalAvatarUrl.test.ts | 44 +++++++++ foundations/core/packages/core/src/classes.ts | 2 + foundations/core/packages/core/src/utils.ts | 25 ++++++ models/contact/src/index.ts | 2 +- plugins/contact-resources/src/index.ts | 3 +- .../src/__tests__/providerAvatar.test.ts | 90 +++++++++++++++++++ plugins/contact/src/index.ts | 3 +- plugins/contact/src/utils.ts | 49 +++++++++- .../src/__tests__/providerAvatar.test.ts | 48 ++++++++++ pods/authProviders/src/utils.ts | 30 ++++++- .../notification-resources/src/push.ts | 3 + .../account/src/__tests__/operations.test.ts | 42 +++++++++ server/account/src/__tests__/utils.test.ts | 63 +++++++++++++ .../src/collections/postgres/migrations.ts | 15 +++- server/account/src/operations.ts | 7 +- server/account/src/utils.ts | 17 +++- 16 files changed, 430 insertions(+), 13 deletions(-) create mode 100644 foundations/core/packages/core/src/__tests__/externalAvatarUrl.test.ts create mode 100644 plugins/contact/src/__tests__/providerAvatar.test.ts create mode 100644 pods/authProviders/src/__tests__/providerAvatar.test.ts diff --git a/foundations/core/packages/core/src/__tests__/externalAvatarUrl.test.ts b/foundations/core/packages/core/src/__tests__/externalAvatarUrl.test.ts new file mode 100644 index 00000000000..8020c058bdf --- /dev/null +++ b/foundations/core/packages/core/src/__tests__/externalAvatarUrl.test.ts @@ -0,0 +1,44 @@ +// +// Copyright © 2026 Hardcore Engineering Inc. +// +// Licensed under the Eclipse Public License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. You may +// obtain a copy of the License at https://www.eclipse.org/legal/epl-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// +// See the License for the specific language governing permissions and +// limitations under the License. +// + +import { normalizeExternalAvatarUrl } from '../utils' + +describe('normalizeExternalAvatarUrl', () => { + it('accepts absolute http(s) URLs', () => { + expect(normalizeExternalAvatarUrl('https://idp.example.com/avatar.png')).toBe('https://idp.example.com/avatar.png') + expect(normalizeExternalAvatarUrl(' http://idp.example.com/a?s=96 ')).toBe('http://idp.example.com/a?s=96') + }) + + it('rejects non-string and empty values', () => { + expect(normalizeExternalAvatarUrl(undefined)).toBeUndefined() + expect(normalizeExternalAvatarUrl(null)).toBeUndefined() + expect(normalizeExternalAvatarUrl(42)).toBeUndefined() + expect(normalizeExternalAvatarUrl({ url: 'https://idp.example.com/a.png' })).toBeUndefined() + expect(normalizeExternalAvatarUrl(' ')).toBeUndefined() + }) + + it('rejects relative URLs and other protocols', () => { + expect(normalizeExternalAvatarUrl('/avatar.png')).toBeUndefined() + expect(normalizeExternalAvatarUrl('javascript:alert(1)')).toBeUndefined() + expect(normalizeExternalAvatarUrl('data:image/png;base64,AAAA')).toBeUndefined() + expect(normalizeExternalAvatarUrl('file:///etc/passwd')).toBeUndefined() + expect(normalizeExternalAvatarUrl('ftp://idp.example.com/a.png')).toBeUndefined() + }) + + it('rejects URLs with credentials or excessive length', () => { + expect(normalizeExternalAvatarUrl('https://user:pass@idp.example.com/a.png')).toBeUndefined() + expect(normalizeExternalAvatarUrl('https://idp.example.com/' + 'a'.repeat(3000))).toBeUndefined() + }) +}) diff --git a/foundations/core/packages/core/src/classes.ts b/foundations/core/packages/core/src/classes.ts index 84a9555caa2..fc3cf47774d 100644 --- a/foundations/core/packages/core/src/classes.ts +++ b/foundations/core/packages/core/src/classes.ts @@ -639,6 +639,8 @@ export interface Person { uuid: PersonUuid firstName: string lastName: string + /** External avatar URL provided by an identity provider (OIDC/OAuth), if any. */ + avatarUrl?: string } export interface PersonInfo extends BasePerson { diff --git a/foundations/core/packages/core/src/utils.ts b/foundations/core/packages/core/src/utils.ts index 568c34a415e..bed800ff035 100644 --- a/foundations/core/packages/core/src/utils.ts +++ b/foundations/core/packages/core/src/utils.ts @@ -1018,3 +1018,28 @@ export function toRank (str: string | undefined): Rank | undefined { export function escapeRegExp (value: string): string { return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') } + +const MAX_EXTERNAL_AVATAR_URL_LENGTH = 2048 + +/** + * Returns a normalized http(s) URL suitable for an external avatar, or undefined + * when the value is not a string, not an absolute http(s) URL, carries credentials + * or is unreasonably long. The URL is never fetched here. + */ +export function normalizeExternalAvatarUrl (value: unknown): string | undefined { + if (typeof value !== 'string') return undefined + const trimmed = value.trim() + if (trimmed === '' || trimmed.length > MAX_EXTERNAL_AVATAR_URL_LENGTH) return undefined + + let url: URL + try { + url = new URL(trimmed) + } catch { + return undefined + } + if (url.protocol !== 'https:' && url.protocol !== 'http:') return undefined + if (url.username !== '' || url.password !== '') return undefined + + const normalized = url.toString() + return normalized.length > MAX_EXTERNAL_AVATAR_URL_LENGTH ? undefined : normalized +} diff --git a/models/contact/src/index.ts b/models/contact/src/index.ts index e4736288baa..0c8fbc56462 100644 --- a/models/contact/src/index.ts +++ b/models/contact/src/index.ts @@ -914,7 +914,7 @@ export function createModel (builder: Builder): void { type: AvatarType.EXTERNAL, getUrl: contact.function.GetExternalUrl }, - contact.avatarProvider.Color + contact.avatarProvider.External ) builder.createDoc( diff --git a/plugins/contact-resources/src/index.ts b/plugins/contact-resources/src/index.ts index b7e2193834f..06153fc31d2 100644 --- a/plugins/contact-resources/src/index.ts +++ b/plugins/contact-resources/src/index.ts @@ -15,6 +15,7 @@ // /* eslint-disable @typescript-eslint/no-unused-vars */ import { + getExternalAvatarUrl, getGravatarUrl, getName, type AvatarInfo, @@ -458,7 +459,7 @@ export default async (): Promise => ({ }), GetExternalUrl: async (person: Data>, name: string) => ({ color: getPersonColor(person, name), - url: person.avatarProps?.url + url: getExternalAvatarUrl(person) }), EmployeeSort: employeeSort, FilterChannelInResult: filterChannelInResult, diff --git a/plugins/contact/src/__tests__/providerAvatar.test.ts b/plugins/contact/src/__tests__/providerAvatar.test.ts new file mode 100644 index 00000000000..937b8244565 --- /dev/null +++ b/plugins/contact/src/__tests__/providerAvatar.test.ts @@ -0,0 +1,90 @@ +// +// Copyright © 2026 Hardcore Engineering Inc. +// +// Licensed under the Eclipse Public License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. You may +// obtain a copy of the License at https://www.eclipse.org/legal/epl-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// +// See the License for the specific language governing permissions and +// limitations under the License. +// + +import contact, { AvatarType } from '..' +import { getAvatarProviderId, getExternalAvatarUrl, getProviderAvatarUpdate } from '../utils' + +const url = 'https://idp.example.com/avatar.png' +const external = { avatarType: AvatarType.EXTERNAL, avatarProps: { url } } + +describe('getProviderAvatarUpdate', () => { + it('uses the provider avatar for a new person', () => { + expect(getProviderAvatarUpdate(undefined, url)).toEqual(external) + }) + + it('ignores missing or non-http avatar urls', () => { + expect(getProviderAvatarUpdate(undefined, undefined)).toBeUndefined() + expect(getProviderAvatarUpdate(undefined, 'javascript:alert(1)')).toBeUndefined() + expect(getProviderAvatarUpdate({ avatarType: AvatarType.COLOR }, 'ftp://idp.example.com/a.png')).toBeUndefined() + }) + + it('replaces the default color avatar', () => { + expect(getProviderAvatarUpdate({ avatarType: AvatarType.COLOR }, url)).toEqual(external) + expect(getProviderAvatarUpdate({ avatarType: AvatarType.COLOR, avatar: null, avatarProps: {} }, url)).toEqual( + external + ) + }) + + it('refreshes a previously synced external avatar', () => { + expect( + getProviderAvatarUpdate( + { avatarType: AvatarType.EXTERNAL, avatarProps: { url: 'https://old.example.com/a' } }, + url + ) + ).toEqual(external) + expect(getProviderAvatarUpdate(external, url)).toBeUndefined() + }) + + it('keeps an avatar chosen by the user', () => { + expect( + getProviderAvatarUpdate({ avatarType: AvatarType.COLOR, avatarProps: { color: 'Sky' } }, url) + ).toBeUndefined() + expect(getProviderAvatarUpdate({ avatarType: AvatarType.IMAGE, avatar: 'blob' as any }, url)).toBeUndefined() + expect( + getProviderAvatarUpdate({ avatarType: AvatarType.GRAVATAR, avatarProps: { url: 'gravatar-id' } }, url) + ).toBeUndefined() + }) +}) + +describe('getAvatarProviderId', () => { + it('resolves every avatar type to its own provider', () => { + expect(getAvatarProviderId(AvatarType.COLOR)).toBe(contact.avatarProvider.Color) + expect(getAvatarProviderId(AvatarType.IMAGE)).toBe(contact.avatarProvider.Image) + expect(getAvatarProviderId(AvatarType.GRAVATAR)).toBe(contact.avatarProvider.Gravatar) + expect(getAvatarProviderId(AvatarType.EXTERNAL)).toBe(contact.avatarProvider.External) + }) + + it('uses distinct provider ids', () => { + const ids = Object.values(AvatarType).map((type) => getAvatarProviderId(type)) + expect(new Set(ids).size).toBe(ids.length) + }) +}) + +describe('getExternalAvatarUrl', () => { + it('returns http(s) avatar urls', () => { + expect(getExternalAvatarUrl({ avatarProps: { url } })).toBe(url) + expect(getExternalAvatarUrl({ avatarProps: { url: 'http://avatars.example.com/u/1?v=4' } })).toBe( + 'http://avatars.example.com/u/1?v=4' + ) + }) + + it('never returns unsafe or missing urls', () => { + expect(getExternalAvatarUrl({})).toBeUndefined() + expect(getExternalAvatarUrl({ avatarProps: {} })).toBeUndefined() + expect(getExternalAvatarUrl({ avatarProps: { url: 'javascript:alert(1)' } })).toBeUndefined() + expect(getExternalAvatarUrl({ avatarProps: { url: 'data:image/svg+xml;base64,AAAA' } })).toBeUndefined() + expect(getExternalAvatarUrl({ avatarProps: { url: '/files/avatar.png' } })).toBeUndefined() + }) +}) diff --git a/plugins/contact/src/index.ts b/plugins/contact/src/index.ts index b600532a804..812dd77bcab 100644 --- a/plugins/contact/src/index.ts +++ b/plugins/contact/src/index.ts @@ -305,7 +305,8 @@ export const contactPlugin = plugin(contactId, { avatarProvider: { Color: '' as Ref, Image: '' as Ref, - Gravatar: '' as Ref + Gravatar: '' as Ref, + External: '' as Ref }, function: { GetColorUrl: '' as Resource, diff --git a/plugins/contact/src/utils.ts b/plugins/contact/src/utils.ts index eceb5ae392a..f37f45c3da1 100644 --- a/plugins/contact/src/utils.ts +++ b/plugins/contact/src/utils.ts @@ -26,6 +26,7 @@ import { generateId, Hierarchy, MeasureContext, + normalizeExternalAvatarUrl, notEmpty, Person as GlobalPerson, PersonId, @@ -39,6 +40,7 @@ import { import platform, { getMetadata, PlatformError } from '@hcengineering/platform' import { ColorDefinition } from '@hcengineering/ui' import contact, { + AvatarInfo, AvatarProvider, AvatarType, Channel, @@ -137,6 +139,8 @@ export function getAvatarProviderId (kind: AvatarType): Ref | un return contact.avatarProvider.Gravatar case AvatarType.COLOR: return contact.avatarProvider.Color + case AvatarType.EXTERNAL: + return contact.avatarProvider.External } return contact.avatarProvider.Image } @@ -415,6 +419,40 @@ export async function getAllUserAccounts (client: Client): Promise it.personUuid).filter(notEmpty) } +/** + * Returns the image URL of an external avatar, or undefined when the stored value is not + * an absolute http(s) URL (so e.g. javascript: or data: values are never used as image source). + */ +export function getExternalAvatarUrl (person: Pick): string | undefined { + return normalizeExternalAvatarUrl(person.avatarProps?.url) +} + +/** + * Returns the avatar fields to apply for an avatar URL provided by an identity provider + * (OIDC/OAuth), or undefined when nothing should change. Only the default color avatar and a + * previously synced external avatar are replaced; an uploaded image, Gravatar or an explicitly + * chosen color are kept. Pass undefined as person for a person that is about to be created. + */ +export function getProviderAvatarUpdate ( + person: Pick | undefined, + avatarUrl: string | undefined +): Pick | undefined { + const url = normalizeExternalAvatarUrl(avatarUrl) + if (url === undefined) return undefined + const update = { avatarType: AvatarType.EXTERNAL, avatarProps: { url } } + if (person === undefined) return update + + if (person.avatarType === AvatarType.EXTERNAL) { + return person.avatarProps?.url === url ? undefined : update + } + + const isDefaultColor = + (person.avatarType ?? AvatarType.COLOR) === AvatarType.COLOR && + person.avatar == null && + (person.avatarProps?.color ?? '') === '' + return isDefaultColor ? update : undefined +} + export async function ensureEmployee ( ctx: MeasureContext, me: Account, @@ -462,7 +500,8 @@ export async function ensureEmployeeForPerson ( personUuid: person.uuid, name: combineName(globalPerson.firstName, globalPerson.lastName), city: '', - avatarType: AvatarType.COLOR + avatarType: AvatarType.COLOR, + ...getProviderAvatarUpdate(undefined, globalPerson.avatarUrl) } personRef = generateId() @@ -480,6 +519,14 @@ export async function ensureEmployeeForPerson ( personUuid: person.uuid }) await client.tx(updatePersonTx) + } else { + // Keep the avatar in sync with the identity provider unless the user has chosen one + const avatarUpdate = getProviderAvatarUpdate(personByUuid, globalPerson?.avatarUrl) + if (avatarUpdate !== undefined) { + await client.tx( + txFactory.createTxUpdateDoc(contact.class.Person, contact.space.Contacts, personByUuid._id, avatarUpdate) + ) + } } const existingIdentifiers = toIdMap( diff --git a/pods/authProviders/src/__tests__/providerAvatar.test.ts b/pods/authProviders/src/__tests__/providerAvatar.test.ts new file mode 100644 index 00000000000..c5fa97b2cb3 --- /dev/null +++ b/pods/authProviders/src/__tests__/providerAvatar.test.ts @@ -0,0 +1,48 @@ +// +// Copyright © 2026 Hardcore Engineering Inc. +// +// Licensed under the Eclipse Public License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. You may +// obtain a copy of the License at https://www.eclipse.org/legal/epl-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// +// See the License for the specific language governing permissions and +// limitations under the License. +// + +import { getProviderAvatarUrl } from '../utils' + +describe('getProviderAvatarUrl', () => { + test('reads the OIDC picture claim', () => { + expect(getProviderAvatarUrl({ sub: 'user', picture: 'https://idp.example.com/u/1.png' })).toBe( + 'https://idp.example.com/u/1.png' + ) + }) + + test('reads the first passport profile photo (Google, GitHub)', () => { + expect( + getProviderAvatarUrl({ + photos: [{ value: 'https://avatars.example.com/u/1?v=4' }, { value: 'https://other.example.com/2' }] + }) + ).toBe('https://avatars.example.com/u/1?v=4') + }) + + test('falls back to photos when the picture claim is not a usable url', () => { + expect( + getProviderAvatarUrl({ picture: 'javascript:alert(1)', photos: [{ value: 'http://idp.example.com/a' }] }) + ).toBe('http://idp.example.com/a') + }) + + test('returns undefined when there is no usable avatar', () => { + expect(getProviderAvatarUrl(undefined)).toBeUndefined() + expect(getProviderAvatarUrl('user')).toBeUndefined() + expect(getProviderAvatarUrl({})).toBeUndefined() + expect(getProviderAvatarUrl({ picture: '' })).toBeUndefined() + expect(getProviderAvatarUrl({ picture: { url: 'https://idp.example.com/a' } })).toBeUndefined() + expect(getProviderAvatarUrl({ photos: 'https://idp.example.com/a' })).toBeUndefined() + expect(getProviderAvatarUrl({ photos: [{ value: 'data:image/png;base64,AAAA' }] })).toBeUndefined() + }) +}) diff --git a/pods/authProviders/src/utils.ts b/pods/authProviders/src/utils.ts index 32f86c812c9..615a6e70b71 100644 --- a/pods/authProviders/src/utils.ts +++ b/pods/authProviders/src/utils.ts @@ -13,7 +13,14 @@ // limitations under the License. // import { AccountDB, joinWithProvider, LoginInfo, loginOrSignUpWithProvider } from '@hcengineering/account' -import { BrandingMap, concatLink, getBranding, MeasureContext, SocialKey } from '@hcengineering/core' +import { + BrandingMap, + concatLink, + getBranding, + MeasureContext, + normalizeExternalAvatarUrl, + SocialKey +} from '@hcengineering/core' import { IncomingHttpHeaders } from 'http' import qs from 'querystringify' @@ -59,6 +66,20 @@ export function encodeState (ctx: any, brandings: BrandingMap): string { return encodeURIComponent(JSON.stringify(state)) } +/** + * Picks the avatar URL from a provider profile: the OIDC `picture` claim, or the first + * entry of the passport `photos` list (Google, GitHub). Only http(s) URLs are returned. + */ +export function getProviderAvatarUrl (user: any): string | undefined { + if (user == null || typeof user !== 'object') return undefined + const candidates = [user.picture, Array.isArray(user.photos) ? user.photos[0]?.value : undefined] + for (const candidate of candidates) { + const url = normalizeExternalAvatarUrl(candidate) + if (url !== undefined) return url + } + return undefined +} + export async function handleProviderAuth ( measureCtx: MeasureContext, db: AccountDB, @@ -78,6 +99,7 @@ export async function handleProviderAuth ( let loginInfo: LoginInfo | null const state = safeParseAuthState(rawState) const branding = getBranding(brandings, state?.branding) + const avatarUrl = getProviderAvatarUrl(user) if (state.inviteId != null && state.inviteId !== '' && state.autoJoin !== true) { loginInfo = await joinWithProvider( @@ -89,7 +111,8 @@ export async function handleProviderAuth ( last, state.inviteId as any, socialKey, - signUpDisabled + signUpDisabled, + avatarUrl ) } else { loginInfo = await loginOrSignUpWithProvider( @@ -100,7 +123,8 @@ export async function handleProviderAuth ( first, last, socialKey, - signUpDisabled === true || state.autoJoin === true + signUpDisabled === true || state.autoJoin === true, + avatarUrl ) } diff --git a/server-plugins/notification-resources/src/push.ts b/server-plugins/notification-resources/src/push.ts index a1d5890d1d7..43f7072860b 100644 --- a/server-plugins/notification-resources/src/push.ts +++ b/server-plugins/notification-resources/src/push.ts @@ -44,6 +44,7 @@ import { encodeObjectURI } from '@hcengineering/view' import contact, { type AvatarInfo, getAvatarProviderId, + getExternalAvatarUrl, getGravatarUrl, Person, PersonSpace @@ -189,6 +190,8 @@ export async function createPushNotification ( } } else if (provider === contact.avatarProvider.Gravatar && senderAvatar.avatarProps?.url !== undefined) { data.icon = getGravatarUrl(senderAvatar.avatarProps?.url, 512) + } else if (provider === contact.avatarProvider.External) { + data.icon = getExternalAvatarUrl(senderAvatar) } } diff --git a/server/account/src/__tests__/operations.test.ts b/server/account/src/__tests__/operations.test.ts index 67ea04e5544..ea3dd7b3803 100644 --- a/server/account/src/__tests__/operations.test.ts +++ b/server/account/src/__tests__/operations.test.ts @@ -54,6 +54,7 @@ import { requestPasswordReset, changePassword, getPerson, + getUserProfile, getSocialIds, getAccountInfo, createAccessLink, @@ -3247,6 +3248,47 @@ describe('account operations', () => { }) }) + describe('getUserProfile', () => { + const ownerUuid = 'profile-owner-uuid' as PersonUuid + const profileDb = { + person: { + findOne: jest.fn() + }, + userProfile: { + findOne: jest.fn() + } + } as unknown as AccountDB + + beforeEach(() => { + ;(profileDb.person.findOne as jest.Mock).mockResolvedValue({ + uuid: ownerUuid, + firstName: 'John', + lastName: 'Doe', + avatarUrl: 'https://idp.example.com/avatar.png?signature=secret' + }) + ;(profileDb.userProfile.findOne as jest.Mock).mockResolvedValue({ + personUuid: ownerUuid, + bio: 'Hello', + isPublic: true + }) + }) + + test('should not expose the provider avatar url in a public profile', async () => { + const result = await getUserProfile(mockCtx, profileDb, mockBranding, undefined, { personUuid: ownerUuid }) + + expect(result).toEqual({ uuid: ownerUuid, firstName: 'John', lastName: 'Doe', bio: 'Hello', isPublic: true }) + expect(result).not.toHaveProperty('avatarUrl') + }) + + test('should not expose the provider avatar url in the own profile', async () => { + ;(decodeTokenVerbose as jest.Mock).mockReturnValue({ account: ownerUuid }) + + const result = await getUserProfile(mockCtx, profileDb, mockBranding, mockToken, {}) + + expect(result).not.toHaveProperty('avatarUrl') + }) + }) + describe('getAccountInfo', () => { const callerAccount = 'caller-account-uuid' as AccountUuid const otherAccount = 'other-account-uuid' as AccountUuid diff --git a/server/account/src/__tests__/utils.test.ts b/server/account/src/__tests__/utils.test.ts index ea1130854a0..53b7957dc43 100644 --- a/server/account/src/__tests__/utils.test.ts +++ b/server/account/src/__tests__/utils.test.ts @@ -1910,6 +1910,69 @@ describe('account utils', () => { expect(result).toBeNull() }) + + describe('provider avatar url', () => { + const personUuid = 'existing-person' as PersonUuid + + function mockExistingAccount (storedAvatarUrl?: string): void { + ;(mockDb.socialId.findOne as jest.Mock).mockResolvedValue({ + _id: 'social-id', + key: 'google:test-id', + personUuid, + verifiedOn: 1 + }) + ;(mockDb.person.findOne as jest.Mock).mockResolvedValue({ + uuid: personUuid, + firstName: 'John', + lastName: 'Doe', + avatarUrl: storedAvatarUrl + }) + ;(mockDb.account.findOne as jest.Mock).mockResolvedValue({ uuid: personUuid }) + } + + async function login (avatarUrl?: string): Promise { + await loginOrSignUpWithProvider( + mockCtx, + mockDb, + mockBranding, + 'test@example.com', + 'John', + 'Doe', + mockSocialId, + false, + avatarUrl + ) + } + + test('should store an http(s) avatar url from the provider', async () => { + mockExistingAccount() + + await login('https://idp.example.com/avatar.png') + + expect(mockDb.person.update).toHaveBeenCalledWith( + { uuid: personUuid }, + { avatarUrl: 'https://idp.example.com/avatar.png' } + ) + }) + + test('should not update an unchanged avatar url', async () => { + mockExistingAccount('https://idp.example.com/avatar.png') + + await login('https://idp.example.com/avatar.png') + + expect(mockDb.person.update).not.toHaveBeenCalled() + }) + + test('should ignore missing and non-http avatar urls', async () => { + mockExistingAccount('https://idp.example.com/avatar.png') + + await login(undefined) + await login('javascript:alert(1)') + await login('data:image/png;base64,AAAA') + + expect(mockDb.person.update).not.toHaveBeenCalled() + }) + }) }) describe('getWorkspaceInvite', () => { diff --git a/server/account/src/collections/postgres/migrations.ts b/server/account/src/collections/postgres/migrations.ts index ddc74cc1dbc..e593681e857 100644 --- a/server/account/src/collections/postgres/migrations.ts +++ b/server/account/src/collections/postgres/migrations.ts @@ -84,7 +84,8 @@ export function getMigrations (ns: string, flavor: DBFlavor): [string, string][] getV24Migration(ns, flavor), getV25Migration(ns, flavor), getV26Migration(ns, flavor), - getV27Migration(ns, flavor) + getV27Migration(ns, flavor), + getV28Migration(ns, flavor) ] } @@ -840,3 +841,15 @@ function getV27Migration (ns: string, flavor: DBFlavor): [string, string] { ` ] } + +function getV28Migration (ns: string, flavor: DBFlavor): [string, string] { + const types = dbTypes[flavor] + return [ + 'account_db_v28_add_person_avatar_url', + ` + -- External avatar URL provided by an identity provider (OIDC/OAuth) on login. + ALTER TABLE ${ns}.person + ADD COLUMN IF NOT EXISTS avatar_url ${types.string}; + ` + ] +} diff --git a/server/account/src/operations.ts b/server/account/src/operations.ts index 0e8953515d0..cbb97adcac8 100644 --- a/server/account/src/operations.ts +++ b/server/account/src/operations.ts @@ -3285,7 +3285,7 @@ async function setMyProfile ( } } -async function getUserProfile ( +export async function getUserProfile ( ctx: MeasureContext, db: AccountDB, branding: Branding | null, @@ -3331,8 +3331,11 @@ async function getUserProfile ( // 1. Requesting own profile (authenticated user requesting their own) // 2. Profile is public if (requestingAccount === targetPersonUuid || profile.isPublic) { + // The identity provider avatar URL may carry signed query parameters; it is only + // meant for the person's own workspace avatar sync and is never part of a profile. + const { avatarUrl, ...profilePerson } = person const res: PersonWithProfile = { - ...person, + ...profilePerson, ...profile } diff --git a/server/account/src/utils.ts b/server/account/src/utils.ts index 5c5115c1c94..4997d0ea508 100644 --- a/server/account/src/utils.ts +++ b/server/account/src/utils.ts @@ -21,6 +21,7 @@ import { groupByArray, isActiveMode, type MeasureContext, + normalizeExternalAvatarUrl, type Person, type PersonId, type PersonUuid, @@ -1630,7 +1631,8 @@ export async function loginOrSignUpWithProvider ( first: string, last: string, socialId: SocialKey, - signUpDisabled = false + signUpDisabled = false, + avatarUrl?: string ): Promise { try { const normalizedEmail = email != null ? cleanEmail(email) : '' @@ -1676,6 +1678,13 @@ export async function loginOrSignUpWithProvider ( await db.person.update({ uuid: personUuid }, { firstName: first, lastName: last }) } + // Keep the provider avatar URL up to date. It is only stored as a link and never fetched here; + // workspaces decide whether to show it (see ensureEmployee in @hcengineering/contact). + const providerAvatarUrl = normalizeExternalAvatarUrl(avatarUrl) + if (providerAvatarUrl !== undefined && person.avatarUrl !== providerAvatarUrl) { + await db.person.update({ uuid: personUuid }, { avatarUrl: providerAvatarUrl }) + } + // We should check and reset password if there's an account with password but no social ids have been // confirmed yet const confirmedSocialId = await db.socialId.findOne({ personUuid, verifiedOn: { $gt: 0 } }) @@ -1732,7 +1741,8 @@ export async function joinWithProvider ( last: string, inviteId: string, socialId: SocialKey, - signUpDisabled = false + signUpDisabled = false, + avatarUrl?: string ): Promise { const normalizedEmail = email != null ? cleanEmail(email) : '' const invite = await getWorkspaceInvite(db, inviteId) @@ -1755,7 +1765,8 @@ export async function joinWithProvider ( first, last, socialId, - signUpDisabled + signUpDisabled, + avatarUrl ) if (loginInfo == null) {