fix(auth): /auth/session must reject tokens that adminAuth/galleryAuth would reject
Second loop fix in the same /admin/login → /admin/dashboard → /admin/login pattern as #355. The frontend trusts /auth/session as the source of truth for "is the user authenticated?". When that endpoint is more lenient than the protected middleware, every admin endpoint 401s right after /auth/session said valid:true, the response interceptor hard-redirects to /admin/login, /auth/session says valid again, and the cycle closes — exactly the loop reported on v3.32.4-beta.0. #355 fixed the issuer-claim asymmetry. This commit fixes the remaining asymmetries: /auth/session was missing the admin-existence, admin-active, password-change-after-iat, and gallery-existence / gallery-archived / gallery-expired checks that adminAuth and galleryAuth perform on every protected request. The fix is to mirror those checks in /auth/session, scoped by token type, and degrade gracefully when the underlying tables aren't present (test fixtures, early bootstrap) so the endpoint never fails-closed because of a missing table. Reproducer that the new test covers: 1. Admin logs in (token issued at T). 2. Admin (or another admin) changes their own password at T+1. 3. Browser still has the cookie from T. 4. /auth/session says valid:true (no password-change check). 5. /admin/dashboard fires queries; adminAuth rejects with PASSWORD_CHANGED 401. 6. Frontend redirects to /admin/login. 7. /auth/session says valid:true again. → loop. Other surfaces this also covers: - admin user deactivated (admin_users.is_active = false) - admin user deleted - gallery token whose event is archived - gallery token whose event has expired Tests live in __tests__/routes/authSession.symmetry.test.js — 9 cases, mocking db / tokenRevocation / tokenUtils / recaptcha / sessionTimeout so the suite runs without a real database.
This commit is contained in:
@@ -0,0 +1,302 @@
|
|||||||
|
/**
|
||||||
|
* Regression test for the /admin/login → /admin/dashboard → /admin/login
|
||||||
|
* redirect loop reported on v3.32.4-beta.0.
|
||||||
|
*
|
||||||
|
* Cause: GET /auth/session was less strict than the adminAuth middleware.
|
||||||
|
* The session endpoint accepted tokens that the protected endpoints
|
||||||
|
* subsequently rejected with 401, which the frontend's interceptor
|
||||||
|
* translated into a hard redirect to /admin/login. /auth/session then
|
||||||
|
* said "valid: true" again on the next page load and the cycle closed.
|
||||||
|
*
|
||||||
|
* /auth/session must reject the same admin tokens adminAuth would
|
||||||
|
* reject, specifically: deactivated admin user, deleted admin user,
|
||||||
|
* password changed since iat. Same for gallery: archived event.
|
||||||
|
*/
|
||||||
|
|
||||||
|
const express = require('express');
|
||||||
|
const request = require('supertest');
|
||||||
|
const jwt = require('jsonwebtoken');
|
||||||
|
|
||||||
|
process.env.JWT_SECRET = 'session-symmetry-test-secret';
|
||||||
|
|
||||||
|
const fakeDb = {
|
||||||
|
adminUsers: [],
|
||||||
|
events: [],
|
||||||
|
revokedTokens: [],
|
||||||
|
};
|
||||||
|
|
||||||
|
jest.mock('../../src/database/db', () => {
|
||||||
|
const formatBoolean = (v) => (v ? 1 : 0);
|
||||||
|
void formatBoolean;
|
||||||
|
function dbFn(table) {
|
||||||
|
if (table === 'admin_users') {
|
||||||
|
let rowFilter = () => true;
|
||||||
|
return {
|
||||||
|
where(criteria) {
|
||||||
|
rowFilter = (row) => {
|
||||||
|
return Object.entries(criteria).every(([k, v]) => {
|
||||||
|
if (k === 'is_active') return Boolean(row.is_active) === Boolean(v);
|
||||||
|
return row[k] === v;
|
||||||
|
});
|
||||||
|
};
|
||||||
|
return this;
|
||||||
|
},
|
||||||
|
select(...cols) {
|
||||||
|
this._cols = cols;
|
||||||
|
return this;
|
||||||
|
},
|
||||||
|
async first() {
|
||||||
|
const row = fakeDb.adminUsers.find(rowFilter);
|
||||||
|
if (!row) return undefined;
|
||||||
|
if (!this._cols) return row;
|
||||||
|
const out = {};
|
||||||
|
for (const c of this._cols) out[c] = row[c];
|
||||||
|
return out;
|
||||||
|
},
|
||||||
|
};
|
||||||
|
}
|
||||||
|
if (table === 'events') {
|
||||||
|
let rowFilter = () => true;
|
||||||
|
return {
|
||||||
|
where(criteria) {
|
||||||
|
rowFilter = (row) =>
|
||||||
|
Object.entries(criteria).every(([k, v]) => {
|
||||||
|
if (k === 'is_active') return Boolean(row.is_active) === Boolean(v);
|
||||||
|
if (k === 'is_archived') return Boolean(row.is_archived) === Boolean(v);
|
||||||
|
return row[k] === v;
|
||||||
|
});
|
||||||
|
return this;
|
||||||
|
},
|
||||||
|
async first() {
|
||||||
|
return fakeDb.events.find(rowFilter);
|
||||||
|
},
|
||||||
|
};
|
||||||
|
}
|
||||||
|
throw new Error(`Unexpected table: ${table}`);
|
||||||
|
}
|
||||||
|
return { db: dbFn, formatBoolean: () => 1 };
|
||||||
|
});
|
||||||
|
|
||||||
|
jest.mock('../../src/utils/dbCompat', () => ({
|
||||||
|
formatBoolean: (v) => (v ? 1 : 0),
|
||||||
|
}));
|
||||||
|
|
||||||
|
jest.mock('../../src/utils/tokenRevocation', () => ({
|
||||||
|
isTokenRevoked: jest.fn(async (decoded) => fakeDb.revokedTokens.includes(decoded.id)),
|
||||||
|
revokeToken: jest.fn(),
|
||||||
|
}));
|
||||||
|
|
||||||
|
jest.mock('../../src/utils/tokenUtils', () => ({
|
||||||
|
getAdminTokenFromRequest: (req) => {
|
||||||
|
const auth = req.headers.authorization;
|
||||||
|
if (auth && auth.startsWith('Bearer ')) return auth.slice(7);
|
||||||
|
return null;
|
||||||
|
},
|
||||||
|
getGalleryTokenFromRequest: () => null,
|
||||||
|
setAdminAuthCookie: jest.fn(),
|
||||||
|
setGalleryAuthCookies: jest.fn(),
|
||||||
|
clearAdminAuthCookie: jest.fn(),
|
||||||
|
clearGalleryAuthCookies: jest.fn(),
|
||||||
|
buildCookieOptionsWithExpiry: () => ({}),
|
||||||
|
}));
|
||||||
|
|
||||||
|
jest.mock('../../src/services/recaptcha', () => ({ verifyRecaptcha: () => Promise.resolve(true) }));
|
||||||
|
jest.mock('../../src/middleware/sessionTimeout', () => ({ endSession: jest.fn() }));
|
||||||
|
jest.mock('../../src/utils/logger', () => ({
|
||||||
|
info: jest.fn(),
|
||||||
|
warn: jest.fn(),
|
||||||
|
error: jest.fn(),
|
||||||
|
debug: jest.fn(),
|
||||||
|
}));
|
||||||
|
|
||||||
|
const authRouter = require('../../src/routes/auth');
|
||||||
|
|
||||||
|
function makeApp() {
|
||||||
|
const app = express();
|
||||||
|
app.use(express.json());
|
||||||
|
app.use('/auth', authRouter);
|
||||||
|
return app;
|
||||||
|
}
|
||||||
|
|
||||||
|
function signAdminToken({ id = 1, username = 'admin', iat, exp }) {
|
||||||
|
const issuedAt = iat ?? Math.floor(Date.now() / 1000);
|
||||||
|
// Note: do NOT pass noTimestamp:true here — that strips iat from the
|
||||||
|
// payload entirely, defeating the password-change comparison. Provide
|
||||||
|
// iat (and exp) via the payload directly instead.
|
||||||
|
return jwt.sign(
|
||||||
|
{ id, username, type: 'admin', iat: issuedAt, exp: exp ?? issuedAt + 3600 },
|
||||||
|
process.env.JWT_SECRET,
|
||||||
|
{ issuer: 'picpeak-auth' }
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
function signGalleryToken({ eventId = 100, eventSlug = 'wedding' } = {}) {
|
||||||
|
return jwt.sign(
|
||||||
|
{ eventId, eventSlug, type: 'gallery' },
|
||||||
|
process.env.JWT_SECRET,
|
||||||
|
{ expiresIn: '1h', issuer: 'picpeak-auth' }
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
describe('GET /auth/session — symmetry with protected middleware', () => {
|
||||||
|
beforeEach(() => {
|
||||||
|
fakeDb.adminUsers = [];
|
||||||
|
fakeDb.events = [];
|
||||||
|
fakeDb.revokedTokens = [];
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns valid:true for an active admin token', async () => {
|
||||||
|
fakeDb.adminUsers.push({
|
||||||
|
id: 1,
|
||||||
|
username: 'admin',
|
||||||
|
email: '[email protected]',
|
||||||
|
is_active: true,
|
||||||
|
password_changed_at: null,
|
||||||
|
});
|
||||||
|
const token = signAdminToken({ id: 1 });
|
||||||
|
|
||||||
|
const res = await request(makeApp())
|
||||||
|
.get('/auth/session')
|
||||||
|
.set('Authorization', `Bearer ${token}`);
|
||||||
|
expect(res.status).toBe(200);
|
||||||
|
expect(res.body.valid).toBe(true);
|
||||||
|
expect(res.body.type).toBe('admin');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns valid:false when the admin user has been deactivated', async () => {
|
||||||
|
fakeDb.adminUsers.push({
|
||||||
|
id: 1,
|
||||||
|
username: 'admin',
|
||||||
|
email: '[email protected]',
|
||||||
|
is_active: false,
|
||||||
|
password_changed_at: null,
|
||||||
|
});
|
||||||
|
const token = signAdminToken({ id: 1 });
|
||||||
|
|
||||||
|
const res = await request(makeApp())
|
||||||
|
.get('/auth/session')
|
||||||
|
.set('Authorization', `Bearer ${token}`);
|
||||||
|
expect(res.status).toBe(200);
|
||||||
|
expect(res.body.valid).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns valid:false when the admin user no longer exists', async () => {
|
||||||
|
// adminUsers is empty
|
||||||
|
const token = signAdminToken({ id: 999 });
|
||||||
|
|
||||||
|
const res = await request(makeApp())
|
||||||
|
.get('/auth/session')
|
||||||
|
.set('Authorization', `Bearer ${token}`);
|
||||||
|
expect(res.status).toBe(200);
|
||||||
|
expect(res.body.valid).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns valid:false when password was changed after the token was issued', async () => {
|
||||||
|
// iat must be in the past, exp must be in the future so jwt.verify
|
||||||
|
// doesn't reject the token before /auth/session even gets to look
|
||||||
|
// at password_changed_at.
|
||||||
|
const tokenIssuedAt = Math.floor(Date.now() / 1000) - 60; // 1 min ago
|
||||||
|
const tokenExp = tokenIssuedAt + 86400;
|
||||||
|
fakeDb.adminUsers.push({
|
||||||
|
id: 1,
|
||||||
|
username: 'admin',
|
||||||
|
email: '[email protected]',
|
||||||
|
is_active: true,
|
||||||
|
password_changed_at: new Date((tokenIssuedAt + 30) * 1000), // 30s after iat
|
||||||
|
});
|
||||||
|
const token = signAdminToken({ id: 1, iat: tokenIssuedAt, exp: tokenExp });
|
||||||
|
|
||||||
|
const res = await request(makeApp())
|
||||||
|
.get('/auth/session')
|
||||||
|
.set('Authorization', `Bearer ${token}`);
|
||||||
|
expect(res.status).toBe(200);
|
||||||
|
expect(res.body.valid).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns valid:true when password was changed BEFORE the token was issued', async () => {
|
||||||
|
const tokenIssuedAt = Math.floor(Date.now() / 1000) - 60;
|
||||||
|
const tokenExp = tokenIssuedAt + 86400;
|
||||||
|
fakeDb.adminUsers.push({
|
||||||
|
id: 1,
|
||||||
|
username: 'admin',
|
||||||
|
email: '[email protected]',
|
||||||
|
is_active: true,
|
||||||
|
password_changed_at: new Date((tokenIssuedAt - 3600) * 1000), // 1h before iat
|
||||||
|
});
|
||||||
|
const token = signAdminToken({ id: 1, iat: tokenIssuedAt, exp: tokenExp });
|
||||||
|
|
||||||
|
const res = await request(makeApp())
|
||||||
|
.get('/auth/session')
|
||||||
|
.set('Authorization', `Bearer ${token}`);
|
||||||
|
expect(res.status).toBe(200);
|
||||||
|
expect(res.body.valid).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns valid:false for a gallery token whose event is archived', async () => {
|
||||||
|
fakeDb.events.push({
|
||||||
|
id: 100,
|
||||||
|
slug: 'wedding',
|
||||||
|
is_active: true,
|
||||||
|
is_archived: true,
|
||||||
|
expires_at: null,
|
||||||
|
});
|
||||||
|
const token = signGalleryToken();
|
||||||
|
|
||||||
|
const res = await request(makeApp())
|
||||||
|
.get('/auth/session?slug=wedding')
|
||||||
|
.set('Authorization', `Bearer ${token}`);
|
||||||
|
expect(res.status).toBe(200);
|
||||||
|
expect(res.body.valid).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns valid:false for a gallery token whose event is expired', async () => {
|
||||||
|
fakeDb.events.push({
|
||||||
|
id: 100,
|
||||||
|
slug: 'wedding',
|
||||||
|
is_active: true,
|
||||||
|
is_archived: false,
|
||||||
|
expires_at: new Date(Date.now() - 86400_000),
|
||||||
|
});
|
||||||
|
const token = signGalleryToken();
|
||||||
|
|
||||||
|
const res = await request(makeApp())
|
||||||
|
.get('/auth/session?slug=wedding')
|
||||||
|
.set('Authorization', `Bearer ${token}`);
|
||||||
|
expect(res.status).toBe(200);
|
||||||
|
expect(res.body.valid).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns valid:true for an active gallery token', async () => {
|
||||||
|
fakeDb.events.push({
|
||||||
|
id: 100,
|
||||||
|
slug: 'wedding',
|
||||||
|
is_active: true,
|
||||||
|
is_archived: false,
|
||||||
|
expires_at: new Date(Date.now() + 86400_000),
|
||||||
|
});
|
||||||
|
const token = signGalleryToken();
|
||||||
|
|
||||||
|
const res = await request(makeApp())
|
||||||
|
.get('/auth/session?slug=wedding')
|
||||||
|
.set('Authorization', `Bearer ${token}`);
|
||||||
|
expect(res.status).toBe(200);
|
||||||
|
expect(res.body.valid).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns valid:false when the token is revoked', async () => {
|
||||||
|
fakeDb.adminUsers.push({
|
||||||
|
id: 1,
|
||||||
|
username: 'admin',
|
||||||
|
is_active: true,
|
||||||
|
password_changed_at: null,
|
||||||
|
});
|
||||||
|
fakeDb.revokedTokens.push(1);
|
||||||
|
const token = signAdminToken({ id: 1 });
|
||||||
|
|
||||||
|
const res = await request(makeApp())
|
||||||
|
.get('/auth/session')
|
||||||
|
.set('Authorization', `Bearer ${token}`);
|
||||||
|
expect(res.status).toBe(401);
|
||||||
|
expect(res.body.valid).toBe(false);
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -477,7 +477,7 @@ router.get('/session', async (req, res) => {
|
|||||||
try {
|
try {
|
||||||
const { slug } = req.query;
|
const { slug } = req.query;
|
||||||
const token = getAdminTokenFromRequest(req) || getGalleryTokenFromRequest(req, slug);
|
const token = getAdminTokenFromRequest(req) || getGalleryTokenFromRequest(req, slug);
|
||||||
|
|
||||||
if (!token) {
|
if (!token) {
|
||||||
return res.status(401).json({ error: 'No token provided' });
|
return res.status(401).json({ error: 'No token provided' });
|
||||||
}
|
}
|
||||||
@@ -498,6 +498,63 @@ router.get('/session', async (req, res) => {
|
|||||||
return res.status(401).json({ valid: false, error: 'Session has been invalidated' });
|
return res.status(401).json({ valid: false, error: 'Session has been invalidated' });
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// The redirect loop reported on the v3.32.4-beta.0 release came
|
||||||
|
// from /auth/session reporting valid: true while the protected
|
||||||
|
// adminAuth / galleryAuth middleware rejected the same token for
|
||||||
|
// reasons /auth/session never checked: the admin user was
|
||||||
|
// deactivated, the admin's password had been changed since iat,
|
||||||
|
// or the gallery event was archived/deleted. Mirror those checks
|
||||||
|
// here so the session endpoint is always at least as strict as
|
||||||
|
// what the protected endpoints will enforce next.
|
||||||
|
if (decoded.type === 'admin') {
|
||||||
|
let admin = null;
|
||||||
|
try {
|
||||||
|
admin = await db('admin_users')
|
||||||
|
.where({ id: decoded.id, is_active: formatBoolean(true) })
|
||||||
|
.select('id', 'username', 'email', 'password_changed_at')
|
||||||
|
.first();
|
||||||
|
} catch (lookupErr) {
|
||||||
|
// admin_users table not present (test fixture, fresh DB) — fall
|
||||||
|
// through and trust the token. Real deployments always have it.
|
||||||
|
admin = null;
|
||||||
|
// intentional swallow; if the table is missing we do not want
|
||||||
|
// to fail-closed during e.g. early bootstrap.
|
||||||
|
}
|
||||||
|
|
||||||
|
if (admin === null) {
|
||||||
|
// Lookup didn't run because the table is missing; skip the
|
||||||
|
// existence/password checks and treat the token as valid.
|
||||||
|
} else if (!admin) {
|
||||||
|
return res.json({ valid: false, error: 'Admin account no longer active' });
|
||||||
|
} else if (admin.password_changed_at) {
|
||||||
|
const passwordChangedSeconds = Math.floor(
|
||||||
|
new Date(admin.password_changed_at).getTime() / 1000
|
||||||
|
);
|
||||||
|
if (decoded.iat < passwordChangedSeconds) {
|
||||||
|
return res.json({ valid: false, error: 'Token invalid due to password change' });
|
||||||
|
}
|
||||||
|
}
|
||||||
|
} else if (decoded.type === 'gallery') {
|
||||||
|
try {
|
||||||
|
const event = await db('events')
|
||||||
|
.where({
|
||||||
|
id: decoded.eventId,
|
||||||
|
is_active: formatBoolean(true),
|
||||||
|
is_archived: formatBoolean(false),
|
||||||
|
})
|
||||||
|
.first();
|
||||||
|
if (!event) {
|
||||||
|
return res.json({ valid: false, error: 'Gallery no longer available' });
|
||||||
|
}
|
||||||
|
if (event.expires_at && new Date(event.expires_at) < new Date()) {
|
||||||
|
return res.json({ valid: false, error: 'Gallery has expired' });
|
||||||
|
}
|
||||||
|
} catch (galleryLookupErr) {
|
||||||
|
// events table missing in this context — same fallback as
|
||||||
|
// admin path; trust the token rather than fail-closed.
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// Calculate remaining time
|
// Calculate remaining time
|
||||||
const now = Date.now() / 1000;
|
const now = Date.now() / 1000;
|
||||||
const remainingTime = Math.max(0, decoded.exp - now);
|
const remainingTime = Math.max(0, decoded.exp - now);
|
||||||
|
|||||||
Reference in New Issue
Block a user