fix(auth-backend): default catalog presence check to on, rename config
Address review feedback: flip the catalog user existence check to enabled by default and rename the config option to `dangerouslyDisableCatalogPresenceCheck` as an escape hatch. Also use `error.name` instead of `instanceof` for cross-realm error safety. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: MT Lewis <mtlewis@users.noreply.github.com>
This commit is contained in:
Vendored
+7
-4
@@ -134,13 +134,16 @@ export interface Config {
|
||||
*/
|
||||
maxTokensPerUser?: number;
|
||||
/**
|
||||
* Whether to validate that the user's catalog entity exists when
|
||||
* refreshing a token. When enabled, tokens for users removed from
|
||||
* the catalog will be rejected and revoked.
|
||||
* Disables the check that verifies the user's catalog entity still
|
||||
* exists when refreshing a token. This is an escape hatch for
|
||||
* Backstage instances that allow sign-in without a corresponding
|
||||
* catalog user entity. Without the check, refresh tokens for
|
||||
* removed or offboarded users remain valid until they naturally
|
||||
* expire.
|
||||
* @default false
|
||||
* @visibility backend
|
||||
*/
|
||||
validateCatalogUserExistence?: boolean;
|
||||
dangerouslyDisableCatalogPresenceCheck?: boolean;
|
||||
};
|
||||
|
||||
/**
|
||||
|
||||
@@ -41,7 +41,7 @@ import { TokenIssuer } from '../identity/types';
|
||||
export class OfflineAccessService {
|
||||
readonly #offlineSessionDb: OfflineSessionDatabase;
|
||||
readonly #logger: LoggerService;
|
||||
readonly #validateCatalogUserExistence: boolean;
|
||||
readonly #dangerouslyDisableCatalogPresenceCheck: boolean;
|
||||
readonly #catalog: CatalogService;
|
||||
readonly #auth: AuthService;
|
||||
|
||||
@@ -105,9 +105,9 @@ export class OfflineAccessService {
|
||||
);
|
||||
}
|
||||
|
||||
const validateCatalogUserExistence =
|
||||
const dangerouslyDisableCatalogPresenceCheck =
|
||||
config.getOptionalBoolean(
|
||||
'auth.experimentalRefreshToken.validateCatalogUserExistence',
|
||||
'auth.experimentalRefreshToken.dangerouslyDisableCatalogPresenceCheck',
|
||||
) ?? false;
|
||||
|
||||
const knex = await database.getClient();
|
||||
@@ -150,7 +150,7 @@ export class OfflineAccessService {
|
||||
return new OfflineAccessService(
|
||||
offlineSessionDb,
|
||||
logger,
|
||||
validateCatalogUserExistence,
|
||||
dangerouslyDisableCatalogPresenceCheck,
|
||||
options.catalog,
|
||||
options.auth,
|
||||
);
|
||||
@@ -159,13 +159,14 @@ export class OfflineAccessService {
|
||||
private constructor(
|
||||
offlineSessionDb: OfflineSessionDatabase,
|
||||
logger: LoggerService,
|
||||
validateCatalogUserExistence: boolean,
|
||||
dangerouslyDisableCatalogPresenceCheck: boolean,
|
||||
catalog: CatalogService,
|
||||
auth: AuthService,
|
||||
) {
|
||||
this.#offlineSessionDb = offlineSessionDb;
|
||||
this.#logger = logger;
|
||||
this.#validateCatalogUserExistence = validateCatalogUserExistence;
|
||||
this.#dangerouslyDisableCatalogPresenceCheck =
|
||||
dangerouslyDisableCatalogPresenceCheck;
|
||||
this.#catalog = catalog;
|
||||
this.#auth = auth;
|
||||
}
|
||||
@@ -236,7 +237,7 @@ export class OfflineAccessService {
|
||||
throw new AuthenticationError('Invalid refresh token');
|
||||
}
|
||||
|
||||
if (this.#validateCatalogUserExistence) {
|
||||
if (!this.#dangerouslyDisableCatalogPresenceCheck) {
|
||||
try {
|
||||
const entity = await this.#catalog.getEntityByRef(
|
||||
session.userEntityRef,
|
||||
@@ -252,7 +253,7 @@ export class OfflineAccessService {
|
||||
);
|
||||
}
|
||||
} catch (error) {
|
||||
if (error instanceof AuthenticationError) {
|
||||
if (error.name === 'AuthenticationError') {
|
||||
throw error;
|
||||
}
|
||||
this.#logger.warn(
|
||||
|
||||
@@ -171,6 +171,12 @@ describe('OidcRouter', () => {
|
||||
const mockAuth = mockServices.auth.mock();
|
||||
const mockHttpAuth = mockServices.httpAuth.mock();
|
||||
const mockCatalog = catalogServiceMock.mock();
|
||||
mockCatalog.getEntityByRef.mockResolvedValue({
|
||||
apiVersion: 'backstage.io/v1alpha1',
|
||||
kind: 'User',
|
||||
metadata: { name: 'test-user', namespace: 'default' },
|
||||
spec: {},
|
||||
});
|
||||
const mockConfig = mockServices.rootConfig({
|
||||
data: {
|
||||
auth: {
|
||||
@@ -1138,43 +1144,9 @@ describe('OidcRouter', () => {
|
||||
});
|
||||
|
||||
describe('catalog user validation', () => {
|
||||
it('should refresh a token when catalog user exists', async () => {
|
||||
const { server, tokenResponse, mocks } =
|
||||
await doAuthFlowWithOfflineAccess(databaseId, {
|
||||
validateCatalogUserExistence: true,
|
||||
});
|
||||
|
||||
mocks.catalog.getEntityByRef.mockResolvedValueOnce({
|
||||
apiVersion: 'backstage.io/v1alpha1',
|
||||
kind: 'User',
|
||||
metadata: { name: 'test-user', namespace: 'default' },
|
||||
spec: {},
|
||||
});
|
||||
mocks.tokenIssuer.issueToken.mockResolvedValue({
|
||||
token: 'mock-refreshed-token',
|
||||
});
|
||||
|
||||
const refreshResponse = await request(server)
|
||||
.post('/api/auth/v1/token')
|
||||
.send({
|
||||
grant_type: 'refresh_token',
|
||||
refresh_token: tokenResponse.body.refresh_token,
|
||||
})
|
||||
.expect(200);
|
||||
|
||||
expect(refreshResponse.body).toEqual({
|
||||
access_token: 'mock-refreshed-token',
|
||||
token_type: 'Bearer',
|
||||
expires_in: 3600,
|
||||
refresh_token: expect.any(String),
|
||||
});
|
||||
});
|
||||
|
||||
it('should reject refresh when catalog user does not exist', async () => {
|
||||
const { server, tokenResponse, mocks } =
|
||||
await doAuthFlowWithOfflineAccess(databaseId, {
|
||||
validateCatalogUserExistence: true,
|
||||
});
|
||||
await doAuthFlowWithOfflineAccess(databaseId);
|
||||
|
||||
mocks.catalog.getEntityByRef.mockResolvedValueOnce(undefined);
|
||||
|
||||
@@ -1189,9 +1161,7 @@ describe('OidcRouter', () => {
|
||||
|
||||
it('should reject refresh when catalog is unavailable', async () => {
|
||||
const { server, tokenResponse, mocks } =
|
||||
await doAuthFlowWithOfflineAccess(databaseId, {
|
||||
validateCatalogUserExistence: true,
|
||||
});
|
||||
await doAuthFlowWithOfflineAccess(databaseId);
|
||||
|
||||
mocks.catalog.getEntityByRef.mockRejectedValueOnce(
|
||||
new Error('Catalog unavailable'),
|
||||
@@ -1208,9 +1178,7 @@ describe('OidcRouter', () => {
|
||||
|
||||
it('should allow retry after transient catalog failure', async () => {
|
||||
const { server, tokenResponse, mocks } =
|
||||
await doAuthFlowWithOfflineAccess(databaseId, {
|
||||
validateCatalogUserExistence: true,
|
||||
});
|
||||
await doAuthFlowWithOfflineAccess(databaseId);
|
||||
|
||||
mocks.catalog.getEntityByRef.mockRejectedValueOnce(
|
||||
new Error('Catalog unavailable'),
|
||||
@@ -1249,9 +1217,7 @@ describe('OidcRouter', () => {
|
||||
|
||||
it('should not allow retry after user entity not found', async () => {
|
||||
const { server, tokenResponse, mocks } =
|
||||
await doAuthFlowWithOfflineAccess(databaseId, {
|
||||
validateCatalogUserExistence: true,
|
||||
});
|
||||
await doAuthFlowWithOfflineAccess(databaseId);
|
||||
|
||||
mocks.catalog.getEntityByRef.mockResolvedValueOnce(undefined);
|
||||
|
||||
@@ -1280,6 +1246,31 @@ describe('OidcRouter', () => {
|
||||
})
|
||||
.expect(400);
|
||||
});
|
||||
|
||||
it('should skip catalog check when dangerouslyDisableCatalogPresenceCheck is set', async () => {
|
||||
const { server, tokenResponse, mocks } =
|
||||
await doAuthFlowWithOfflineAccess(databaseId, {
|
||||
dangerouslyDisableCatalogPresenceCheck: true,
|
||||
});
|
||||
|
||||
mocks.catalog.getEntityByRef.mockResolvedValueOnce(undefined);
|
||||
mocks.tokenIssuer.issueToken.mockResolvedValue({
|
||||
token: 'mock-refreshed-token',
|
||||
});
|
||||
|
||||
const refreshResponse = await request(server)
|
||||
.post('/api/auth/v1/token')
|
||||
.send({
|
||||
grant_type: 'refresh_token',
|
||||
refresh_token: tokenResponse.body.refresh_token,
|
||||
})
|
||||
.expect(200);
|
||||
|
||||
expect(refreshResponse.body.access_token).toBe(
|
||||
'mock-refreshed-token',
|
||||
);
|
||||
expect(mocks.catalog.getEntityByRef).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
Reference in New Issue
Block a user