fix: oidc issues with cache (#3512)

closes #3497
closes #3502

src/backend/controllers/auth/AuthController.test.ts

@@ -955,6 +955,40 @@ describe('AuthController.handleLogin', () => {
         expect(isCompleteLoginResponse(res.body)).toBe(true);
     });
 
+    it('refuses an email the account has moved off', async () => {
+        const moverName = `lm_${Math.random().toString(36).slice(2, 10)}`;
+        const oldEmail = `${moverName}-old@test.local`;
+        const newEmail = `${moverName}-new@test.local`;
+        await controller.handleSignup(
+            makeReq({ username: moverName, email: oldEmail, password }),
+            makeRes(),
+        );
+        // Warm the by-email lookup the way a real login would.
+        await controller.handleLogin(
+            makeReq({ email: oldEmail, password }),
+            makeRes(),
+        );
+
+        const mover = await server.stores.user.getByUsername(moverName);
+        await server.stores.user.update(mover!.id, {
+            email: newEmail,
+            clean_email: newEmail,
+        });
+
+        await expect(
+            controller.handleLogin(
+                makeReq({ email: oldEmail, password }),
+                makeRes(),
+            ),
+        ).rejects.toMatchObject({ statusCode: 404 });
+        const res = makeRes();
+        await controller.handleLogin(
+            makeReq({ email: newEmail, password }),
+            res,
+        );
+        expect(isCompleteLoginResponse(res.body)).toBe(true);
+    });
+
     it('returns 400 when neither username nor email is supplied', async () => {
         await expect(
             controller.handleLogin(makeReq({ password }), makeRes()),
@@ -4130,6 +4164,30 @@ describe('AuthController user-protected mutations (validation paths)', () => {
         ).rejects.toMatchObject({ statusCode: 400 });
     });
 
+    it('change-email: accepts an address that resolves back to the caller', async () => {
+        // `foo+tag@gmail.com` canonicalizes to the caller's own row, so the
+        // collision check has to exclude them — otherwise Puter reports your
+        // own address as already in use and there's no way to set it.
+        const local = `ch_${uniq()}`;
+        const { user, actor } = await makeUserAndActor();
+        await server.stores.user.update(user.id, {
+            email: `${local}@gmail.com`,
+            clean_email: `${local}@gmail.com`,
+            email_confirmed: 1,
+        });
+
+        const res = makeRes();
+        await controller.handleChangeEmail(
+            makeReq({ new_email: `${local}+work@gmail.com` }, { actor }),
+            res,
+        );
+        expect(res.body).toEqual({});
+        const after = await server.stores.user.getById(user.id, {
+            force: true,
+        });
+        expect(after!.unconfirmed_change_email).toBe(`${local}+work@gmail.com`);
+    });
+
     it('change-email: stages the new email + token on success', async () => {
         const { user, actor } = await makeUserAndActor();
         const newEmail = `ch_${uniq()}@test.local`;

src/backend/controllers/auth/AuthController.ts

@@ -1004,8 +1004,7 @@ export class AuthController extends PuterController {
                     is_temp: user!.password === null && user!.email === null,
                     ip:
                         (req?.headers?.['x-forwarded-for'] as
-                            | string
-                            | undefined) ||
+                            string | undefined) ||
                         (
                             req as unknown as {
                                 connection?: { remoteAddress?: string };
@@ -2339,15 +2338,18 @@ export class AuthController extends PuterController {
         }
         await this.#validateEmail(new_email);
 
-        // Block if any confirmed account (password or OIDC) already
+        // Block if any OTHER confirmed account (password or OIDC) already
         // owns that email. Match raw + canonical to collapse gmail
-        // aliases.
+        // aliases — which is also why the caller has to be excluded: an
+        // alias of your own current address resolves back to you, and
+        // "already in use" about yourself is nonsense.
         const canonical = cleanEmail(new_email);
         const existing =
             (await this.stores.user.getByEmail(new_email)) ??
             (await this.stores.user.getByCleanEmail(canonical));
         if (
             existing &&
+            existing.id !== req.actor!.user.id &&
             (existing.email_confirmed || existing.password !== null)
         ) {
             throw new HttpError(400, 'This email is already in use.', {

src/backend/controllers/oidc/OIDCController.test.ts

@@ -1045,6 +1045,39 @@ describe('OIDCController signup veto (abuse harness)', () => {
         expect(captured.redirectUrl).toContain('message=signup_blocked');
         expect(captured.redirectUrl).not.toContain('request_code');
     });
+
+    it('keeps a veto legible when the listener stamps its own code', async () => {
+        const email = `veto-${Math.random().toString(36).slice(2, 8)}@test.local`;
+        signupValidateOverride = (data) => {
+            if (data.email !== email) return;
+            data.allow = false;
+            data.code = 'email_reputation_too_low';
+            data.trail_id = 'trail-custom-code';
+        };
+        stubIdp(`sub-${Math.random().toString(36).slice(2, 8)}`, email);
+
+        const state = oidc().signState({
+            provider: 'custom',
+            redirect_uri: TEST_ORIGIN + '/',
+        });
+        const { res, captured } = makeRes();
+        await callRoute(
+            'get',
+            '/auth/oidc/callback/signup',
+            makeReq({ query: { code: 'c', state } }),
+            res,
+        );
+
+        // A listener code isn't one of the display codes, but the failure
+        // is still a blocked signup — collapsing it to `unauthorized`
+        // would tell the user sign-in broke and bury the Request Code
+        // support needs.
+        expect(captured.redirectUrl).toContain('message=signup_blocked');
+        expect(captured.redirectUrl).not.toContain('message=unauthorized');
+        expect(captured.redirectUrl).toContain(
+            'request_code=trail-custom-code',
+        );
+    });
 });
 
 describe('OIDCController browser binding', () => {

src/backend/controllers/oidc/OIDCController.ts

@@ -47,6 +47,23 @@ const ALLOWED_ERRORS = [
     'signup_blocked',
 ] as const;
 
+/**
+ * Pick the display code for a failed user resolution.
+ *
+ * A `code` is only ever set when the signup-validate harness vetoed the signup,
+ * but the code it stamps comes from an abuse listener and is not drawn from
+ * {@link ALLOWED_ERRORS} — so clamping it directly turns every vetoed OIDC
+ * signup into a bare `unauthorized`, which reads as "sign-in broke" and hides
+ * both the real cause and the Request Code that support looks the decision up
+ * by. Anything unrecognized falls back to the veto's own category instead.
+ */
+function resolutionErrorCode(code: string | undefined): string {
+    if (!code) return 'unauthorized';
+    return (ALLOWED_ERRORS as readonly string[]).includes(code)
+        ? code
+        : 'signup_blocked';
+}
+
 // GUI pages an OIDC flow may return to: /desktop, /dashboard, and direct app
 // landings (/app/<name> and its desktop-booted twin /desktop/app/<name>,
 // mirroring APP_NAME_REGEX in AppDriver). Strict whitelist — never a
@@ -404,7 +421,7 @@ export class OIDCController extends PuterController {
                         origin,
                         'login',
                         'other',
-                        resolved.code ?? 'unauthorized',
+                        resolutionErrorCode(resolved.code),
                         stateDecoded,
                         resolved.requestCode,
                         (p) => this.services.oidc.signPopupReturn(p),
@@ -442,6 +459,7 @@ export class OIDCController extends PuterController {
             const origin = this.config.origin ?? '';
             const result = await this.#processCallback(req, res, 'signup');
             if ('error' in result) {
+                console.warn(`OIDC signup callback error: ${result.error}`);
                 return res.redirect(
                     302,
                     buildErrorRedirectUrl(
@@ -470,7 +488,7 @@ export class OIDCController extends PuterController {
                         origin,
                         'signup',
                         'other',
-                        resolved.code ?? 'unauthorized',
+                        resolutionErrorCode(resolved.code),
                         stateDecoded,
                         resolved.requestCode,
                         (p) => this.services.oidc.signPopupReturn(p),

src/backend/stores/user/UserStore.test.ts

@@ -55,6 +55,95 @@ describe('UserStore', () => {
         expect(typeof cachedUser?.requires_email_confirmation).toBe('boolean');
     });
 
+    it('stops resolving an email the account no longer holds', async () => {
+        const username = `us-${Math.random().toString(36).slice(2, 10)}`;
+        const oldEmail = `${username}-old@test.local`;
+        const newEmail = `${username}-new@test.local`;
+        const user = await server.stores.user.create({
+            username,
+            uuid: uuidv4(),
+            password: null,
+            email: oldEmail,
+        });
+
+        // Warm the by-email key, then move the account to a new address.
+        expect((await server.stores.user.getByEmail(oldEmail))?.id).toBe(
+            user.id,
+        );
+        await server.stores.user.update(user.id, {
+            email: newEmail,
+            clean_email: newEmail,
+        });
+
+        // The old address is no longer this account's — a cached copy of the
+        // pre-update row answering for it is what let a replaced email keep
+        // working as a login.
+        expect(await server.stores.user.getByEmail(oldEmail)).toBeNull();
+        expect((await server.stores.user.getByEmail(newEmail))?.id).toBe(
+            user.id,
+        );
+    });
+
+    it('stops resolving a username the account no longer holds', async () => {
+        const oldUsername = `us-${Math.random().toString(36).slice(2, 10)}`;
+        const newUsername = `${oldUsername}-renamed`;
+        const user = await server.stores.user.create({
+            username: oldUsername,
+            uuid: uuidv4(),
+            password: null,
+            email: `${oldUsername}@test.local`,
+        });
+
+        expect((await server.stores.user.getByUsername(oldUsername))?.id).toBe(
+            user.id,
+        );
+        await server.stores.user.update(user.id, { username: newUsername });
+
+        expect(await server.stores.user.getByUsername(oldUsername)).toBeNull();
+        expect((await server.stores.user.getByUsername(newUsername))?.id).toBe(
+            user.id,
+        );
+    });
+
+    it('stops resolving an email revoked from a competing account', async () => {
+        const shared = `shared-${Math.random().toString(36).slice(2, 10)}@test.local`;
+        const makeClaimant = async () => {
+            const username = `uc-${Math.random().toString(36).slice(2, 10)}`;
+            const claimant = await server.stores.user.create({
+                username,
+                uuid: uuidv4(),
+                password: null,
+                email: shared,
+                clean_email: shared,
+            });
+            await server.stores.user.update(claimant.id, {
+                email_confirmed: true,
+            });
+            return claimant;
+        };
+
+        const loser = await makeClaimant();
+        const winner = await makeClaimant();
+
+        // Warm the loser's cache entries before its claim is revoked.
+        expect((await server.stores.user.getById(loser.id))?.email).toBe(
+            shared,
+        );
+
+        await server.stores.user.unconfirmOthersByEmail(
+            winner.id,
+            shared,
+            shared,
+        );
+
+        const strippedLoser = await server.stores.user.getById(loser.id);
+        expect(strippedLoser?.email).toBeNull();
+        expect(strippedLoser?.email_confirmed).toBe(false);
+        expect((await server.stores.user.getByEmail(shared))?.id).toBe(
+            winner.id,
+        );
+    });
+
     it('counts other accounts holding the same phone number', async () => {
         const phone = `+1415555${Math.floor(1000 + Math.random() * 9000)}`;
         const makeUser = async () => {

src/backend/stores/user/UserStore.ts

@@ -465,12 +465,36 @@ export class UserStore extends PuterStore {
         const setClause = keys.map((k) => `\`${k}\` = ?`).join(', ');
         const values = keys.map((k) => dbPatch[k]);
 
+        // Identifying columns are themselves cache keys, so changing one
+        // leaves the old key holding a full copy of the pre-update row.
+        // Snapshot the row first so the keys this write retires can be
+        // dropped: otherwise a replaced email keeps resolving to the account
+        // for the rest of the TTL, and login and password recovery accept it
+        // as if it were still the account's address.
+        const touchesIdentity = keys.some((k) =>
+            (USER_ID_PROPERTIES as readonly string[]).includes(k),
+        );
+        const before = touchesIdentity
+            ? await this.getByProperty('id', userId, { force: true })
+            : null;
+
         await this.clients.db.write(
             `UPDATE \`user\` SET ${setClause} WHERE \`id\` = ?`,
             [...values, userId],
         );
 
         const fresh = await this.getByProperty('id', userId, { force: true });
+
+        if (before) {
+            const live = new Set(fresh ? this.#cacheKeysForUser(fresh) : []);
+            const retired = this.#cacheKeysForUser(before).filter(
+                (key) => !live.has(key),
+            );
+            if (retired.length > 0) {
+                await this.publishCacheKeys({ keys: retired, broadcast: true });
+            }
+        }
+
         if (fresh) {
             await this.#refreshCache(fresh);
         } else {
@@ -501,6 +525,15 @@ export class UserStore extends PuterStore {
         email: string,
         cleanEmailValue: string,
     ): Promise<void> {
+        // Read the rows this strips before stripping them: the update goes
+        // straight to SQL, so without their pre-image we don't know which
+        // cache keys it retires — and a cached copy still carries the address
+        // that was just revoked, which would keep answering lookups for it.
+        const stripped = (await this.clients.db.pread(
+            'SELECT * FROM `user` WHERE `id` != ? AND (`email` = ? OR `clean_email` = ?)',
+            [userId, email, cleanEmailValue],
+        )) as Array<Record<string, unknown>>;
+
         await this.clients.db.write(
             `UPDATE \`user\`
                 SET \`email\` = NULL,
@@ -519,6 +552,10 @@ export class UserStore extends PuterStore {
                 cleanEmailValue,
             ],
         );
+
+        for (const row of stripped) {
+            await this.invalidate(this.#normalizeRow(row));
+        }
     }
 
     async invalidate(user: UserRow): Promise<void> {