Skip to content

Commit f46fac5

Browse files
qoomonqoomon
committed
refactor: simplify access token error creation
1 parent 0dd6c7c commit f46fac5

File tree

1 file changed

+74
-64
lines changed

1 file changed

+74
-64
lines changed

‎server/src/access-token-manager.ts

Lines changed: 74 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -79,27 +79,27 @@ export async function accessTokenManager(options: {
7979
owner: tokenRequest.owner,
8080
});
8181
if (!appInstallation) {
82-
throw new GithubAccessTokenError(createErrorMessage([{
82+
throw new GithubAccessTokenError([{
8383
owner: tokenRequest.owner,
8484
// BE AWARE to prevent leaking owner existence
8585
issues: tokenRequest.owner !== callerIdentity.repository_owner ?
8686
[NOT_AUTHORIZED_MESSAGE] :
8787
[`'${GITHUB_APP.name}' has not been installed. Install from ${GITHUB_APP.html_url}`],
88-
}], effectiveCallerIdentitySubjects));
88+
}], effectiveCallerIdentitySubjects);
8989
}
9090
log.debug({appInstallation}, 'App installation');
9191

9292
const accessPolicyPaths = [...options.accessPolicyLocation.owner.paths, ...options.accessPolicyLocation.repo.paths];
9393
if (!accessPolicyPaths.every((path) => appInstallation.single_file_paths?.includes(path))) {
9494
log.debug({required: accessPolicyPaths, actual: appInstallation.single_file_paths},
9595
`App installation is missing 'single_file' permission for access policy file(s)`);
96-
throw new GithubAccessTokenError(createErrorMessage([{
96+
throw new GithubAccessTokenError([{
9797
owner: tokenRequest.owner,
9898
// BE AWARE to prevent leaking owner existence
9999
issues: callerIdentity.repository !== `${tokenRequest.owner}/${options.accessPolicyLocation.owner.repo}` ?
100100
[NOT_AUTHORIZED_MESSAGE] :
101101
[`'${GITHUB_APP.name}' installation is missing 'single_file' permission for access policy file(s)`],
102-
}], effectiveCallerIdentitySubjects));
102+
}], effectiveCallerIdentitySubjects);
103103
}
104104

105105
// --- verify requested token permissions ------------------------------------------------------------------------
@@ -112,7 +112,7 @@ export async function accessTokenManager(options: {
112112

113113
if (requestedAppInstallationPermissions.denied.length > 0) {
114114
// TODO potential security issue: Do not leak app installation permissions
115-
throw new GithubAccessTokenError(createErrorMessage([{
115+
throw new GithubAccessTokenError([{
116116
owner: tokenRequest.owner,
117117
// BE AWARE to prevent leaking owner existence
118118
issues: callerIdentity.repository !== `${tokenRequest.owner}/${options.accessPolicyLocation.owner.repo}` ?
@@ -121,7 +121,7 @@ export async function accessTokenManager(options: {
121121
scope, permission,
122122
message: `Permission has not been granted to '${GITHUB_APP.name}' installation`,
123123
})),
124-
}], effectiveCallerIdentitySubjects));
124+
}], effectiveCallerIdentitySubjects);
125125
}
126126

127127
const appInstallationClient = await createOctokit(GITHUB_APP_CLIENT, appInstallation, {
@@ -138,14 +138,14 @@ export async function accessTokenManager(options: {
138138
}).catch((error) => {
139139
if (error instanceof GithubAccessPolicyError) {
140140
log.debug({issues: error.issues}, `'${tokenRequest.owner}' access policy`);
141-
throw new GithubAccessTokenError(createErrorMessage([{
141+
throw new GithubAccessTokenError([{
142142
owner: tokenRequest.owner,
143143
// BE AWARE to prevent leaking owner existence
144144
issues: tokenRequest.owner !== callerIdentity.repository_owner ?
145145
[NOT_AUTHORIZED_MESSAGE] :
146146
[formatAccessPolicyError(error)],
147147

148-
}], effectiveCallerIdentitySubjects));
148+
}], effectiveCallerIdentitySubjects);
149149
}
150150
throw error;
151151
});
@@ -158,13 +158,13 @@ export async function accessTokenManager(options: {
158158
[`repo:${tokenRequest.owner}/*:**`]; // e.g., ['repo:qoomon/*:**' ]
159159

160160
if (!matchSubject(allowedSubjects, effectiveCallerIdentitySubjects)) {
161-
throw new GithubAccessTokenError(createErrorMessage([{
161+
throw new GithubAccessTokenError([{
162162
owner: tokenRequest.owner,
163163
// BE AWARE to prevent leaking owner existence
164164
issues: tokenRequest.owner !== callerIdentity.repository_owner ?
165165
[NOT_AUTHORIZED_MESSAGE] :
166166
['OIDC token subject is not allowed by owner access policy'],
167-
}], effectiveCallerIdentitySubjects));
167+
}], effectiveCallerIdentitySubjects);
168168
}
169169

170170
// grant requested permissions explicitly to prevent accidental permission escalation
@@ -182,11 +182,11 @@ export async function accessTokenManager(options: {
182182
// --- verify requested permissions against owner access policy ----------------------------------------------
183183

184184
if (!hasEntries(ownerGrantedPermissions)) {
185-
throw new GithubAccessTokenError(createErrorMessage([{
185+
throw new GithubAccessTokenError([{
186186
owner: tokenRequest.owner,
187187
// BE AWARE to prevent leaking owner existence
188188
issues: [NOT_AUTHORIZED_MESSAGE],
189-
}], effectiveCallerIdentitySubjects));
189+
}], effectiveCallerIdentitySubjects);
190190
}
191191

192192
const requestedOwnerPermissions = verifyPermissions({
@@ -196,13 +196,13 @@ export async function accessTokenManager(options: {
196196

197197
// -- deny permissions
198198
if (requestedOwnerPermissions.denied.length > 0) {
199-
throw new GithubAccessTokenError(createErrorMessage([{
199+
throw new GithubAccessTokenError([{
200200
owner: tokenRequest.owner,
201201
issues: requestedOwnerPermissions.denied.map(({scope, permission}) => ({
202202
scope, permission,
203203
message: NOT_AUTHORIZED_MESSAGE,
204204
})),
205-
}], effectiveCallerIdentitySubjects));
205+
}], effectiveCallerIdentitySubjects);
206206
}
207207

208208
// --- grant permissions
@@ -244,13 +244,13 @@ export async function accessTokenManager(options: {
244244
// -- deny permissions
245245
if (requestedRepositoryPermissions.denied.length > 0) {
246246
// TODO Potential security issue: Do not leak repository existence
247-
throw new GithubAccessTokenError(createErrorMessage([{
247+
throw new GithubAccessTokenError([{
248248
owner: tokenRequest.owner,
249249
issues: requestedRepositoryPermissions.denied.map(({scope, permission}) => ({
250250
scope, permission,
251251
message: NOT_AUTHORIZED_MESSAGE,
252252
})),
253-
}], effectiveCallerIdentitySubjects));
253+
}], effectiveCallerIdentitySubjects);
254254
}
255255
}
256256
}
@@ -349,7 +349,7 @@ export async function accessTokenManager(options: {
349349

350350
if (hasEntries(requestedTokenIssues)) {
351351
// TODO Potential security issue: Do not leak repository existence
352-
throw new GithubAccessTokenError(createErrorMessage(requestedTokenIssues, effectiveCallerIdentitySubjects));
352+
throw new GithubAccessTokenError(requestedTokenIssues, effectiveCallerIdentitySubjects);
353353
}
354354
}
355355

@@ -387,41 +387,13 @@ export async function accessTokenManager(options: {
387387
// --- Access Manager Functions --------------------------------------------------------------------------------------
388388

389389
/**
390-
* Create error message
391-
* @param reasons - error reasons
392-
* @param callerIdentitySubjects - caller identity subjects
393-
* @return error message
390+
* Format access policy error
391+
* @param error - access policy error
392+
* @return formatted error message
394393
*/
395-
function createErrorMessage(
396-
reasons: ({
397-
owner: string,
398-
issues: (string | { scope: string, permission: string, message: string })[],
399-
} | {
400-
owner: string, repo: string,
401-
issues: (string | { scope: string, permission: string, message: string })[],
402-
})[],
403-
callerIdentitySubjects: string[],
404-
): string {
405-
let message = 'Issues:\n' +
406-
reasons.map((reason) => {
407-
let messagePrefix = reason.owner;
408-
if ('repo' in reason && reason.repo) {
409-
messagePrefix += `/${reason.repo}`;
410-
}
411-
return `${messagePrefix}:\n` +
412-
reason.issues.map((issue) => {
413-
if (typeof issue === 'string') {
414-
return issue;
415-
}
416-
return `${issue.scope}: ${issue.permission} - ${issue.message}`;
417-
}).map((message) => indent(message, '- ')).join('\n');
418-
}).map((message) => indent(message, '- ')).join('\n');
419-
420-
message += '\n' +
421-
'Effective OIDC token subjects:\n' +
422-
`${callerIdentitySubjects.map((subject) => indent(subject, '- ')).join('\n')}`;
423-
424-
return message;
394+
function formatAccessPolicyError(error: GithubAccessPolicyError) {
395+
return error.message + (!error.issues?.length ? '' : '\n' +
396+
error.issues.map((issue) => indent(issue, '- ')).join('\n'));
425397
}
426398

427399
/**
@@ -816,16 +788,6 @@ function regexpOfSubjectPattern(subjectPattern: string): RegExp {
816788
return RegExp(`^${regexp}$`, 'i');
817789
}
818790

819-
/**
820-
* Format access policy error
821-
* @param error - access policy error
822-
* @return formatted error message
823-
*/
824-
function formatAccessPolicyError(error: GithubAccessPolicyError) {
825-
return error.message + (!error.issues?.length ? '' : '\n' +
826-
error.issues.map((issue) => indent(issue, '- ')).join('\n'));
827-
}
828-
829791
// --- GitHub Functions ----------------------------------------------------------------------------------------------
830792

831793
/**
@@ -941,10 +903,58 @@ async function getRepositoryFileContent(client: Octokit, {
941903
export class GithubAccessTokenError extends Error {
942904
/**
943905
* Creates a new GitHub access token error
944-
* @param msg - error message
906+
* @param reasons - error reasons
907+
* @param callerIdentitySubjects - caller identity subjects
945908
*/
946-
constructor(msg: string) {
947-
super(msg);
909+
constructor(
910+
reasons: ({
911+
owner: string,
912+
issues: (string | { scope: string, permission: string, message: string })[],
913+
} | {
914+
owner: string, repo: string,
915+
issues: (string | { scope: string, permission: string, message: string })[],
916+
})[],
917+
callerIdentitySubjects: string[],
918+
) {
919+
super(createAccessTokenRequestErrorMessage(reasons, callerIdentitySubjects));
920+
921+
/**
922+
* Create error message
923+
* @param reasons - error reasons
924+
* @param callerIdentitySubjects - caller identity subjects
925+
* @return error message
926+
*/
927+
function createAccessTokenRequestErrorMessage(
928+
reasons: ({
929+
owner: string,
930+
issues: (string | { scope: string, permission: string, message: string })[],
931+
} | {
932+
owner: string, repo: string,
933+
issues: (string | { scope: string, permission: string, message: string })[],
934+
})[],
935+
callerIdentitySubjects: string[],
936+
): string {
937+
let message = 'Issues:\n' +
938+
reasons.map((reason) => {
939+
let messagePrefix = reason.owner;
940+
if ('repo' in reason && reason.repo) {
941+
messagePrefix += `/${reason.repo}`;
942+
}
943+
return `${messagePrefix}:\n` +
944+
reason.issues.map((issue) => {
945+
if (typeof issue === 'string') {
946+
return issue;
947+
}
948+
return `${issue.scope}: ${issue.permission} - ${issue.message}`;
949+
}).map((message) => indent(message, '- ')).join('\n');
950+
}).map((message) => indent(message, '- ')).join('\n');
951+
952+
message += '\n' +
953+
'Effective OIDC token subjects:\n' +
954+
`${callerIdentitySubjects.map((subject) => indent(subject, '- ')).join('\n')}`;
955+
956+
return message;
957+
}
948958

949959
Object.setPrototypeOf(this, GithubAccessTokenError.prototype);
950960
}

0 commit comments

Comments
 (0)