fix(oidc): local-credential lockout, session hydration, split-origin gaps (codex round 3)
- OIDC-owned accounts can never authenticate locally: the password login rejects auth_provider='oidc' rows outright (generic 401), and the super-admin password reset refuses them with a clear message — previously a reset would have minted a local password bypassing the IdP's MFA/access policies - /auth/session now returns a full adminUser payload (role join) and AdminAuthContext hydrates user state from it: an SSO redirect establishes the session without any login JSON, which left the header identity blank and current-admin form defaults empty - the /sso/login error path redirects absolute to the frontend base (same split-origin reasoning as the callback) - docker-compose.yml passes API_URL through to the backend (production compose uses env_file and needs nothing; dev compose is gitignored) - authSession.symmetry test mock taught the joined admin lookup (leftJoin, prefixed columns, aliases) — the route change made the old mock throw, which read as "table missing, trust token" Tests: new case pins that a known-good password on an OIDC-owned row still gets 401. 14/14 OIDC, 13/13 symmetry.
This commit is contained in:
@@ -222,6 +222,21 @@ describe('OIDC SSO (#798)', () => {
|
|||||||
expect(cfg.clientSecret).toBe(idp.clientSecret);
|
expect(cfg.clientSecret).toBe(idp.clientSecret);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('refuses local password login for OIDC-owned accounts', async () => {
|
||||||
|
// Give the JIT admin a KNOWN password hash directly in the DB — the
|
||||||
|
// auth_provider check must reject the login even with valid credentials
|
||||||
|
// (otherwise a password reset would mint an IdP-bypassing local login).
|
||||||
|
await db('admin_users').where({ id: agentCookies.jitAdminId }).update({
|
||||||
|
password_hash: await bcrypt.hash('KnownPass123', 4),
|
||||||
|
});
|
||||||
|
const row = await db('admin_users').where({ id: agentCookies.jitAdminId }).first();
|
||||||
|
|
||||||
|
const res = await request(app)
|
||||||
|
.post('/api/auth/admin/login')
|
||||||
|
.send({ username: row.email, password: 'KnownPass123' });
|
||||||
|
expect(res.status).toBe(401);
|
||||||
|
});
|
||||||
|
|
||||||
it('returns 404 from /sso/login when SSO is disabled', async () => {
|
it('returns 404 from /sso/login when SSO is disabled', async () => {
|
||||||
await oidcService.saveOidcSettings({ oidc_enabled: false });
|
await oidcService.saveOidcSettings({ oidc_enabled: false });
|
||||||
await request(app).get('/api/auth/admin/sso/login').expect(404);
|
await request(app).get('/api/auth/admin/sso/login').expect(404);
|
||||||
|
|||||||
@@ -32,9 +32,17 @@ jest.mock('../../src/database/db', () => {
|
|||||||
if (table === 'admin_users') {
|
if (table === 'admin_users') {
|
||||||
let rowFilter = () => true;
|
let rowFilter = () => true;
|
||||||
return {
|
return {
|
||||||
|
// The session route joins roles for the adminUser payload (#798);
|
||||||
|
// fake rows carry no role fields, so the join is a pass-through.
|
||||||
|
leftJoin() {
|
||||||
|
return this;
|
||||||
|
},
|
||||||
where(criteria) {
|
where(criteria) {
|
||||||
rowFilter = (row) => {
|
rowFilter = (row) => {
|
||||||
return Object.entries(criteria).every(([k, v]) => {
|
return Object.entries(criteria).every(([rawKey, v]) => {
|
||||||
|
// Joined queries prefix columns ('admin_users.id') — the fake
|
||||||
|
// rows use bare names.
|
||||||
|
const k = rawKey.replace(/^admin_users\./, '');
|
||||||
if (k === 'is_active') return Boolean(row.is_active) === Boolean(v);
|
if (k === 'is_active') return Boolean(row.is_active) === Boolean(v);
|
||||||
return row[k] === v;
|
return row[k] === v;
|
||||||
});
|
});
|
||||||
@@ -50,7 +58,12 @@ jest.mock('../../src/database/db', () => {
|
|||||||
if (!row) return undefined;
|
if (!row) return undefined;
|
||||||
if (!this._cols) return row;
|
if (!this._cols) return row;
|
||||||
const out = {};
|
const out = {};
|
||||||
for (const c of this._cols) out[c] = row[c];
|
for (const c of this._cols) {
|
||||||
|
// Support 'table.col' and 'table.col as alias' shapes.
|
||||||
|
const [source, alias] = c.split(/\s+as\s+/i);
|
||||||
|
const bare = source.includes('.') ? source.split('.').pop() : source;
|
||||||
|
out[alias || bare] = row[bare];
|
||||||
|
}
|
||||||
return out;
|
return out;
|
||||||
},
|
},
|
||||||
};
|
};
|
||||||
|
|||||||
@@ -143,8 +143,11 @@ router.post('/admin/login', [
|
|||||||
)
|
)
|
||||||
.first();
|
.first();
|
||||||
|
|
||||||
// Use generic error to prevent user enumeration
|
// Use generic error to prevent user enumeration. OIDC-owned accounts
|
||||||
if (!admin || !await bcrypt.compare(password, admin.password_hash)) {
|
// (#798) never authenticate locally — their random hash is unusable by
|
||||||
|
// design, and the explicit check keeps that true even if a hash ever
|
||||||
|
// gets set through some other path.
|
||||||
|
if (!admin || admin.auth_provider === 'oidc' || !await bcrypt.compare(password, admin.password_hash)) {
|
||||||
await trackFailedAttempt(username, ipAddress, userAgent);
|
await trackFailedAttempt(username, ipAddress, userAgent);
|
||||||
return res.status(401).json({ error: getGenericAuthError() });
|
return res.status(401).json({ error: getGenericAuthError() });
|
||||||
}
|
}
|
||||||
@@ -654,12 +657,22 @@ router.get('/session', async (req, res) => {
|
|||||||
// or the gallery event was archived/deleted. Mirror those checks
|
// or the gallery event was archived/deleted. Mirror those checks
|
||||||
// here so the session endpoint is always at least as strict as
|
// here so the session endpoint is always at least as strict as
|
||||||
// what the protected endpoints will enforce next.
|
// what the protected endpoints will enforce next.
|
||||||
|
// Full user payload for admin sessions — the SSO callback establishes
|
||||||
|
// the session via redirect (no JSON response the SPA could store), so
|
||||||
|
// session restoration must be able to hydrate the user object (#798).
|
||||||
|
let adminUser = null;
|
||||||
|
|
||||||
if (decoded.type === 'admin') {
|
if (decoded.type === 'admin') {
|
||||||
let admin = null;
|
let admin = null;
|
||||||
try {
|
try {
|
||||||
admin = await db('admin_users')
|
admin = await db('admin_users')
|
||||||
.where({ id: decoded.id, is_active: formatBoolean(true) })
|
.leftJoin('roles', 'roles.id', 'admin_users.role_id')
|
||||||
.select('id', 'username', 'email', 'password_changed_at')
|
.where({ 'admin_users.id': decoded.id, 'admin_users.is_active': formatBoolean(true) })
|
||||||
|
.select(
|
||||||
|
'admin_users.id', 'admin_users.username', 'admin_users.email',
|
||||||
|
'admin_users.password_changed_at', 'admin_users.must_change_password',
|
||||||
|
'roles.name as role_name', 'roles.display_name as role_display_name'
|
||||||
|
)
|
||||||
.first();
|
.first();
|
||||||
} catch (lookupErr) {
|
} catch (lookupErr) {
|
||||||
// admin_users table not present (test fixture, fresh DB) — fall
|
// admin_users table not present (test fixture, fresh DB) — fall
|
||||||
@@ -698,6 +711,19 @@ router.get('/session', async (req, res) => {
|
|||||||
// Helper lookup failed (test stub may not export it) — fall through
|
// Helper lookup failed (test stub may not export it) — fall through
|
||||||
// and trust the token. Real deployments always have the middleware.
|
// and trust the token. Real deployments always have the middleware.
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if (admin) {
|
||||||
|
adminUser = {
|
||||||
|
id: admin.id,
|
||||||
|
username: admin.username,
|
||||||
|
email: admin.email,
|
||||||
|
mustChangePassword: admin.must_change_password || false,
|
||||||
|
role: admin.role_name ? {
|
||||||
|
name: admin.role_name,
|
||||||
|
displayName: admin.role_display_name
|
||||||
|
} : null
|
||||||
|
};
|
||||||
|
}
|
||||||
} else if (decoded.type === 'gallery') {
|
} else if (decoded.type === 'gallery') {
|
||||||
try {
|
try {
|
||||||
const event = await db('events')
|
const event = await db('events')
|
||||||
@@ -729,7 +755,11 @@ router.get('/session', async (req, res) => {
|
|||||||
expiresIn: Math.floor(remainingTime),
|
expiresIn: Math.floor(remainingTime),
|
||||||
user: decoded.username || decoded.eventSlug,
|
user: decoded.username || decoded.eventSlug,
|
||||||
eventSlug: decoded.eventSlug,
|
eventSlug: decoded.eventSlug,
|
||||||
adminUsername: decoded.username
|
adminUsername: decoded.username,
|
||||||
|
// Full admin payload (or null) — lets the SPA hydrate its user
|
||||||
|
// state after a redirect-established session (SSO, #798) where no
|
||||||
|
// login JSON response ever reached it.
|
||||||
|
adminUser
|
||||||
});
|
});
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
res.json({
|
res.json({
|
||||||
@@ -894,7 +924,11 @@ router.get('/admin/sso/login', async (req, res) => {
|
|||||||
return res.status(404).json({ error: 'SSO is not enabled' });
|
return res.status(404).json({ error: 'SSO is not enabled' });
|
||||||
}
|
}
|
||||||
logger.error('OIDC login initiation failed', { error: error.message });
|
logger.error('OIDC login initiation failed', { error: error.message });
|
||||||
return res.redirect('/admin/login?sso_error=config');
|
// Absolute like the callback's redirects: in split-origin deployments a
|
||||||
|
// relative path would resolve on the API origin and 404.
|
||||||
|
const { getFrontendBaseUrl } = require('../utils/frontendUrl');
|
||||||
|
const frontendBase = (await getFrontendBaseUrl().catch(() => '')) || '';
|
||||||
|
return res.redirect(`${frontendBase}/admin/login?sso_error=config`);
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
@@ -459,6 +459,13 @@ async function resetAdminPassword(id, resetById) {
|
|||||||
throw new NotFoundError('Admin user', id);
|
throw new NotFoundError('Admin user', id);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// OIDC-owned accounts (#798) have no usable local password by design —
|
||||||
|
// minting one here would hand out a login that bypasses the IdP's MFA
|
||||||
|
// and access policies.
|
||||||
|
if (user.auth_provider === 'oidc') {
|
||||||
|
throw new ValidationError('This account is managed by your identity provider (SSO) — reset the password there.');
|
||||||
|
}
|
||||||
|
|
||||||
const newPassword = generateReadablePassword();
|
const newPassword = generateReadablePassword();
|
||||||
const passwordHash = await bcrypt.hash(newPassword, getBcryptRounds());
|
const passwordHash = await bcrypt.hash(newPassword, getBcryptRounds());
|
||||||
|
|
||||||
|
|||||||
@@ -60,6 +60,9 @@ services:
|
|||||||
- SMTP_PASS=${SMTP_PASS}
|
- SMTP_PASS=${SMTP_PASS}
|
||||||
- EMAIL_FROM=${EMAIL_FROM:[email protected]}
|
- EMAIL_FROM=${EMAIL_FROM:[email protected]}
|
||||||
- FRONTEND_URL=${FRONTEND_URL:-http://localhost:3000}
|
- FRONTEND_URL=${FRONTEND_URL:-http://localhost:3000}
|
||||||
|
# Public API origin for split-origin deployments (#798 SSO redirect_uri).
|
||||||
|
# Empty = same origin as FRONTEND_URL (the standard proxied setup).
|
||||||
|
- API_URL=${API_URL:-}
|
||||||
- ADMIN_URL=${ADMIN_URL:-http://localhost:3001}
|
- ADMIN_URL=${ADMIN_URL:-http://localhost:3001}
|
||||||
- TZ=${TZ:-UTC}
|
- TZ=${TZ:-UTC}
|
||||||
- STORAGE_PATH=/app/storage
|
- STORAGE_PATH=/app/storage
|
||||||
|
|||||||
@@ -50,12 +50,19 @@ export const AdminAuthProvider: React.FC<AdminAuthProviderProps> = ({ children }
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
const response = await api.get<{ valid: boolean; type: string; adminUsername?: string; user?: string }>(
|
const response = await api.get<{ valid: boolean; type: string; adminUsername?: string; user?: string; adminUser?: AdminUser | null }>(
|
||||||
'/auth/session'
|
'/auth/session'
|
||||||
);
|
);
|
||||||
|
|
||||||
if (response.data?.valid && response.data.type === 'admin') {
|
if (response.data?.valid && response.data.type === 'admin') {
|
||||||
setIsAuthenticated(true);
|
setIsAuthenticated(true);
|
||||||
|
// Redirect-established sessions (SSO, #798) never went through
|
||||||
|
// login(), so sessionStorage has no user — hydrate from the
|
||||||
|
// session payload. Server truth also refreshes stale local copies.
|
||||||
|
if (response.data.adminUser) {
|
||||||
|
setUser(response.data.adminUser);
|
||||||
|
sessionStorage.setItem('admin_user', JSON.stringify(response.data.adminUser));
|
||||||
|
}
|
||||||
} else {
|
} else {
|
||||||
sessionStorage.removeItem('admin_user');
|
sessionStorage.removeItem('admin_user');
|
||||||
setIsAuthenticated(false);
|
setIsAuthenticated(false);
|
||||||
|
|||||||
Reference in New Issue
Block a user