Skip to content

refactor(auth): drop the 14 unreachable commenter-auth catch wrappers #449

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mrbobbytables wants to merge 1 commit into
base: main
Choose a base branch
from
+78 −406
Open

refactor(auth): drop the 14 unreachable commenter-auth catch wrappers #449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 2 additions & 10 deletions __tests__/issueCommentTest/approveMilestoneErrorPaths.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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/,
)
})
})
69 changes: 2 additions & 67 deletions __tests__/issueCommentTest/errorPaths.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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)
Expand Down Expand Up @@ -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`)
Expand All @@ -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`)
Expand All @@ -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`)
Expand All @@ -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', () => {
Expand All @@ -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)

Expand All @@ -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)

Expand All @@ -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)
Expand All @@ -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`)
Expand Down
12 changes: 2 additions & 10 deletions __tests__/label/errorPaths.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down
49 changes: 0 additions & 49 deletions __tests__/utils/auth.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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', () => {
Expand Down
Loading
Loading