Skip to content

Commit f001cee

Browse files
authored
Fix admin check failing when user lacks membership in unrelated orgs (#35)
A 403 from GitHub's membership API means the user isn't a member of that org — not an infrastructure error. Treat it the same as 404 (return false) instead of throwing. Also stop throwing when any single org check fails, which was discarding successful results for orgs the user can actually administer.
1 parent f4c5652 commit f001cee

2 files changed

Lines changed: 82 additions & 23 deletions

File tree

lib/github/admin-authorization.ts

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ export async function isGitHubOrgAdmin(user: UserWithToken, orgSlug: string): Pr
6262
cache: "no-store",
6363
})
6464

65-
if (membershipRes.status === 404) return false
65+
if (membershipRes.status === 404 || membershipRes.status === 403) return false
6666
if (!membershipRes.ok) {
6767
throw new Error(`Failed GitHub org membership check: ${membershipRes.status}`)
6868
}
@@ -205,10 +205,13 @@ export async function filterInstalledOrganizationsForAdmin<T extends InstalledOr
205205

206206
const failedChecks = orgCheckResults.filter((result) => result.error !== null)
207207
if (failedChecks.length > 0) {
208-
const failedOrgSlugs = failedChecks.map((result) => result.org.githubOrgSlug)
209-
throw new Error(
210-
`GitHub org-admin checks failed for ${failedChecks.length} org(s): ${failedOrgSlugs.join(", ")}`
211-
)
208+
console.warn("[admin-auth] GitHub org-admin checks failed for some orgs", {
209+
userId: user.id,
210+
failed: failedChecks.map((result) => ({
211+
orgSlug: result.org.githubOrgSlug,
212+
error: result.error,
213+
})),
214+
})
212215
}
213216

214217
const authorizedOrgs = results.filter((result) => result.isAdmin).map((result) => result.org)

tests/unit/admin-authorization-extended.test.ts

Lines changed: 74 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,23 @@ describe("isGitHubOrgAdmin", () => {
4646
expect(result).toBe(false)
4747
})
4848

49+
it("returns false on 403 response", async () => {
50+
global.fetch = vi
51+
.fn()
52+
.mockResolvedValue(new Response("Forbidden", { status: 403 })) as typeof global.fetch
53+
54+
const result = await isGitHubOrgAdmin(
55+
{
56+
id: "user_1",
57+
githubId: "1001",
58+
githubUsername: "orgadmin",
59+
githubAccessTokenEncrypted: "enc-token",
60+
},
61+
"fiveonefour"
62+
)
63+
expect(result).toBe(false)
64+
})
65+
4966
it("returns true for active admin", async () => {
5067
global.fetch = vi.fn().mockResolvedValue(
5168
new Response(JSON.stringify({ state: "active", role: "admin" }), {
@@ -217,28 +234,67 @@ describe("filterInstalledOrganizationsForAdmin - additional coverage", () => {
217234
expect(result).toHaveLength(0)
218235
})
219236

220-
it("throws when GitHub org-admin check fails", async () => {
237+
it("treats failed org-admin checks as non-admin instead of throwing", async () => {
221238
vi.stubEnv("NODE_ENV", "production")
222239
global.fetch = vi.fn().mockRejectedValue(new Error("Network error")) as typeof global.fetch
223240

224-
await expect(
225-
filterInstalledOrganizationsForAdmin(
241+
const result = await filterInstalledOrganizationsForAdmin(
242+
{
243+
id: "user_1",
244+
githubId: "1001",
245+
githubUsername: "orgadmin",
246+
githubAccessTokenEncrypted: "enc-token",
247+
},
248+
[
226249
{
227-
id: "user_1",
228-
githubId: "1001",
229-
githubUsername: "orgadmin",
230-
githubAccessTokenEncrypted: "enc-token",
250+
adminUserId: "user_2",
251+
githubOrgSlug: "fiveonefour",
252+
githubAccountType: "organization",
253+
githubAccountId: "2001",
254+
installationId: 12001,
231255
},
232-
[
233-
{
234-
adminUserId: "user_2",
235-
githubOrgSlug: "fiveonefour",
236-
githubAccountType: "organization",
237-
githubAccountId: "2001",
238-
installationId: 12001,
239-
},
240-
]
241-
)
242-
).rejects.toThrow("GitHub org-admin checks failed")
256+
]
257+
)
258+
expect(result).toHaveLength(0)
259+
})
260+
261+
it("returns authorized orgs even when some checks fail", async () => {
262+
vi.stubEnv("NODE_ENV", "production")
263+
global.fetch = vi
264+
.fn()
265+
.mockResolvedValueOnce(new Response("Forbidden", { status: 403 }))
266+
.mockResolvedValueOnce(
267+
new Response(JSON.stringify({ state: "active", role: "admin" }), {
268+
status: 200,
269+
headers: { "Content-Type": "application/json" },
270+
})
271+
) as typeof global.fetch
272+
273+
const result = await filterInstalledOrganizationsForAdmin(
274+
{
275+
id: "user_1",
276+
githubId: "1001",
277+
githubUsername: "orgadmin",
278+
githubAccessTokenEncrypted: "enc-token",
279+
},
280+
[
281+
{
282+
adminUserId: "user_2",
283+
githubOrgSlug: "ChambreSonore",
284+
githubAccountType: "organization",
285+
githubAccountId: "10694701",
286+
installationId: 112308378,
287+
},
288+
{
289+
adminUserId: "user_2",
290+
githubOrgSlug: "514-labs",
291+
githubAccountType: "organization",
292+
githubAccountId: "140028474",
293+
installationId: 112316261,
294+
},
295+
]
296+
)
297+
expect(result).toHaveLength(1)
298+
expect(result[0].githubOrgSlug).toBe("514-labs")
243299
})
244300
})

0 commit comments

Comments
 (0)