diff --git a/.changeset/patch-modernize-check-permissions-utils.md b/.changeset/patch-modernize-check-permissions-utils.md new file mode 100644 index 0000000000..071281928f --- /dev/null +++ b/.changeset/patch-modernize-check-permissions-utils.md @@ -0,0 +1,10 @@ +--- +"gh-aw": patch +--- + +Cleaned and modernized `check_permissions_utils.cjs` and improved test coverage. + +This change modernizes JavaScript patterns (optional chaining, nullish coalescing, +array shorthand), simplifies error handling, and expands unit tests to reach +full coverage for the module. + diff --git a/actions/setup/js/check_permissions_utils.cjs b/actions/setup/js/check_permissions_utils.cjs index 21c1fea2c4..083b4b0566 100644 --- a/actions/setup/js/check_permissions_utils.cjs +++ b/actions/setup/js/check_permissions_utils.cjs @@ -11,8 +11,7 @@ * @returns {string[]} Array of required permission levels */ function parseRequiredPermissions() { - const requiredPermissionsEnv = process.env.GH_AW_REQUIRED_ROLES; - return requiredPermissionsEnv ? requiredPermissionsEnv.split(",").filter(p => p.trim() !== "") : []; + return process.env.GH_AW_REQUIRED_ROLES?.split(",").filter(p => p.trim()) ?? []; } /** @@ -20,8 +19,7 @@ function parseRequiredPermissions() { * @returns {string[]} Array of allowed bot identifiers */ function parseAllowedBots() { - const allowedBotsEnv = process.env.GH_AW_ALLOWED_BOTS; - return allowedBotsEnv ? allowedBotsEnv.split(",").filter(b => b.trim() !== "") : []; + return process.env.GH_AW_ALLOWED_BOTS?.split(",").filter(b => b.trim()) ?? []; } /** @@ -55,17 +53,17 @@ async function checkBotStatus(actor, owner, repo) { return { isBot: true, isActive: true }; } catch (botError) { // If we get a 404, the bot is not installed/active on this repository - if (typeof botError === "object" && botError !== null && "status" in botError && botError.status === 404) { + if (botError?.status === 404) { core.warning(`Bot '${actor}' is not active/installed on ${owner}/${repo}`); return { isBot: true, isActive: false }; } // For other errors, we'll treat as inactive to be safe - const errorMessage = botError instanceof Error ? botError.message : String(botError); + const errorMessage = botError?.message ?? String(botError); core.warning(`Failed to check bot status: ${errorMessage}`); return { isBot: true, isActive: false, error: errorMessage }; } } catch (error) { - const errorMessage = error instanceof Error ? error.message : String(error); + const errorMessage = error?.message ?? String(error); core.warning(`Error checking bot status: ${errorMessage}`); return { isBot: false, isActive: false, error: errorMessage }; } @@ -94,17 +92,17 @@ async function checkRepositoryPermission(actor, owner, repo, requiredPermissions core.info(`Repository permission level: ${permission}`); // Check if user has one of the required permission levels - for (const requiredPerm of requiredPermissions) { - if (permission === requiredPerm || (requiredPerm === "maintainer" && permission === "maintain")) { - core.info(`✅ User has ${permission} access to repository`); - return { authorized: true, permission: permission }; - } + const hasPermission = requiredPermissions.some(requiredPerm => permission === requiredPerm || (requiredPerm === "maintainer" && permission === "maintain")); + + if (hasPermission) { + core.info(`✅ User has ${permission} access to repository`); + return { authorized: true, permission }; } core.warning(`User permission '${permission}' does not meet requirements: ${requiredPermissions.join(", ")}`); - return { authorized: false, permission: permission }; + return { authorized: false, permission }; } catch (repoError) { - const errorMessage = repoError instanceof Error ? repoError.message : String(repoError); + const errorMessage = repoError?.message ?? String(repoError); core.warning(`Repository permission check failed: ${errorMessage}`); return { authorized: false, error: errorMessage }; } diff --git a/actions/setup/js/check_permissions_utils.test.cjs b/actions/setup/js/check_permissions_utils.test.cjs index 05410a67c9..71251ccdc6 100644 --- a/actions/setup/js/check_permissions_utils.test.cjs +++ b/actions/setup/js/check_permissions_utils.test.cjs @@ -28,7 +28,9 @@ global.github = mockGithub; describe("check_permissions_utils", () => { let parseRequiredPermissions; + let parseAllowedBots; let checkRepositoryPermission; + let checkBotStatus; let originalEnv; beforeEach(async () => { @@ -36,21 +38,66 @@ describe("check_permissions_utils", () => { vi.clearAllMocks(); // Store original environment - originalEnv = process.env.GH_AW_REQUIRED_ROLES; + originalEnv = { + GH_AW_REQUIRED_ROLES: process.env.GH_AW_REQUIRED_ROLES, + GH_AW_ALLOWED_BOTS: process.env.GH_AW_ALLOWED_BOTS, + }; // Import the module functions const module = await import("./check_permissions_utils.cjs"); parseRequiredPermissions = module.parseRequiredPermissions; + parseAllowedBots = module.parseAllowedBots; checkRepositoryPermission = module.checkRepositoryPermission; + checkBotStatus = module.checkBotStatus; }); afterEach(() => { // Restore original environment - if (originalEnv !== undefined) { - process.env.GH_AW_REQUIRED_ROLES = originalEnv; - } else { - delete process.env.GH_AW_REQUIRED_ROLES; - } + Object.keys(originalEnv).forEach(key => { + if (originalEnv[key] !== undefined) { + process.env[key] = originalEnv[key]; + } else { + delete process.env[key]; + } + }); + }); + + describe("parseAllowedBots", () => { + it("should parse comma-separated bot identifiers", () => { + process.env.GH_AW_ALLOWED_BOTS = "dependabot[bot],renovate[bot],github-actions[bot]"; + const result = parseAllowedBots(); + expect(result).toEqual(["dependabot[bot]", "renovate[bot]", "github-actions[bot]"]); + }); + + it("should filter out empty strings", () => { + process.env.GH_AW_ALLOWED_BOTS = "dependabot[bot],,renovate[bot],"; + const result = parseAllowedBots(); + expect(result).toEqual(["dependabot[bot]", "renovate[bot]"]); + }); + + it("should filter out whitespace-only entries", () => { + process.env.GH_AW_ALLOWED_BOTS = "dependabot[bot], ,renovate[bot]"; + const result = parseAllowedBots(); + expect(result).toEqual(["dependabot[bot]", "renovate[bot]"]); + }); + + it("should return empty array when env var is not set", () => { + delete process.env.GH_AW_ALLOWED_BOTS; + const result = parseAllowedBots(); + expect(result).toEqual([]); + }); + + it("should return empty array when env var is empty string", () => { + process.env.GH_AW_ALLOWED_BOTS = ""; + const result = parseAllowedBots(); + expect(result).toEqual([]); + }); + + it("should handle single bot identifier", () => { + process.env.GH_AW_ALLOWED_BOTS = "dependabot[bot]"; + const result = parseAllowedBots(); + expect(result).toEqual(["dependabot[bot]"]); + }); }); describe("parseRequiredPermissions", () => { @@ -222,4 +269,109 @@ describe("check_permissions_utils", () => { expect(successLog[0]).toContain("write"); }); }); + + describe("checkBotStatus", () => { + it("should identify bot by [bot] suffix", async () => { + mockGithub.rest.repos.getCollaboratorPermissionLevel.mockResolvedValue({ + data: { permission: "write" }, + }); + + const result = await checkBotStatus("dependabot[bot]", "testowner", "testrepo"); + + expect(result).toEqual({ + isBot: true, + isActive: true, + }); + + expect(mockCore.info).toHaveBeenCalledWith("Checking if bot 'dependabot[bot]' is active on testowner/testrepo"); + expect(mockCore.info).toHaveBeenCalledWith("Bot 'dependabot[bot]' is active with permission level: write"); + }); + + it("should return false for non-bot users", async () => { + const result = await checkBotStatus("regularuser", "testowner", "testrepo"); + + expect(result).toEqual({ + isBot: false, + isActive: false, + }); + + expect(mockGithub.rest.repos.getCollaboratorPermissionLevel).not.toHaveBeenCalled(); + }); + + it("should handle 404 error for inactive bot", async () => { + const apiError = { status: 404, message: "Not Found" }; + mockGithub.rest.repos.getCollaboratorPermissionLevel.mockRejectedValue(apiError); + + const result = await checkBotStatus("renovate[bot]", "testowner", "testrepo"); + + expect(result).toEqual({ + isBot: true, + isActive: false, + }); + + expect(mockCore.warning).toHaveBeenCalledWith("Bot 'renovate[bot]' is not active/installed on testowner/testrepo"); + }); + + it("should handle other API errors", async () => { + const apiError = new Error("API rate limit exceeded"); + mockGithub.rest.repos.getCollaboratorPermissionLevel.mockRejectedValue(apiError); + + const result = await checkBotStatus("github-actions[bot]", "testowner", "testrepo"); + + expect(result).toEqual({ + isBot: true, + isActive: false, + error: "API rate limit exceeded", + }); + + expect(mockCore.warning).toHaveBeenCalledWith("Failed to check bot status: API rate limit exceeded"); + }); + + it("should handle non-Error API failures", async () => { + mockGithub.rest.repos.getCollaboratorPermissionLevel.mockRejectedValue("String error"); + + const result = await checkBotStatus("bot[bot]", "testowner", "testrepo"); + + expect(result).toEqual({ + isBot: true, + isActive: false, + error: "String error", + }); + + expect(mockCore.warning).toHaveBeenCalledWith("Failed to check bot status: String error"); + }); + + it("should handle unexpected errors gracefully", async () => { + // Simulate an error during bot detection + const unexpectedError = new Error("Unexpected error"); + mockGithub.rest.repos.getCollaboratorPermissionLevel.mockImplementation(() => { + throw unexpectedError; + }); + + const result = await checkBotStatus("test[bot]", "testowner", "testrepo"); + + expect(result).toEqual({ + isBot: true, + isActive: false, + error: "Unexpected error", + }); + }); + + it("should verify bot is installed on repository", async () => { + mockGithub.rest.repos.getCollaboratorPermissionLevel.mockResolvedValue({ + data: { permission: "admin" }, + }); + + const result = await checkBotStatus("dependabot[bot]", "testowner", "testrepo"); + + expect(mockGithub.rest.repos.getCollaboratorPermissionLevel).toHaveBeenCalledWith({ + owner: "testowner", + repo: "testrepo", + username: "dependabot[bot]", + }); + + expect(result.isBot).toBe(true); + expect(result.isActive).toBe(true); + }); + }); }); diff --git a/actions/setup/js/package.json b/actions/setup/js/package.json index ec5d511c95..9e98fb3b60 100644 --- a/actions/setup/js/package.json +++ b/actions/setup/js/package.json @@ -22,10 +22,10 @@ "test:js-watch": "vitest", "test:js-coverage": "vitest run --coverage", "format:terser": "find . -name '*.cjs' -type f -not -path './node_modules/*' | while read file; do if ! grep -qE '^(await |///