paulononato/backstage
1
0
Fork 0
Code Issues Pull Requests Actions 30 Packages Projects Releases Wiki Activity

chore: code review comments

Browse Source 
Signed-off-by: benjdlambert <ben@blam.sh>
This commit is contained in:
benjdlambert
2025-09-09 13:17:00 +02:00
parent c9f1fb203a
commit ec6cb6bce2
7 changed files with 247 additions and 177 deletions
Download Patch File Download Diff File Expand all files Collapse all files
plugins/auth-backend/src/database/OidcDatabase.ts
+27 -9
View File
@@ -202,14 +202,24 @@ export class OidcDatabase {
});
}
const [updated] = await this.db<OAuthAuthorizationSessionRow>(
const returnedRows = await this.db<OAuthAuthorizationSessionRow>(
'oauth_authorization_sessions',
)
.where('id', session.id)
.update(updatedFields)
.returning('*');
return this.rowToAuthorizationSession(updated) as AuthorizationSession;
if (returnedRows.length !== 1) {
throw new Error(
`Failed to retrieve updated authorization session with id ${session.id}`,
);
}
const [returnedSession] = returnedRows;
return this.rowToAuthorizationSession(
returnedSession,
) as AuthorizationSession;
}
async getAuthorizationSession({ id }: { id: string }) {
@@ -290,17 +300,25 @@ export class OidcDatabase {
});
}
const [updated] = await this.db<OidcAuthorizationCodeRow>(
const returnedRows = await this.db<OidcAuthorizationCodeRow>(
'oidc_authorization_codes',
)
.where('code', authorizationCode.code)
.update(updatedFields)
.returning('*');
return this.rowToAuthorizationCode(updated) as AuthorizationCode;
if (returnedRows.length !== 1) {
throw new Error(
`Failed to retrieve updated authorization code with code ${authorizationCode.code}`,
);
}
const [returnedCode] = returnedRows;
return this.rowToAuthorizationCode(returnedCode) as AuthorizationCode;
}
private rowToClient(row: Partial<OidcClientRow>): Partial<Client> {
private rowToClient(row: OidcClientRow): Client {
return {
clientId: row.client_id,
clientName: row.client_name,
@@ -332,12 +350,12 @@ export class OidcDatabase {
code_challenge_method: session.codeChallengeMethod,
nonce: session.nonce,
status: session.status,
expires_at: session.expiresAt,
expires_at: toDate(session.expiresAt),
};
}
private rowToAuthorizationSession(
row: Partial<OAuthAuthorizationSessionRow>,
row: OAuthAuthorizationSessionRow,
): Partial<AuthorizationSession> {
return {
id: row.id,
@@ -361,13 +379,13 @@ export class OidcDatabase {
return {
code: authorizationCode.code,
session_id: authorizationCode.sessionId,
expires_at: authorizationCode.expiresAt,
expires_at: toDate(authorizationCode.expiresAt),
used: authorizationCode.used,
};
}
private rowToAuthorizationCode(
row: Partial<OidcAuthorizationCodeRow>,
row: OidcAuthorizationCodeRow,
): Partial<AuthorizationCode> {
return {
code: row.code,
plugins/auth-backend/src/migrations.test.ts
+140
View File
@@ -186,4 +186,144 @@ describe('migrations', () => {
await knex.destroy();
},
);
it.each(databases.eachSupportedId())(
'20250909120000_oidc_client_registration.js, %p',
async databaseId => {
const knex = await databases.init(databaseId);
await migrateUntilBefore(
knex,
'20250909120000_oidc_client_registration.js',
);
await migrateUpOnce(knex);
await knex
.insert({
client_id: 'test-client-id',
client_secret: 'test-client-secret',
client_name: 'Test Client',
response_types: JSON.stringify(['code']),
grant_types: JSON.stringify(['authorization_code']),
redirect_uris: JSON.stringify(['https://example.com/callback']),
scope: 'openid profile',
metadata: JSON.stringify({ description: 'Test client' }),
})
.into('oidc_clients');
await expect(
knex('oidc_clients').where('client_id', 'test-client-id').first(),
).resolves.toEqual({
client_id: 'test-client-id',
client_secret: 'test-client-secret',
client_name: 'Test Client',
response_types: JSON.stringify(['code']),
grant_types: JSON.stringify(['authorization_code']),
redirect_uris: JSON.stringify(['https://example.com/callback']),
scope: 'openid profile',
metadata: JSON.stringify({ description: 'Test client' }),
});
await knex
.insert({
id: 'test-session-id',
client_id: 'test-client-id',
user_entity_ref: 'user:default/test-user',
redirect_uri: 'https://example.com/callback',
scope: 'openid',
state: 'test-state',
response_type: 'code',
code_challenge: 'test-challenge',
code_challenge_method: 'S256',
nonce: 'test-nonce',
status: 'pending',
expires_at: new Date(Date.now() + 3600000),
})
.into('oauth_authorization_sessions');
await expect(
knex('oauth_authorization_sessions')
.where('id', 'test-session-id')
.first(),
).resolves.toEqual(
expect.objectContaining({
id: 'test-session-id',
client_id: 'test-client-id',
user_entity_ref: 'user:default/test-user',
redirect_uri: 'https://example.com/callback',
scope: 'openid',
state: 'test-state',
response_type: 'code',
code_challenge: 'test-challenge',
code_challenge_method: 'S256',
nonce: 'test-nonce',
status: 'pending',
}),
);
await knex
.insert({
code: 'test-auth-code',
session_id: 'test-session-id',
expires_at: new Date(Date.now() + 600000),
used: false,
})
.into('oidc_authorization_codes');
await expect(
knex('oidc_authorization_codes')
.where('code', 'test-auth-code')
.first(),
).resolves.toEqual(
expect.objectContaining({
code: 'test-auth-code',
session_id: 'test-session-id',
}),
);
await expect(
knex
.insert({
id: 'invalid-session',
client_id: 'non-existent-client',
redirect_uri: 'https://example.com/callback',
response_type: 'code',
expires_at: new Date(),
})
.into('oauth_authorization_sessions'),
).rejects.toThrow();
await expect(
knex
.insert({
code: 'invalid-code',
session_id: 'non-existent-session',
expires_at: new Date(),
})
.into('oidc_authorization_codes'),
).rejects.toThrow();
await knex('oauth_authorization_sessions')
.where('id', 'test-session-id')
.del();
await expect(
knex('oidc_authorization_codes').where('session_id', 'test-session-id'),
).resolves.toHaveLength(0);
await migrateDownOnce(knex);
const tables = [
'oidc_clients',
'oauth_authorization_sessions',
'oidc_authorization_codes',
];
for (const table of tables) {
await expect(knex.schema.hasTable(table)).resolves.toBe(false);
}
await knex.destroy();
},
);
});
plugins/auth-backend/src/service/OidcRouter.test.ts
+20 -29
View File
@@ -24,6 +24,7 @@ import {
startTestBackend,
TestDatabases,
TestDatabaseId,
mockCredentials,
} from '@backstage/backend-test-utils';
import request from 'supertest';
import crypto from 'crypto';
@@ -413,13 +414,9 @@ describe('OidcRouter', () => {
],
});
httpAuth.credentials.mockResolvedValueOnce({
principal: {
type: 'user',
userEntityRef: 'user:default/test-user',
},
$$type: '@backstage/BackstageCredentials',
});
httpAuth.credentials.mockResolvedValueOnce(
mockCredentials.user('user:default/test-user'),
);
auth.isPrincipal.mockReturnValueOnce(true);
@@ -437,7 +434,7 @@ describe('OidcRouter', () => {
it('should reject auth session', async () => {
const {
mocks: { service },
mocks: { service, httpAuth, auth },
router,
} = await createRouter(databaseId);
@@ -457,6 +454,12 @@ describe('OidcRouter', () => {
state: 'test-state',
});
httpAuth.credentials.mockResolvedValueOnce(
mockCredentials.user('user:default/test-user'),
);
auth.isPrincipal.mockReturnValueOnce(true);
const { server } = await startTestBackend({
features: [
createBackendPlugin({
@@ -496,13 +499,9 @@ describe('OidcRouter', () => {
router,
} = await createRouter(databaseId);
httpAuth.credentials.mockResolvedValueOnce({
principal: {
type: 'user',
userEntityRef: 'user:default/test-user',
},
$$type: '@backstage/BackstageCredentials',
});
httpAuth.credentials.mockResolvedValueOnce(
mockCredentials.user('user:default/test-user'),
);
auth.isPrincipal.mockReturnValueOnce(true);
@@ -590,13 +589,9 @@ describe('OidcRouter', () => {
token: 'mock-access-token-pkce',
});
httpAuth.credentials.mockResolvedValueOnce({
principal: {
type: 'user',
userEntityRef: 'user:default/test-user-pkce',
},
$$type: '@backstage/BackstageCredentials',
});
httpAuth.credentials.mockResolvedValueOnce(
mockCredentials.user('user:default/test-user-pkce'),
);
auth.isPrincipal.mockReturnValueOnce(true);
@@ -725,13 +720,9 @@ describe('OidcRouter', () => {
token: 'mock-access-token-s256',
});
httpAuth.credentials.mockResolvedValueOnce({
principal: {
type: 'user',
userEntityRef: 'user:default/test-user-s256',
},
$$type: '@backstage/BackstageCredentials',
});
httpAuth.credentials.mockResolvedValueOnce(
mockCredentials.user('user:default/test-user-s256'),
);
auth.isPrincipal.mockReturnValueOnce(true);
plugins/auth-backend/src/service/OidcRouter.ts
+28 -9
View File
@@ -134,19 +134,19 @@ export class OidcRouter {
clientId: clientId as string,
redirectUri: redirectUri as string,
responseType: responseType as string,
scope: scope as string,
state: state as string,
nonce: nonce as string,
codeChallenge: codeChallenge as string,
codeChallengeMethod: codeChallengeMethod as string,
scope: scope as string | undefined,
state: state as string | undefined,
nonce: nonce as string | undefined,
codeChallenge: codeChallenge as string | undefined,
codeChallengeMethod: codeChallengeMethod as string | undefined,
});
// todo(blam): maybe this URL could be overridable by config if
// the plugin is mounted somewhere else?
// support slashes in baseUrl?
const authSessionRedirectUrl = new URL(
`/oauth2/authorize/${result.id}`,
this.appUrl,
`./oauth2/authorize/${result.id}`,
ensureTrailingSlash(this.appUrl),
);
return res.redirect(authSessionRedirectUrl.toString());
@@ -171,7 +171,7 @@ export class OidcRouter {
});
// Authorization Session request details endpoint
// Returns Authorization Session request details for the frontned
// Returns Authorization Session request details for the frontend
router.get('/v1/sessions/:sessionId', async (req, res) => {
const { sessionId } = req.params;
@@ -263,12 +263,25 @@ export class OidcRouter {
});
}
const httpCredentials = await this.httpAuth.credentials(req);
if (!this.auth.isPrincipal(httpCredentials, 'user')) {
return res.status(401).json({
error: 'unauthorized',
error_description: 'Authentication required',
});
}
const { userEntityRef } = httpCredentials.principal;
try {
const session = await this.oidc.getAuthorizationSession({
sessionId,
});
await this.oidc.rejectAuthorizationSession({ sessionId });
await this.oidc.rejectAuthorizationSession({
sessionId,
userEntityRef,
});
const errorParams = new URLSearchParams();
errorParams.append('error', 'access_denied');
@@ -413,3 +426,9 @@ export class OidcRouter {
return router;
}
}
function ensureTrailingSlash(appUrl: string): string | URL | undefined {
if (appUrl.endsWith('/')) {
return appUrl;
}
return `${appUrl}/`;
}
plugins/auth-backend/src/service/OidcService.test.ts
+25 -42
View File
@@ -466,6 +466,7 @@ describe('OidcService', () => {
await service.rejectAuthorizationSession({
sessionId: authSession.id,
userEntityRef: 'user:default/test',
});
await expect(
@@ -553,6 +554,7 @@ describe('OidcService', () => {
await service.rejectAuthorizationSession({
sessionId: authSession.id,
userEntityRef: 'user:default/test',
});
await expect(
@@ -580,6 +582,7 @@ describe('OidcService', () => {
await service.rejectAuthorizationSession({
sessionId: authSession.id,
userEntityRef: 'user:default/test',
});
await expect(
@@ -595,6 +598,7 @@ describe('OidcService', () => {
await expect(
service.rejectAuthorizationSession({
sessionId: 'invalid-session',
userEntityRef: 'user:default/test',
}),
).rejects.toThrow('Invalid authorization session');
});
@@ -621,6 +625,7 @@ describe('OidcService', () => {
await expect(
service.rejectAuthorizationSession({
sessionId: authSession.id,
userEntityRef: 'user:default/test',
}),
).rejects.toThrow('Authorization session not found or expired');
});
@@ -641,49 +646,15 @@ describe('OidcService', () => {
await service.rejectAuthorizationSession({
sessionId: authSession.id,
userEntityRef: 'user:default/test',
});
await expect(
service.rejectAuthorizationSession({
sessionId: authSession.id,
}),
).rejects.toThrow('Authorization session not found or expired');
});
});
describe('authorize', () => {
it('should create direct authorization', async () => {
const { service } = await createOidcService(databaseId);
const client = await service.registerClient({
clientName: 'Test Client',
redirectUris: ['https://example.com/callback'],
});
const result = await service.authorize({
clientId: client.clientId,
redirectUri: 'https://example.com/callback',
responseType: 'code',
userEntityRef: 'user:default/test',
state: 'test-state',
});
expect(result.redirectUrl).toMatch(
/^https:\/\/example\.com\/callback\?code=.+&state=test-state$/,
);
});
it('should throw error for invalid client', async () => {
const { service } = await createOidcService(databaseId);
await expect(
service.authorize({
clientId: 'invalid-client',
redirectUri: 'https://example.com/callback',
responseType: 'code',
userEntityRef: 'user:default/test',
}),
).rejects.toThrow('Invalid client_id');
).rejects.toThrow('Authorization session not found or expired');
});
});
@@ -698,14 +669,18 @@ describe('OidcService', () => {
redirectUris: ['https://example.com/callback'],
});
const authResult = await service.authorize({
const authSession = await service.createAuthorizationSession({
clientId: client.clientId,
redirectUri: 'https://example.com/callback',
responseType: 'code',
userEntityRef: 'user:default/test',
scope: 'openid',
});
const authResult = await service.approveAuthorizationSession({
sessionId: authSession.id,
userEntityRef: 'user:default/test',
});
const code = new URL(authResult.redirectUrl).searchParams.get('code')!;
const tokenResult = await service.exchangeCodeForToken({
@@ -751,15 +726,19 @@ describe('OidcService', () => {
.update(codeVerifier)
.digest('base64url');
const authResult = await service.authorize({
const authSession = await service.createAuthorizationSession({
clientId: client.clientId,
redirectUri: 'https://example.com/callback',
responseType: 'code',
userEntityRef: 'user:default/test',
codeChallenge,
codeChallengeMethod: 'S256',
});
const authResult = await service.approveAuthorizationSession({
sessionId: authSession.id,
userEntityRef: 'user:default/test',
});
const code = new URL(authResult.redirectUrl).searchParams.get('code')!;
const tokenResult = await service.exchangeCodeForToken({
@@ -781,15 +760,19 @@ describe('OidcService', () => {
});
const codeChallenge = 'test-challenge';
const authResult = await service.authorize({
const authSession = await service.createAuthorizationSession({
clientId: client.clientId,
redirectUri: 'https://example.com/callback',
responseType: 'code',
userEntityRef: 'user:default/test',
codeChallenge,
codeChallengeMethod: 'S256',
});
const authResult = await service.approveAuthorizationSession({
sessionId: authSession.id,
userEntityRef: 'user:default/test',
});
const code = new URL(authResult.redirectUrl).searchParams.get('code')!;
await expect(
plugins/auth-backend/src/service/OidcService.ts
+7 -88
View File
@@ -300,9 +300,14 @@ export class OidcService {
};
}
public async rejectAuthorizationSession(opts: { sessionId: string }) {
public async rejectAuthorizationSession(opts: {
sessionId: string;
userEntityRef: string;
}) {
const { sessionId, userEntityRef } = opts;
const session = await this.oidc.getAuthorizationSession({
id: opts.sessionId,
id: sessionId,
});
if (!session) {
@@ -320,94 +325,8 @@ export class OidcService {
await this.oidc.updateAuthorizationSession({
id: session.id,
status: 'rejected',
});
}
public async authorize(opts: {
clientId: string;
redirectUri: string;
responseType: string;
scope?: string;
state?: string;
nonce?: string;
codeChallenge?: string;
codeChallengeMethod?: string;
userEntityRef: string;
}) {
const {
clientId,
redirectUri,
responseType,
scope,
state,
nonce,
codeChallenge,
codeChallengeMethod,
userEntityRef,
} = opts;
if (responseType !== 'code') {
throw new InputError('Only authorization code flow is supported');
}
const client = await this.oidc.getClient({ clientId });
if (!client) {
throw new InputError('Invalid client_id');
}
if (!client.redirectUris.includes(redirectUri)) {
throw new InputError('Invalid redirect_uri');
}
if (codeChallenge) {
if (
!codeChallengeMethod ||
!['S256', 'plain'].includes(codeChallengeMethod)
) {
throw new InputError('Invalid code_challenge_method');
}
}
const sessionId = crypto.randomUUID();
const sessionExpiresAt = DateTime.now().plus({ hours: 1 }).toJSDate();
await this.oidc.createAuthorizationSession({
id: sessionId,
clientId,
userEntityRef,
redirectUri,
responseType,
scope,
state,
codeChallenge,
codeChallengeMethod,
nonce,
expiresAt: sessionExpiresAt,
});
await this.oidc.updateAuthorizationSession({
id: sessionId,
status: 'approved',
});
const authorizationCode = crypto.randomBytes(32).toString('base64url');
const codeExpiresAt = DateTime.now().plus({ minutes: 10 }).toJSDate();
await this.oidc.createAuthorizationCode({
code: authorizationCode,
sessionId,
expiresAt: codeExpiresAt,
});
const redirectUrl = new URL(redirectUri);
redirectUrl.searchParams.append('code', authorizationCode);
if (state) {
redirectUrl.searchParams.append('state', state);
}
return {
redirectUrl: redirectUrl.toString(),
};
}
public async exchangeCodeForToken(params: {