From 9e11e3a9b6b21edb65dfb99ad6b8b37af7cf1d86 Mon Sep 17 00:00:00 2001 From: Bob Killen Date: Sun, 11 Oct 2026 09:39:15 -0500 Subject: [PATCH] refactor(auth): drop unreachable commenter-auth catch wrappers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit checkOrgMember, checkCollaborator and checkIssueComments each wrap their whole body in try/catch and resolve false on any failure, so they never reject. The fourteen `try { … } catch (e) { throw new Error('could not check … auth: ' + e) }` wrappers around them — in close, reopen, lock, milestone, retitle, unassign, assign, cc, uncc, labels/remove and in auth.ts (getOrgCollabCommentUsers, checkCommenterAuth) — were dead code kept green only by unit tests that mocked the callee to reject, pinning error messages the action can never emit (including the misspelt "couldn ot check commentor Auth"). Call the probes directly, have getOrgCollabCommentUsers reuse checkCommenterAuth (same three probes in the same order, so identical API traffic), and delete the fifteen mock-only unit cases. Repack dist. Fixes #386 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Bob Killen --- .../approveMilestoneErrorPaths.test.ts | 12 +- __tests__/issueCommentTest/errorPaths.test.ts | 69 +-------- __tests__/label/errorPaths.test.ts | 12 +- __tests__/utils/auth.test.ts | 49 ------ dist/index.js | 143 +++--------------- src/issueComment/assign.ts | 18 +-- src/issueComment/cc.ts | 18 +-- src/issueComment/close.ts | 16 +- src/issueComment/lock.ts | 8 +- src/issueComment/milestone.ts | 8 +- src/issueComment/reopen.ts | 16 +- src/issueComment/retitle.ts | 8 +- src/issueComment/unassign.ts | 18 +-- src/issueComment/uncc.ts | 18 +-- src/labels/remove.ts | 8 +- src/utils/auth.ts | 63 ++------ 16 files changed, 78 insertions(+), 406 deletions(-) diff --git a/__tests__/issueCommentTest/approveMilestoneErrorPaths.test.ts b/__tests__/issueCommentTest/approveMilestoneErrorPaths.test.ts index 7bcc285..482405e 100644 --- a/__tests__/issueCommentTest/approveMilestoneErrorPaths.test.ts +++ b/__tests__/issueCommentTest/approveMilestoneErrorPaths.test.ts @@ -11,8 +11,8 @@ import issueCommentEvent from '../fixtures/issues/issueCommentEvent.json' import pullReqListReviews from '../fixtures/pullReq/pullReqListReviews.json' import * as utils from '../testUtils' -// the auth helpers swallow HTTP failures themselves, so the handlers' own -// "could not check auth" branches are only reachable when the helper rejects +// the auth helpers are stubbed so each error path can be reached +// without routing the membership, collaborator and comment probes vi.mock('../../src/utils/auth', { spy: true }) const server = setupServer() @@ -99,12 +99,4 @@ describe('/milestone error paths', () => { /missing issue number/, ) }) - - it('wraps a failed collaborator check', async () => { - vi.mocked(auth.checkCollaborator).mockRejectedValueOnce(new Error('auth exploded')) - - await expect(milestone(comment('/milestone v1'))).rejects.toThrow( - /could not check commenter auth: Error: auth exploded/, - ) - }) }) diff --git a/__tests__/issueCommentTest/errorPaths.test.ts b/__tests__/issueCommentTest/errorPaths.test.ts index 1991f33..41baf14 100644 --- a/__tests__/issueCommentTest/errorPaths.test.ts +++ b/__tests__/issueCommentTest/errorPaths.test.ts @@ -15,8 +15,8 @@ import * as auth from '../../src/utils/auth' import issueCommentEvent from '../fixtures/issues/issueCommentEvent.json' import * as utils from '../testUtils' -// the auth helpers swallow HTTP failures themselves, so the handlers' own -// "could not check auth" branches are only reachable when the helper rejects +// the auth helpers are stubbed so each error path can be reached +// without routing the membership, collaborator and comment probes vi.mock('../../src/utils/auth', { spy: true }) const server = setupServer() @@ -30,7 +30,6 @@ const issue = `${utils.api}/repos/Codertocat/Hello-World/issues/1` const pull = `${utils.api}/repos/Codertocat/Hello-World/pulls/1` const forbidden = { message: 'Resource not accessible by integration' } const author = issueCommentEvent.issue.user.login -const stranger = 'not-the-author' function comment(body: string, commenter = author) { const payload = structuredClone(issueCommentEvent) @@ -67,14 +66,6 @@ describe('/assign error paths', () => { await expect(assign(comment('/assign'))).rejects.toThrow(/could not self assign/) }) - it('wraps a failed authorization lookup', async () => { - vi.mocked(auth.getOrgCollabCommentUsers).mockRejectedValueOnce(new Error('lookup exploded')) - - await expect(assign(comment('/assign @some-user'))).rejects.toThrow( - /could not get authorized users: Error: lookup exploded/, - ) - }) - it('wraps a failed assignment of authorized users', async () => { vi.mocked(auth.getOrgCollabCommentUsers).mockResolvedValueOnce(['some-user']) failing('post', `${issue}/assignees`) @@ -96,14 +87,6 @@ describe('/unassign error paths', () => { await expect(unassign(comment('/unassign'))).rejects.toThrow(/could not remove assignee/) }) - it('wraps a failed commenter authorization check', async () => { - vi.mocked(auth.checkCommenterAuth).mockRejectedValueOnce(new Error('auth exploded')) - - await expect(unassign(comment('/unassign @some-user'))).rejects.toThrow( - /Error: auth exploded/, - ) - }) - it('wraps a failed unassignment of another user', async () => { vi.mocked(auth.checkCommenterAuth).mockResolvedValueOnce(true) failing('delete', `${issue}/assignees`) @@ -126,14 +109,6 @@ describe('/cc error paths', () => { await expect(cc(comment('/cc'))).rejects.toThrow(/could not self cc/) }) - it('wraps a failed authorization lookup', async () => { - vi.mocked(auth.getOrgCollabCommentUsers).mockRejectedValueOnce(new Error('lookup exploded')) - - await expect(cc(comment('/cc @some-user'))).rejects.toThrow( - /could not get authorized users: Error: lookup exploded/, - ) - }) - it('wraps a failed review request for authorized users', async () => { vi.mocked(auth.getOrgCollabCommentUsers).mockResolvedValueOnce(['some-user']) failing('post', `${pull}/requested_reviewers`) @@ -153,14 +128,6 @@ describe('/uncc error paths', () => { await expect(uncc(comment('/uncc'))).rejects.toThrow(/could not self uncc/) }) - - it('wraps a failed commenter authorization check', async () => { - vi.mocked(auth.checkCommenterAuth).mockRejectedValueOnce(new Error('auth exploded')) - - await expect(uncc(comment('/uncc @some-user'))).rejects.toThrow( - /could not get authorized users: Error: auth exploded/, - ) - }) }) describe('/reopen error paths', () => { @@ -170,14 +137,6 @@ describe('/reopen error paths', () => { ) }) - it('wraps a failed collaborator check for a non-author', async () => { - vi.mocked(auth.checkCollaborator).mockRejectedValueOnce(new Error('auth exploded')) - - await expect(reopen(comment('/reopen', stranger))).rejects.toThrow( - /could not check commentor auth: Error: auth exploded/, - ) - }) - it('wraps a failed reopen', async () => { failing('patch', issue) @@ -192,14 +151,6 @@ describe('/close error paths', () => { ) }) - it('wraps a failed collaborator check for a non-author', async () => { - vi.mocked(auth.checkCollaborator).mockRejectedValueOnce(new Error('auth exploded')) - - await expect(close(comment('/close', stranger))).rejects.toThrow( - /could not check commentor auth: Error: auth exploded/, - ) - }) - it('wraps a failed close', async () => { failing('patch', issue) @@ -214,14 +165,6 @@ describe('/retitle error paths', () => { ) }) - it('wraps a failed collaborator check', async () => { - vi.mocked(auth.checkCollaborator).mockRejectedValueOnce(new Error('auth exploded')) - - await expect(retitle(comment('/retitle new title'))).rejects.toThrow( - /could not check Commentor auth: Error: auth exploded/, - ) - }) - it('wraps a failed title update', async () => { vi.mocked(auth.checkCollaborator).mockResolvedValueOnce(true) failing('patch', issue) @@ -237,14 +180,6 @@ describe('/lock error paths', () => { await expect(lock(commentWithoutIssue('/lock'))).rejects.toThrow(/missing issue number/) }) - it('wraps a failed collaborator check', async () => { - vi.mocked(auth.checkCollaborator).mockRejectedValueOnce(new Error('auth exploded')) - - await expect(lock(comment('/lock'))).rejects.toThrow( - /could not check commenter auth: Error: auth exploded/, - ) - }) - it('wraps a failed lock', async () => { vi.mocked(auth.checkCollaborator).mockResolvedValueOnce(true) failing('put', `${issue}/lock`) diff --git a/__tests__/label/errorPaths.test.ts b/__tests__/label/errorPaths.test.ts index 91a7b9e..8627874 100644 --- a/__tests__/label/errorPaths.test.ts +++ b/__tests__/label/errorPaths.test.ts @@ -14,8 +14,8 @@ import issueCommentEvent from '../fixtures/issues/issueCommentEvent.json' import labelFileContents from '../fixtures/labels/labelFileContentsResp.json' import * as utils from '../testUtils' -// checkCollaborator swallows HTTP failures itself, so /remove's own -// "could not check commenter auth" branch is only reachable when it rejects +// checkCollaborator is stubbed so /remove's own error paths can be +// reached without routing the collaborator probe vi.mock('../../src/utils/auth', { spy: true }) const server = setupServer() @@ -63,14 +63,6 @@ describe('/remove error paths', () => { ) }) - it('wraps a failed commenter authorization check', async () => { - vi.mocked(auth.checkCollaborator).mockRejectedValueOnce(new Error('auth exploded')) - - await expect(remove(comment('/remove some-label'))).rejects.toThrow( - /could not check commenter auth: Error: auth exploded/, - ) - }) - it('wraps a failed label lookup', async () => { vi.mocked(auth.checkCollaborator).mockResolvedValueOnce(true) issueLookupFails() diff --git a/__tests__/utils/auth.test.ts b/__tests__/utils/auth.test.ts index 82d60fe..3f5dfc1 100644 --- a/__tests__/utils/auth.test.ts +++ b/__tests__/utils/auth.test.ts @@ -253,44 +253,6 @@ describe('checkCommenterAuth', () => { await expect(checkCommenterAuth(octokit, context, 1, 'some-user')).resolves.toBe(true) }) - - // Each helper swallows API errors and logs them, so the only way an error - // escapes into checkCommenterAuth's own catch blocks is the logger throwing. - it('wraps an error escaping the org membership check', async () => { - vi.spyOn(core, 'warning').mockImplementation(() => { - throw new Error('logger down') - }) - vi.spyOn(octokit.orgs, 'checkMembershipForUser').mockRejectedValueOnce({ status: 500 }) - - await expect(checkCommenterAuth(octokit, context, 1, 'some-user')).rejects.toThrow( - 'error in checking org member: Error: logger down', - ) - }) - - it('wraps an error escaping the collaborator check', async () => { - vi.spyOn(core, 'warning').mockImplementation(() => { - throw new Error('logger down') - }) - vi.spyOn(octokit.orgs, 'checkMembershipForUser').mockRejectedValueOnce({ status: 404 }) - vi.spyOn(octokit.repos, 'checkCollaborator').mockRejectedValueOnce({ status: 500 }) - - await expect(checkCommenterAuth(octokit, context, 1, 'some-user')).rejects.toThrow( - 'could not check collaborator: Error: logger down', - ) - }) - - it('wraps an error escaping the issue comments check', async () => { - vi.spyOn(core, 'warning').mockImplementation(() => { - throw new Error('logger down') - }) - vi.spyOn(octokit.orgs, 'checkMembershipForUser').mockRejectedValueOnce({ status: 404 }) - vi.spyOn(octokit.repos, 'checkCollaborator').mockRejectedValueOnce({ status: 404 }) - vi.spyOn(octokit.issues, 'listComments').mockRejectedValueOnce({ status: 500 }) - - await expect(checkCommenterAuth(octokit, context, 1, 'some-user')).rejects.toThrow( - 'could not check issue comments: Error: logger down', - ) - }) }) describe('getOrgCollabCommentUsers', () => { @@ -322,17 +284,6 @@ describe('getOrgCollabCommentUsers', () => { getOrgCollabCommentUsers(octokit, context, 1, ['some-user', 'nobody']), ).resolves.toEqual(['some-user']) }) - - it('wraps an error escaping any of the checks', async () => { - vi.spyOn(core, 'warning').mockImplementation(() => { - throw new Error('logger down') - }) - vi.spyOn(octokit.orgs, 'checkMembershipForUser').mockRejectedValueOnce({ status: 500 }) - - await expect( - getOrgCollabCommentUsers(octokit, context, 1, ['some-user']), - ).rejects.toThrow('could not get authorized user: Error: logger down') - }) }) describe('assertAuthorizedByOwnersOrMembership', () => { diff --git a/dist/index.js b/dist/index.js index a087c24..8d73177 100644 --- a/dist/index.js +++ b/dist/index.js @@ -42275,19 +42275,11 @@ async function checkIssueComments(octokit, context, issueNum, user) { */ async function getOrgCollabCommentUsers(octokit, context, issueNum, args) { const toReturn = []; - try { - await Promise.all(args.map(async (arg) => { - const isOrgMember = await checkOrgMember(octokit, context, arg); - const isCollaborator = await checkCollaborator(octokit, context, arg); - const hasCommented = await checkIssueComments(octokit, context, issueNum, arg); - if (isOrgMember || isCollaborator || hasCommented) { - toReturn.push(arg); - } - })); - } - catch (e) { - throw new Error(`could not get authorized user: ${e}`); - } + await Promise.all(args.map(async (arg) => { + if (await checkCommenterAuth(octokit, context, issueNum, arg)) { + toReturn.push(arg); + } + })); return toReturn; } /** @@ -42297,34 +42289,13 @@ async function getOrgCollabCommentUsers(octokit, context, issueNum, args) { * @param octokit - a hydrated github client * @param context - the github actions event context * @param issueNum - the issue or pr number this runtime is associated with - * @param args - the users to check auth on + * @param user - the user to check auth on */ async function checkCommenterAuth(octokit, context, issueNum, user) { - let isOrgMember = false; - let isCollaborator = false; - let hasCommented = false; - try { - isOrgMember = await checkOrgMember(octokit, context, user); - } - catch (e) { - throw new Error(`error in checking org member: ${e}`); - } - try { - isCollaborator = await checkCollaborator(octokit, context, user); - } - catch (e) { - throw new Error(`could not check collaborator: ${e}`); - } - try { - hasCommented = await checkIssueComments(octokit, context, issueNum, user); - } - catch (e) { - throw new Error(`could not check issue comments: ${e}`); - } - if (isOrgMember || isCollaborator || hasCommented) { - return true; - } - return false; + const isOrgMember = await checkOrgMember(octokit, context, user); + const isCollaborator = await checkCollaborator(octokit, context, user); + const hasCommented = await checkIssueComments(octokit, context, issueNum, user); + return isOrgMember || isCollaborator || hasCommented; } /** * When the repository has OWNERS files, use them to authorize the action, @@ -45619,13 +45590,7 @@ async function remove(context = github_context) { } // Only users who: // - are collaborators - let isAuthUser = false; - try { - isAuthUser = await checkCollaborator(octokit, context, commenterId); - } - catch (e) { - throw new Error(`could not check commenter auth: ${e}`); - } + const isAuthUser = await checkCollaborator(octokit, context, commenterId); if (!isAuthUser) { throw new Error(`commenter is not authorized to remove a label. Must be repo collaborator`); } @@ -45823,13 +45788,7 @@ async function assign_assign(context = github_context) { // - are members of the org // - are collaborators // - have previously commented on this issue - let authUsers = []; - try { - authUsers = await getOrgCollabCommentUsers(octokit, context, issueNumber, commentArgs); - } - catch (e) { - throw new Error(`could not get authorized users: ${e}`); - } + const authUsers = await getOrgCollabCommentUsers(octokit, context, issueNumber, commentArgs); switch (authUsers.length) { case 0: throw new Error(`no authorized users found. Only users who are members of the org, are collaborators, or have previously commented on this issue may be assigned`); @@ -45902,13 +45861,7 @@ async function cc(context = github_context) { // - are members of the org // - are collaborators // - have previously commented on this issue - let authUsers = []; - try { - authUsers = await getOrgCollabCommentUsers(octokit, context, pullNumber, commentArgs); - } - catch (e) { - throw new Error(`could not get authorized users: ${e}`); - } + const authUsers = await getOrgCollabCommentUsers(octokit, context, pullNumber, commentArgs); switch (authUsers.length) { case 0: throw new Error(`no authorized users found. Only users who are members of the org, are collaborators, or have previously commented on this issue may be cc'd`); @@ -45971,18 +45924,9 @@ async function close_close(context = github_context) { // - are collaborators // - pass `authorization.close`, which is read only when the first two refuse const isAuthor = commenterId === context.payload.issue?.user?.login; - let isAuthUser = isAuthor; - if (!isAuthor) { - try { - isAuthUser = await checkCollaborator(octokit, context, commenterId); - } - catch (e) { - throw new Error(`could not check commentor auth: ${e}`); - } - } - if (!isAuthUser) { - isAuthUser = await closePolicyAllows(octokit, context, commenterId); - } + const isAuthUser = isAuthor + || (await checkCollaborator(octokit, context, commenterId)) + || (await closePolicyAllows(octokit, context, commenterId)); if (isAuthUser) { const notPlanned = hasKeyword(getCommandArgs('/close', commentBody), 'not-planned'); try { @@ -46038,13 +45982,7 @@ async function lock(context = github_context) { const commentArgs = getCommandArgs('/lock', commentBody); // Only users who: // - are collaborators - let isAuthUser = false; - try { - isAuthUser = await checkCollaborator(octokit, context, commenterId); - } - catch (e) { - throw new Error(`could not check commenter auth: ${e}`); - } + const isAuthUser = await checkCollaborator(octokit, context, commenterId); if (isAuthUser) { let lockReason; if (commentArgs.length > 0) { @@ -46226,13 +46164,7 @@ async function milestone(context = github_context) { } // Only users who: // - are collaborators - let isAuthUser = false; - try { - isAuthUser = await checkCollaborator(octokit, context, commenterId); - } - catch (e) { - throw new Error(`could not check commenter auth: ${e}`); - } + const isAuthUser = await checkCollaborator(octokit, context, commenterId); if (!isAuthUser) { throw new Error(`commenter is not authorized to set a milestone. Must be repo collaborator`); } @@ -46287,18 +46219,9 @@ async function reopen(context = github_context) { // - are collaborators // - pass `authorization.close`, which is read only when the first two refuse const isAuthor = commenterId === context.payload.issue?.user?.login; - let isAuthUser = isAuthor; - if (!isAuthor) { - try { - isAuthUser = await checkCollaborator(octokit, context, commenterId); - } - catch (e) { - throw new Error(`could not check commentor auth: ${e}`); - } - } - if (!isAuthUser) { - isAuthUser = await closePolicyAllows(octokit, context, commenterId); - } + const isAuthUser = isAuthor + || (await checkCollaborator(octokit, context, commenterId)) + || (await closePolicyAllows(octokit, context, commenterId)); if (isAuthUser) { try { await octokit.issues.update({ @@ -46341,13 +46264,7 @@ async function retitle(context = github_context) { } // Only users who: // - are collaborators - let isAuthUser = false; - try { - isAuthUser = await checkCollaborator(octokit, context, commenterId); - } - catch (e) { - throw new Error(`could not check Commentor auth: ${e}`); - } + const isAuthUser = await checkCollaborator(octokit, context, commenterId); if (isAuthUser) { try { await octokit.issues.update({ @@ -46397,13 +46314,7 @@ async function unassign(context = github_context) { } return; } - let isAuthUser = false; - try { - isAuthUser = await checkCommenterAuth(octokit, context, issueNumber, commenterId); - } - catch (e) { - throw new Error(`couldn ot check commentor Auth: ${e}`); - } + const isAuthUser = await checkCommenterAuth(octokit, context, issueNumber, commenterId); if (isAuthUser) { try { await octokit.issues.removeAssignees({ @@ -46453,13 +46364,7 @@ async function uncc(context = github_context) { // - are members of the org // - are collaborators // - have previously commented on this issue - let authUser = false; - try { - authUser = await checkCommenterAuth(octokit, context, pullNumber, commenterId); - } - catch (e) { - throw new Error(`could not get authorized users: ${e}`); - } + const authUser = await checkCommenterAuth(octokit, context, pullNumber, commenterId); if (authUser) { await octokit.pulls.removeRequestedReviewers({ ...context.repo, diff --git a/src/issueComment/assign.ts b/src/issueComment/assign.ts index 9ee0f17..c65e614 100644 --- a/src/issueComment/assign.ts +++ b/src/issueComment/assign.ts @@ -48,18 +48,12 @@ export async function assign(context: Context = github.context): Promise { // - are members of the org // - are collaborators // - have previously commented on this issue - let authUsers: string[] = [] - try { - authUsers = await getOrgCollabCommentUsers( - octokit, - context, - issueNumber, - commentArgs, - ) - } - catch (e) { - throw new Error(`could not get authorized users: ${e}`) - } + const authUsers = await getOrgCollabCommentUsers( + octokit, + context, + issueNumber, + commentArgs, + ) switch (authUsers.length) { case 0: diff --git a/src/issueComment/cc.ts b/src/issueComment/cc.ts index e03b1d3..6d13590 100644 --- a/src/issueComment/cc.ts +++ b/src/issueComment/cc.ts @@ -45,18 +45,12 @@ export async function cc(context: Context = github.context): Promise { // - are members of the org // - are collaborators // - have previously commented on this issue - let authUsers: string[] = [] - try { - authUsers = await getOrgCollabCommentUsers( - octokit, - context, - pullNumber, - commentArgs, - ) - } - catch (e) { - throw new Error(`could not get authorized users: ${e}`) - } + const authUsers = await getOrgCollabCommentUsers( + octokit, + context, + pullNumber, + commentArgs, + ) switch (authUsers.length) { case 0: diff --git a/src/issueComment/close.ts b/src/issueComment/close.ts index cb0553a..6566df9 100644 --- a/src/issueComment/close.ts +++ b/src/issueComment/close.ts @@ -31,18 +31,10 @@ export async function close(context: Context = github.context): Promise { // - are collaborators // - pass `authorization.close`, which is read only when the first two refuse const isAuthor = commenterId === context.payload.issue?.user?.login - let isAuthUser: boolean = isAuthor - if (!isAuthor) { - try { - isAuthUser = await checkCollaborator(octokit, context, commenterId) - } - catch (e) { - throw new Error(`could not check commentor auth: ${e}`) - } - } - if (!isAuthUser) { - isAuthUser = await closePolicyAllows(octokit, context, commenterId) - } + const isAuthUser + = isAuthor + || (await checkCollaborator(octokit, context, commenterId)) + || (await closePolicyAllows(octokit, context, commenterId)) if (isAuthUser) { const notPlanned = hasKeyword(getCommandArgs('/close', commentBody), 'not-planned') diff --git a/src/issueComment/lock.ts b/src/issueComment/lock.ts index 76f853f..53243ef 100644 --- a/src/issueComment/lock.ts +++ b/src/issueComment/lock.ts @@ -39,13 +39,7 @@ export async function lock(context: Context = github.context): Promise { // Only users who: // - are collaborators - let isAuthUser: boolean = false - try { - isAuthUser = await checkCollaborator(octokit, context, commenterId) - } - catch (e) { - throw new Error(`could not check commenter auth: ${e}`) - } + const isAuthUser = await checkCollaborator(octokit, context, commenterId) if (isAuthUser) { let lockReason: LockReason | undefined diff --git a/src/issueComment/milestone.ts b/src/issueComment/milestone.ts index 13dcfdc..1cbe3f6 100644 --- a/src/issueComment/milestone.ts +++ b/src/issueComment/milestone.ts @@ -32,13 +32,7 @@ export async function milestone( // Only users who: // - are collaborators - let isAuthUser: boolean = false - try { - isAuthUser = await checkCollaborator(octokit, context, commenterId) - } - catch (e) { - throw new Error(`could not check commenter auth: ${e}`) - } + const isAuthUser = await checkCollaborator(octokit, context, commenterId) if (!isAuthUser) { throw new Error( diff --git a/src/issueComment/reopen.ts b/src/issueComment/reopen.ts index d22fa6e..441a765 100644 --- a/src/issueComment/reopen.ts +++ b/src/issueComment/reopen.ts @@ -28,18 +28,10 @@ export async function reopen(context: Context = github.context): Promise { // - are collaborators // - pass `authorization.close`, which is read only when the first two refuse const isAuthor = commenterId === context.payload.issue?.user?.login - let isAuthUser: boolean = isAuthor - if (!isAuthor) { - try { - isAuthUser = await checkCollaborator(octokit, context, commenterId) - } - catch (e) { - throw new Error(`could not check commentor auth: ${e}`) - } - } - if (!isAuthUser) { - isAuthUser = await closePolicyAllows(octokit, context, commenterId) - } + const isAuthUser + = isAuthor + || (await checkCollaborator(octokit, context, commenterId)) + || (await closePolicyAllows(octokit, context, commenterId)) if (isAuthUser) { try { diff --git a/src/issueComment/retitle.ts b/src/issueComment/retitle.ts index 9e1203e..b115efa 100644 --- a/src/issueComment/retitle.ts +++ b/src/issueComment/retitle.ts @@ -37,13 +37,7 @@ export async function retitle( // Only users who: // - are collaborators - let isAuthUser: boolean = false - try { - isAuthUser = await checkCollaborator(octokit, context, commenterId) - } - catch (e) { - throw new Error(`could not check Commentor auth: ${e}`) - } + const isAuthUser = await checkCollaborator(octokit, context, commenterId) if (isAuthUser) { try { diff --git a/src/issueComment/unassign.ts b/src/issueComment/unassign.ts index e6ffa08..3e75d97 100644 --- a/src/issueComment/unassign.ts +++ b/src/issueComment/unassign.ts @@ -45,18 +45,12 @@ export async function unassign( return } - let isAuthUser: boolean = false - try { - isAuthUser = await checkCommenterAuth( - octokit, - context, - issueNumber, - commenterId, - ) - } - catch (e) { - throw new Error(`couldn ot check commentor Auth: ${e}`) - } + const isAuthUser = await checkCommenterAuth( + octokit, + context, + issueNumber, + commenterId, + ) if (isAuthUser) { try { diff --git a/src/issueComment/uncc.ts b/src/issueComment/uncc.ts index 6af0b12..fa678dc 100644 --- a/src/issueComment/uncc.ts +++ b/src/issueComment/uncc.ts @@ -44,18 +44,12 @@ export async function uncc(context: Context = github.context): Promise { // - are members of the org // - are collaborators // - have previously commented on this issue - let authUser: boolean = false - try { - authUser = await checkCommenterAuth( - octokit, - context, - pullNumber, - commenterId, - ) - } - catch (e) { - throw new Error(`could not get authorized users: ${e}`) - } + const authUser = await checkCommenterAuth( + octokit, + context, + pullNumber, + commenterId, + ) if (authUser) { await octokit.pulls.removeRequestedReviewers({ diff --git a/src/labels/remove.ts b/src/labels/remove.ts index 91effa7..53abdb4 100644 --- a/src/labels/remove.ts +++ b/src/labels/remove.ts @@ -29,13 +29,7 @@ export async function remove(context: Context = github.context): Promise { // Only users who: // - are collaborators - let isAuthUser: boolean = false - try { - isAuthUser = await checkCollaborator(octokit, context, commenterId) - } - catch (e) { - throw new Error(`could not check commenter auth: ${e}`) - } + const isAuthUser = await checkCollaborator(octokit, context, commenterId) if (!isAuthUser) { throw new Error( diff --git a/src/utils/auth.ts b/src/utils/auth.ts index 6e887fa..db99313 100644 --- a/src/utils/auth.ts +++ b/src/utils/auth.ts @@ -159,27 +159,13 @@ export async function getOrgCollabCommentUsers( ): Promise { const toReturn: string[] = [] - try { - await Promise.all( - args.map(async (arg) => { - const isOrgMember = await checkOrgMember(octokit, context, arg) - const isCollaborator = await checkCollaborator(octokit, context, arg) - const hasCommented = await checkIssueComments( - octokit, - context, - issueNum, - arg, - ) - - if (isOrgMember || isCollaborator || hasCommented) { - toReturn.push(arg) - } - }), - ) - } - catch (e) { - throw new Error(`could not get authorized user: ${e}`) - } + await Promise.all( + args.map(async (arg) => { + if (await checkCommenterAuth(octokit, context, issueNum, arg)) { + toReturn.push(arg) + } + }), + ) return toReturn } @@ -191,7 +177,7 @@ export async function getOrgCollabCommentUsers( * @param octokit - a hydrated github client * @param context - the github actions event context * @param issueNum - the issue or pr number this runtime is associated with - * @param args - the users to check auth on + * @param user - the user to check auth on */ export async function checkCommenterAuth( octokit: Octokit, @@ -199,36 +185,11 @@ export async function checkCommenterAuth( issueNum: number, user: string, ): Promise { - let isOrgMember: boolean = false - let isCollaborator: boolean = false - let hasCommented: boolean = false - - try { - isOrgMember = await checkOrgMember(octokit, context, user) - } - catch (e) { - throw new Error(`error in checking org member: ${e}`) - } - - try { - isCollaborator = await checkCollaborator(octokit, context, user) - } - catch (e) { - throw new Error(`could not check collaborator: ${e}`) - } - - try { - hasCommented = await checkIssueComments(octokit, context, issueNum, user) - } - catch (e) { - throw new Error(`could not check issue comments: ${e}`) - } - - if (isOrgMember || isCollaborator || hasCommented) { - return true - } + const isOrgMember = await checkOrgMember(octokit, context, user) + const isCollaborator = await checkCollaborator(octokit, context, user) + const hasCommented = await checkIssueComments(octokit, context, issueNum, user) - return false + return isOrgMember || isCollaborator || hasCommented } /**