From bfc857d2469a4ece91b3259b5e90ece4a333432d Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 2 Jul 2026 11:16:49 +0200 Subject: [PATCH] feat(oidc): use the picture claim as avatar when none is uploaded When a user signs in via OIDC and hasn't uploaded a custom avatar, their `picture` claim is now used as their avatar. The users.avatar column holds either an uploaded file name or an absolute https URL from the claim, and a single resolver on each side (server avatarUrl, client avatarSrc) renders both. An uploaded avatar always wins and is never overwritten; the picture refreshes on each login otherwise. Only https URLs are stored, matching the image CSP. All the scattered /uploads/avatars/ builders now go through the resolvers, which also fixes collection member avatars that were rendering a bare file name. Discussion #1399 --- .../src/components/Collab/CollabNotesCard.tsx | 3 +- .../src/components/Collab/WhatsNextWidget.tsx | 3 +- .../components/Collections/CollectionHero.tsx | 3 +- .../Collections/ShareCollectionModal.tsx | 3 +- .../src/components/Planner/DayPlanSidebar.tsx | 3 +- .../src/components/Planner/PlaceInspector.tsx | 5 +- client/src/components/Todo/TodoListPanel.tsx | 5 +- client/src/components/Todo/TodoRow.tsx | 3 +- client/src/pages/SharedTripPage.tsx | 3 +- client/src/utils/avatarSrc.test.ts | 18 ++++++ client/src/utils/avatarSrc.ts | 12 ++++ server/src/nest/oidc/oidc.controller.ts | 6 ++ server/src/services/adminService.ts | 3 +- server/src/services/authService.ts | 7 ++- server/src/services/avatarUrl.ts | 11 +++- server/src/services/fileService.ts | 3 +- server/src/services/inAppNotifications.ts | 9 +-- server/src/services/journeyService.ts | 3 +- server/src/services/oidcService.ts | 28 ++++++++- server/src/services/tripService.ts | 7 ++- server/tests/unit/services/avatarUrl.test.ts | 25 ++++++++ .../tests/unit/services/oidcService.test.ts | 57 +++++++++++++++++++ 22 files changed, 193 insertions(+), 27 deletions(-) create mode 100644 client/src/utils/avatarSrc.test.ts create mode 100644 client/src/utils/avatarSrc.ts create mode 100644 server/tests/unit/services/avatarUrl.test.ts diff --git a/client/src/components/Collab/CollabNotesCard.tsx b/client/src/components/Collab/CollabNotesCard.tsx index 77e9f2fad..e9fae8547 100644 --- a/client/src/components/Collab/CollabNotesCard.tsx +++ b/client/src/components/Collab/CollabNotesCard.tsx @@ -1,4 +1,5 @@ import { useState, useCallback } from 'react' +import { avatarSrc } from '../../utils/avatarSrc' import Markdown from 'react-markdown' import remarkGfm from 'remark-gfm' import remarkBreaks from 'remark-breaks' @@ -28,7 +29,7 @@ interface NoteCardProps { export function NoteCard({ note, currentUser, canEdit, onUpdate, onDelete, onEdit, onView, onPreviewFile, getCategoryColor, tripId, t }: NoteCardProps) { const [hovered, setHovered] = useState(false) - const author = note.author || note.user || { username: note.username, avatar: note.avatar_url || (note.avatar ? `/uploads/avatars/${note.avatar}` : null) } + const author = note.author || note.user || { username: note.username, avatar: note.avatar_url || avatarSrc(note.avatar) } const color = getCategoryColor ? getCategoryColor(note.category) : (note.color || '#6366f1') const handleTogglePin = useCallback(() => { diff --git a/client/src/components/Collab/WhatsNextWidget.tsx b/client/src/components/Collab/WhatsNextWidget.tsx index 9c4ff4588..14a9cfcf6 100644 --- a/client/src/components/Collab/WhatsNextWidget.tsx +++ b/client/src/components/Collab/WhatsNextWidget.tsx @@ -1,4 +1,5 @@ import React, { useMemo } from 'react' +import { avatarSrc } from '../../utils/avatarSrc' import { useTripStore } from '../../store/tripStore' import { useSettingsStore } from '../../store/settingsStore' import { useTranslation } from '../../i18n' @@ -179,7 +180,7 @@ export default function WhatsNextWidget({ tripMembers = [] }: WhatsNextWidgetPro overflow: 'hidden', flexShrink: 0, }}> {p.avatar - ? + ? : p.username?.[0]?.toUpperCase() } diff --git a/client/src/components/Collections/CollectionHero.tsx b/client/src/components/Collections/CollectionHero.tsx index 1bf5fc658..1b43ad188 100644 --- a/client/src/components/Collections/CollectionHero.tsx +++ b/client/src/components/Collections/CollectionHero.tsx @@ -1,4 +1,5 @@ import React from 'react' +import { avatarSrc } from '../../utils/avatarSrc' import { Share2, Users, Link2, Pencil } from 'lucide-react' import type { CollectionMember, CollectionLink } from '@trek/shared' import type { TranslationFn } from '../../types' @@ -67,7 +68,7 @@ export default function CollectionHero({ {shown.map(m => ( m.avatar - ? {m.username} + ? {m.username} : {initials(m.username)} ))} {extra > 0 && +{extra}} diff --git a/client/src/components/Collections/ShareCollectionModal.tsx b/client/src/components/Collections/ShareCollectionModal.tsx index d3ad469fc..b9ff7868e 100644 --- a/client/src/components/Collections/ShareCollectionModal.tsx +++ b/client/src/components/Collections/ShareCollectionModal.tsx @@ -1,4 +1,5 @@ import React, { useEffect, useMemo, useState } from 'react' +import { avatarSrc } from '../../utils/avatarSrc' import { UserPlus, UserMinus, Loader2, Clock, Crown, LogOut } from 'lucide-react' import type { CollectionMember, CollectionRole } from '@trek/shared' import Modal from '../shared/Modal' @@ -29,7 +30,7 @@ function MemberAvatar({ member }: { member: CollectionMember }): React.ReactElem return ( {member.avatar ? ( - + ) : ( initial )} diff --git a/client/src/components/Planner/DayPlanSidebar.tsx b/client/src/components/Planner/DayPlanSidebar.tsx index b1cfe4040..5745abf79 100644 --- a/client/src/components/Planner/DayPlanSidebar.tsx +++ b/client/src/components/Planner/DayPlanSidebar.tsx @@ -3,6 +3,7 @@ interface DragDataPayload { placeId?: string; assignmentId?: string; noteId?: st declare global { interface Window { __dragData: DragDataPayload | null } } import React, { useState, useEffect, useLayoutEffect, useRef, useMemo } from 'react' +import { avatarSrc } from '../../utils/avatarSrc' import { ChevronDown, ChevronRight, ChevronUp, Navigation, RotateCcw, ExternalLink, Clock, Pencil, GripVertical, Ticket, Plus, FileText, Trash2, Car, Lock, Hotel, Footprints, Route as RouteIcon, Bookmark } from 'lucide-react' import { assignmentsApi, reservationsApi } from '../../api/client' import { calculateRoute, calculateRouteWithLegs, optimizeRoute, generateGoogleMapsUrl } from '../Map/RouteCalculator' @@ -1867,7 +1868,7 @@ const DayPlanSidebar = React.memo(function DayPlanSidebar(props: DayPlanSidebarP marginLeft: pi > 0 ? -4 : 0, flexShrink: 0, overflow: 'hidden', }}> - {p.avatar ? : p.username?.[0]?.toUpperCase()} + {p.avatar ? : p.username?.[0]?.toUpperCase()} ))} {assignment.participants.length > 5 && ( diff --git a/client/src/components/Planner/PlaceInspector.tsx b/client/src/components/Planner/PlaceInspector.tsx index e2ccbdf20..4d1684f98 100644 --- a/client/src/components/Planner/PlaceInspector.tsx +++ b/client/src/components/Planner/PlaceInspector.tsx @@ -1,4 +1,5 @@ import React, { useState, useEffect, useRef, useCallback, useMemo } from 'react' +import { avatarSrc } from '../../utils/avatarSrc' import { openFile } from '../../utils/fileDownload' import Markdown from 'react-markdown' import remarkGfm from 'remark-gfm' @@ -542,7 +543,7 @@ function ParticipantsBox({ tripMembers, participantIds, allJoined, onSetParticip display: 'flex', alignItems: 'center', justifyContent: 'center', fontSize: 'calc(7px * var(--fs-scale-caption, 1))', fontWeight: 700, overflow: 'hidden', flexShrink: 0, }}> - {(member.avatar_url || member.avatar) ? : member.username?.[0]?.toUpperCase()} + {(member.avatar_url || member.avatar) ? : member.username?.[0]?.toUpperCase()} {member.username} @@ -582,7 +583,7 @@ function ParticipantsBox({ tripMembers, participantIds, allJoined, onSetParticip display: 'flex', alignItems: 'center', justifyContent: 'center', fontSize: 'calc(8px * var(--fs-scale-caption, 1))', fontWeight: 700, overflow: 'hidden', flexShrink: 0, }}> - {(member.avatar_url || member.avatar) ? : member.username?.[0]?.toUpperCase()} + {(member.avatar_url || member.avatar) ? : member.username?.[0]?.toUpperCase()} {member.username} {member.is_guest && } diff --git a/client/src/components/Todo/TodoListPanel.tsx b/client/src/components/Todo/TodoListPanel.tsx index bf194d9cc..4046e504a 100644 --- a/client/src/components/Todo/TodoListPanel.tsx +++ b/client/src/components/Todo/TodoListPanel.tsx @@ -1,4 +1,5 @@ import { useState, useMemo, useEffect, useRef } from 'react' +import { avatarSrc } from '../../utils/avatarSrc' import ReactDOM from 'react-dom' import { useTripStore } from '../../store/tripStore' import { useCanDo } from '../../store/permissionsStore' @@ -481,7 +482,7 @@ function DetailPane({ item, tripId, categories, members, onClose }: { value: String(m.id), label: m.is_guest ? `${m.username} · ${t('members.guest')}` : m.username, icon: m.avatar ? ( - + ) : ( {m.username.charAt(0).toUpperCase()} @@ -671,7 +672,7 @@ function NewTaskPane({ tripId, categories, members, defaultCategory, onCreated, ...members.map(m => ({ value: String(m.id), label: m.username, icon: m.avatar ? ( - + ) : ( {m.username.charAt(0).toUpperCase()} diff --git a/client/src/components/Todo/TodoRow.tsx b/client/src/components/Todo/TodoRow.tsx index fde1a112d..f49941b9d 100644 --- a/client/src/components/Todo/TodoRow.tsx +++ b/client/src/components/Todo/TodoRow.tsx @@ -1,4 +1,5 @@ import { CheckSquare, Square, ChevronRight, Flag, Calendar, GripVertical, UserRound } from 'lucide-react' +import { avatarSrc } from '../../utils/avatarSrc' import type { TodoItem } from '../../types' import { katColor, PRIO_CONFIG, type Member } from './todoListModel' @@ -125,7 +126,7 @@ export default function TodoRow({ item, members, categories, today, isSelected, border: '1px solid var(--border-faint)', }}> {assignedUser.avatar ? ( - + ) : ( {assignedUser.username.charAt(0).toUpperCase()} diff --git a/client/src/pages/SharedTripPage.tsx b/client/src/pages/SharedTripPage.tsx index 96bb41e23..1dfe055a2 100644 --- a/client/src/pages/SharedTripPage.tsx +++ b/client/src/pages/SharedTripPage.tsx @@ -1,4 +1,5 @@ import { useEffect } from 'react' +import { avatarSrc } from '../utils/avatarSrc' import { MapContainer, TileLayer, Marker, Tooltip, useMap } from 'react-leaflet' import L from 'leaflet' import { useTranslation, SUPPORTED_LANGUAGES } from '../i18n' @@ -380,7 +381,7 @@ export default function SharedTripPage() { )}
- {msg.avatar ? : (msg.username || '?')[0].toUpperCase()} + {msg.avatar ? : (msg.username || '?')[0].toUpperCase()}
diff --git a/client/src/utils/avatarSrc.test.ts b/client/src/utils/avatarSrc.test.ts new file mode 100644 index 000000000..c041272ed --- /dev/null +++ b/client/src/utils/avatarSrc.test.ts @@ -0,0 +1,18 @@ +import { describe, it, expect } from 'vitest' +import { avatarSrc } from './avatarSrc' + +describe('avatarSrc', () => { + it('prefixes an uploaded file name with the avatars path', () => { + expect(avatarSrc('abc123.jpg')).toBe('/uploads/avatars/abc123.jpg') + }) + + it('passes an absolute https URL (OIDC picture) through untouched', () => { + expect(avatarSrc('https://idp.example.com/u/pic.png')).toBe('https://idp.example.com/u/pic.png') + }) + + it('returns null for empty input', () => { + expect(avatarSrc(null)).toBeNull() + expect(avatarSrc(undefined)).toBeNull() + expect(avatarSrc('')).toBeNull() + }) +}) diff --git a/client/src/utils/avatarSrc.ts b/client/src/utils/avatarSrc.ts new file mode 100644 index 000000000..b5693c4f9 --- /dev/null +++ b/client/src/utils/avatarSrc.ts @@ -0,0 +1,12 @@ +/** + * Resolve a user's raw `avatar` field to an . + * + * The value is either an uploaded file name (served from /uploads/avatars) or, + * for SSO users who never uploaded one, an absolute https URL from their OIDC + * `picture` claim (#1399), which is used as-is. Server responses usually expose a + * pre-resolved `avatar_url`; use this only where components read the raw field. + */ +export function avatarSrc(avatar?: string | null): string | null { + if (!avatar) return null + return /^https:\/\//i.test(avatar) ? avatar : `/uploads/avatars/${avatar}` +} diff --git a/server/src/nest/oidc/oidc.controller.ts b/server/src/nest/oidc/oidc.controller.ts index 8d783603f..2dc6be5ad 100644 --- a/server/src/nest/oidc/oidc.controller.ts +++ b/server/src/nest/oidc/oidc.controller.ts @@ -128,6 +128,12 @@ export class OidcController { return f('/login?oidc_error=subject_mismatch'); } + // Some IdPs put `picture` only in the id_token, not the userinfo response — + // fall back to it so the avatar (#1399) still populates. + if (!userInfo.picture && typeof idVerify.claims.picture === 'string') { + userInfo.picture = idVerify.claims.picture; + } + const result = this.oidc.findOrCreateUser(userInfo, config, pending.inviteToken); if ('error' in result) return f('/login?oidc_error=' + result.error); diff --git a/server/src/services/adminService.ts b/server/src/services/adminService.ts index c6ef3a6af..ff3a4e611 100644 --- a/server/src/services/adminService.ts +++ b/server/src/services/adminService.ts @@ -1,4 +1,5 @@ import bcrypt from 'bcryptjs'; +import { avatarUrl } from './avatarUrl'; import crypto from 'crypto'; import path from 'path'; import fs from 'fs'; @@ -71,7 +72,7 @@ export function listUsers() { } catch { /* */ } return users.map(u => ({ ...u, - avatar_url: u.avatar ? `/uploads/avatars/${u.avatar}` : null, + avatar_url: avatarUrl(u), created_at: utcSuffix(u.created_at), updated_at: utcSuffix(u.updated_at as string), last_login: utcSuffix(u.last_login), diff --git a/server/src/services/authService.ts b/server/src/services/authService.ts index 3e368c224..ec212e731 100644 --- a/server/src/services/authService.ts +++ b/server/src/services/authService.ts @@ -711,7 +711,9 @@ export function getSettings(userId: number): { error?: string; status?: number; export async function saveAvatar(userId: number, filename: string) { const current = db.prepare('SELECT avatar FROM users WHERE id = ?').get(userId) as { avatar: string | null } | undefined; - if (current && current.avatar) { + // Only a locally uploaded file has something to clean up. An OIDC picture URL + // (#1399) has no file on disk, so skip the rm — path.join on a URL is meaningless. + if (current?.avatar && !/^https:\/\//i.test(current.avatar)) { // Fire-and-forget: leftover files are harmless; the DB update is // the source of truth for which avatar is current. const oldPath = path.join(avatarDir, current.avatar); @@ -726,7 +728,8 @@ export async function saveAvatar(userId: number, filename: string) { export async function deleteAvatar(userId: number) { const current = db.prepare('SELECT avatar FROM users WHERE id = ?').get(userId) as { avatar: string | null } | undefined; - if (current && current.avatar) { + // An OIDC picture URL (#1399) has no local file — only rm an uploaded one. + if (current?.avatar && !/^https:\/\//i.test(current.avatar)) { const filePath = path.join(avatarDir, current.avatar); await fs.promises.rm(filePath, { force: true }).catch(() => {}); } diff --git a/server/src/services/avatarUrl.ts b/server/src/services/avatarUrl.ts index d3919f5f0..069effdc1 100644 --- a/server/src/services/avatarUrl.ts +++ b/server/src/services/avatarUrl.ts @@ -1,3 +1,12 @@ +/** + * Resolve a user's stored avatar reference to a renderable URL. + * + * The value is either an uploaded file name (served from /uploads/avatars) or, + * for SSO users who never uploaded one, an absolute https URL taken from their + * OIDC `picture` claim (#1399) — that one is passed through untouched. + */ export function avatarUrl(user: { avatar?: string | null }): string | null { - return user.avatar ? `/uploads/avatars/${user.avatar}` : null; + if (!user.avatar) return null; + if (/^https:\/\//i.test(user.avatar)) return user.avatar; + return `/uploads/avatars/${user.avatar}`; } diff --git a/server/src/services/fileService.ts b/server/src/services/fileService.ts index 072fecc82..e4fdc0068 100644 --- a/server/src/services/fileService.ts +++ b/server/src/services/fileService.ts @@ -1,4 +1,5 @@ import path from 'path'; +import { avatarUrl } from './avatarUrl'; import fs from 'fs'; import type { Request } from 'express'; import { db } from '../db/database'; @@ -66,7 +67,7 @@ export function formatFile(file: TripFile & { trip_id?: number; uploaded_by_avat return { ...file, url: `/api/trips/${tripId}/files/${file.id}/download`, - uploaded_by_avatar: file.uploaded_by_avatar ? `/uploads/avatars/${file.uploaded_by_avatar}` : null, + uploaded_by_avatar: avatarUrl({ avatar: file.uploaded_by_avatar }), }; } diff --git a/server/src/services/inAppNotifications.ts b/server/src/services/inAppNotifications.ts index ea972b4d9..fbccd6569 100644 --- a/server/src/services/inAppNotifications.ts +++ b/server/src/services/inAppNotifications.ts @@ -1,4 +1,5 @@ import { db } from '../db/database'; +import { avatarUrl } from './avatarUrl'; import { broadcastToUser } from '../websocket'; import { getAction } from './inAppNotificationActions'; import { isEnabledForEvent, type NotifEventType } from './notificationPreferencesService'; @@ -171,7 +172,7 @@ function createNotification(input: NotificationInput): number[] { notification: { ...row, sender_username: sender?.username ?? null, - sender_avatar: sender?.avatar ? `/uploads/avatars/${sender.avatar}` : null, + sender_avatar: avatarUrl({ avatar: sender?.avatar }), }, }); } @@ -232,7 +233,7 @@ export function createNotificationForRecipient( ...row, created_at: toUtcIso(row.created_at), sender_username: sender?.username ?? null, - sender_avatar: sender?.avatar ? `/uploads/avatars/${sender.avatar}` : null, + sender_avatar: avatarUrl({ avatar: sender?.avatar }), }, }); @@ -265,7 +266,7 @@ function getNotifications( const mapped = rows.map(r => ({ ...r, created_at: toUtcIso(r.created_at), - sender_avatar: r.sender_avatar ? `/uploads/avatars/${r.sender_avatar}` : null, + sender_avatar: avatarUrl({ avatar: r.sender_avatar }), })); return { notifications: mapped, total, unread_count }; @@ -347,7 +348,7 @@ async function respondToBoolean( const mappedUpdated = { ...updated, - sender_avatar: updated.sender_avatar ? `/uploads/avatars/${updated.sender_avatar}` : null, + sender_avatar: avatarUrl({ avatar: updated.sender_avatar }), }; broadcastToUser(userId, { type: 'notification:updated', notification: mappedUpdated }); diff --git a/server/src/services/journeyService.ts b/server/src/services/journeyService.ts index 22dddcc50..ff1b54646 100644 --- a/server/src/services/journeyService.ts +++ b/server/src/services/journeyService.ts @@ -1,4 +1,5 @@ import { db, canAccessTrip } from '../db/database'; +import { avatarUrl } from './avatarUrl'; import type { Journey, JourneyEntry, JourneyPhoto, JourneyContributor } from '../types'; import { broadcastToUser } from '../websocket'; import { @@ -214,7 +215,7 @@ export function getJourneyFull(journeyId: number, userId: number) { .all(journeyId) as any[]; const contributors = contributorsRaw.map((c) => ({ ...c, - avatar_url: c.avatar ? `/uploads/avatars/${c.avatar}` : null, + avatar_url: avatarUrl(c), })); // stats diff --git a/server/src/services/oidcService.ts b/server/src/services/oidcService.ts index 546ce91c6..aa273dd1b 100644 --- a/server/src/services/oidcService.ts +++ b/server/src/services/oidcService.ts @@ -32,6 +32,8 @@ export interface OidcUserInfo { email_verified?: boolean | string; name?: string; preferred_username?: string; + // Standard OIDC profile claim: URL of the user's profile picture. + picture?: string; groups?: string[]; roles?: string[]; [key: string]: unknown; @@ -355,6 +357,17 @@ export async function verifyIdToken( // Find or create user by OIDC sub / email // --------------------------------------------------------------------------- +// Sanitize the OIDC `picture` claim before we store it as the avatar. Only https +// URLs are usable: the app's CSP allows https image sources but not http, and we +// render the value directly. Non-strings, non-https and oversized values (e.g. a +// large data: URI) are ignored so a user payload never carries junk. #1399 +function safeOidcPicture(picture: unknown): string | null { + if (typeof picture !== 'string') return null; + const url = picture.trim(); + if (!url || url.length > 1024) return null; + return /^https:\/\//i.test(url) ? url : null; +} + export function findOrCreateUser( userInfo: OidcUserInfo, config: OidcConfig, @@ -363,6 +376,7 @@ export function findOrCreateUser( const email = userInfo.email!.trim().toLowerCase(); const name = userInfo.name || userInfo.preferred_username || email.split('@')[0]; const sub = userInfo.sub; + const picture = safeOidcPicture(userInfo.picture); // Try to find existing user by sub, then by email let user = db.prepare('SELECT * FROM users WHERE oidc_sub = ? AND oidc_issuer = ?').get(sub, config.issuer) as User | undefined; @@ -402,6 +416,14 @@ export function findOrCreateUser( } } } + // Keep the avatar in sync with the OIDC picture, but never clobber a custom + // upload: only fill it when empty or when the current value is itself an OIDC + // picture URL, so the picture refreshes on each login without overriding an + // uploaded one. #1399 + if (picture && picture !== user.avatar && (!user.avatar || /^https:\/\//i.test(user.avatar))) { + db.prepare('UPDATE users SET avatar = ? WHERE id = ?').run(picture, user.id); + user = { ...user, avatar: picture } as User; + } return { user }; } @@ -452,11 +474,11 @@ export function findOrCreateUser( if (updated.changes === 0) throw inviteRaceError; } return db.prepare( - 'INSERT INTO users (username, email, password_hash, role, oidc_sub, oidc_issuer, first_seen_version, login_count) VALUES (?, ?, ?, ?, ?, ?, ?, 0)', - ).run(username, email, hash, role, sub, config.issuer, process.env.APP_VERSION || '0.0.0'); + 'INSERT INTO users (username, email, password_hash, role, oidc_sub, oidc_issuer, avatar, first_seen_version, login_count) VALUES (?, ?, ?, ?, ?, ?, ?, ?, 0)', + ).run(username, email, hash, role, sub, config.issuer, picture, process.env.APP_VERSION || '0.0.0'); }); const result = createUser() as { lastInsertRowid: number | bigint }; - user = { id: Number(result.lastInsertRowid), username, email, role } as User; + user = { id: Number(result.lastInsertRowid), username, email, role, avatar: picture } as User; return { user }; } catch (err) { if (err === inviteRaceError) { diff --git a/server/src/services/tripService.ts b/server/src/services/tripService.ts index 1b0b0c3b1..8df373a69 100644 --- a/server/src/services/tripService.ts +++ b/server/src/services/tripService.ts @@ -1,4 +1,5 @@ import path from 'path'; +import { avatarUrl } from './avatarUrl'; import fs from 'fs'; import { randomUUID } from 'crypto'; import { db, isOwner } from '../db/database'; @@ -365,8 +366,8 @@ export function listMembers(tripId: string | number, tripOwnerId: number) { const owner = db.prepare('SELECT id, username, email, avatar FROM users WHERE id = ?').get(tripOwnerId) as Pick; return { - owner: { ...owner, role: 'owner', is_guest: false, avatar_url: owner.avatar ? `/uploads/avatars/${owner.avatar}` : null }, - members: members.map(m => ({ ...m, is_guest: !!m.is_guest, avatar_url: m.avatar ? `/uploads/avatars/${m.avatar}` : null })), + owner: { ...owner, role: 'owner', is_guest: false, avatar_url: avatarUrl(owner) }, + members: members.map(m => ({ ...m, is_guest: !!m.is_guest, avatar_url: avatarUrl(m) })), }; } @@ -398,7 +399,7 @@ export function addMember(tripId: string | number, identifier: string, tripOwner const tripInfo = db.prepare('SELECT title FROM trips WHERE id = ?').get(tripId) as { title: string } | undefined; return { - member: { ...target, role: 'member', avatar_url: target.avatar ? `/uploads/avatars/${target.avatar}` : null }, + member: { ...target, role: 'member', avatar_url: avatarUrl(target) }, targetUserId: target.id, tripTitle: tripInfo?.title || 'Untitled', }; diff --git a/server/tests/unit/services/avatarUrl.test.ts b/server/tests/unit/services/avatarUrl.test.ts new file mode 100644 index 000000000..5a4e1c21f --- /dev/null +++ b/server/tests/unit/services/avatarUrl.test.ts @@ -0,0 +1,25 @@ +/** + * Unit tests for avatarUrl — AVATAR-URL-001 through AVATAR-URL-003. + * The stored value is either an uploaded file name or an absolute https URL from + * an OIDC `picture` claim (#1399); the helper resolves both to a renderable src. + */ +import { describe, it, expect } from 'vitest'; +import { avatarUrl } from '../../../src/services/avatarUrl'; + +describe('avatarUrl', () => { + it('AVATAR-URL-001: prefixes an uploaded file name with the avatars path', () => { + expect(avatarUrl({ avatar: 'abc123.jpg' })).toBe('/uploads/avatars/abc123.jpg'); + }); + + it('AVATAR-URL-002: passes an absolute https URL (OIDC picture) through untouched', () => { + expect(avatarUrl({ avatar: 'https://idp.example.com/u/pic.png' })).toBe( + 'https://idp.example.com/u/pic.png', + ); + }); + + it('AVATAR-URL-003: returns null when no avatar is set', () => { + expect(avatarUrl({ avatar: null })).toBeNull(); + expect(avatarUrl({ avatar: undefined })).toBeNull(); + expect(avatarUrl({})).toBeNull(); + }); +}); diff --git a/server/tests/unit/services/oidcService.test.ts b/server/tests/unit/services/oidcService.test.ts index 6a86b2cc1..0b22c5158 100644 --- a/server/tests/unit/services/oidcService.test.ts +++ b/server/tests/unit/services/oidcService.test.ts @@ -464,6 +464,63 @@ describe('findOrCreateUser', () => { const token = testDb.prepare("SELECT used_count FROM invite_tokens WHERE token = 'tok-full'").get() as any; expect(token.used_count).toBe(1); }); + + // ── OIDC picture claim → avatar (#1399) ────────────────────────────────── + + it('OIDC-SVC-040: new user stores the https picture claim as their avatar', () => { + const result = findOrCreateUser( + { sub: 'sub-pic-1', email: 'pic1@example.com', name: 'Pic One', picture: 'https://idp.example.com/u/pic1.png' }, + MOCK_CONFIG + ); + expect('user' in result).toBe(true); + const row = testDb.prepare("SELECT avatar FROM users WHERE email = 'pic1@example.com'").get() as any; + expect(row.avatar).toBe('https://idp.example.com/u/pic1.png'); + }); + + it('OIDC-SVC-041: new user with a non-https picture claim stores no avatar', () => { + findOrCreateUser( + { sub: 'sub-pic-2', email: 'pic2@example.com', name: 'Pic Two', picture: 'http://idp.example.com/u/pic2.png' }, + MOCK_CONFIG + ); + const row = testDb.prepare("SELECT avatar FROM users WHERE email = 'pic2@example.com'").get() as any; + expect(row.avatar).toBeNull(); + }); + + it('OIDC-SVC-042: existing user with no avatar gets the OIDC picture', () => { + const { user } = createUser(testDb, { email: 'pic3@example.com' }); + testDb.prepare('UPDATE users SET oidc_sub = ?, oidc_issuer = ?, avatar = NULL WHERE id = ?') + .run('sub-pic-3', MOCK_CONFIG.issuer, user.id); + findOrCreateUser( + { sub: 'sub-pic-3', email: 'pic3@example.com', name: 'Pic Three', picture: 'https://idp.example.com/u/pic3.png' }, + MOCK_CONFIG + ); + const row = testDb.prepare('SELECT avatar FROM users WHERE id = ?').get(user.id) as any; + expect(row.avatar).toBe('https://idp.example.com/u/pic3.png'); + }); + + it('OIDC-SVC-043: a custom uploaded avatar is never overwritten by the OIDC picture', () => { + const { user } = createUser(testDb, { email: 'pic4@example.com' }); + testDb.prepare('UPDATE users SET oidc_sub = ?, oidc_issuer = ?, avatar = ? WHERE id = ?') + .run('sub-pic-4', MOCK_CONFIG.issuer, 'uploaded-abc.jpg', user.id); + findOrCreateUser( + { sub: 'sub-pic-4', email: 'pic4@example.com', name: 'Pic Four', picture: 'https://idp.example.com/u/pic4.png' }, + MOCK_CONFIG + ); + const row = testDb.prepare('SELECT avatar FROM users WHERE id = ?').get(user.id) as any; + expect(row.avatar).toBe('uploaded-abc.jpg'); + }); + + it('OIDC-SVC-044: a previously stored OIDC picture URL is refreshed on next login', () => { + const { user } = createUser(testDb, { email: 'pic5@example.com' }); + testDb.prepare('UPDATE users SET oidc_sub = ?, oidc_issuer = ?, avatar = ? WHERE id = ?') + .run('sub-pic-5', MOCK_CONFIG.issuer, 'https://idp.example.com/u/old.png', user.id); + findOrCreateUser( + { sub: 'sub-pic-5', email: 'pic5@example.com', name: 'Pic Five', picture: 'https://idp.example.com/u/new.png' }, + MOCK_CONFIG + ); + const row = testDb.prepare('SELECT avatar FROM users WHERE id = ?').get(user.id) as any; + expect(row.avatar).toBe('https://idp.example.com/u/new.png'); + }); }); // ── exchangeCodeForToken ──────────────────────────────────────────────────────