From ac5ea5adfd2924afbeec046fddeabf4a9c8535a2 Mon Sep 17 00:00:00 2001 From: Joshua Miller Date: Mon, 12 Aug 2024 17:34:12 -0500 Subject: [PATCH] :bulb: make org config issues clearer (#85) * This adds a new response state- bad_config- so that those impacted by this application can more easily identify what problem their org sync is running into. --- src/services/gitHub.ts | 35 ++++++++++++++++++++++++----------- src/services/gitHubCache.ts | 4 ++-- src/services/gitHubTypes.ts | 15 ++++++++++++++- src/services/githubSync.ts | 32 +++++++++++++++++++++++++------- 4 files changed, 65 insertions(+), 21 deletions(-) diff --git a/src/services/gitHub.ts b/src/services/gitHub.ts index ee1923b..13131fb 100644 --- a/src/services/gitHub.ts +++ b/src/services/gitHub.ts @@ -1,7 +1,7 @@ import { Octokit } from "octokit"; import { createAppAuth } from "@octokit/auth-app"; import { Config } from "../config"; -import { AddMemberResponse, CopilotAddResponse, GitHubClient, GitHubId, GitHubTeamId, InstalledClient, Org, OrgInvite, OrgRoles, RemoveMemberResponse, Response } from "./gitHubTypes"; +import { AddMemberResponse, CopilotAddResponse, GitHubClient, GitHubId, GitHubTeamId, InstalledClient, Org, OrgConfigResponse, OrgInvite, OrgRoles, RemoveMemberResponse, Response } from "./gitHubTypes"; import { AppConfig } from "./appConfig"; import yaml from "js-yaml"; import { throttling } from "@octokit/plugin-throttling"; @@ -615,7 +615,7 @@ class InstalledGitHubClient implements InstalledClient { } - public async GetConfigurationForInstallation(): Response { + public async GetConfigurationForInstallation(): OrgConfigResponse { // TODO: this function doesn't really belong on this class... // i.e., it doesn't fit with a "GitHub Facade" const getContentRequest = { @@ -631,7 +631,8 @@ class InstalledGitHubClient implements InstalledClient { } catch { return { - successful: false + successful: false, + state: "NoConfig" } } @@ -639,7 +640,8 @@ class InstalledGitHubClient implements InstalledClient { if (!Array.isArray(potentialFiles)) { return { - successful: false + successful: false, + state: "NoConfig" } } @@ -649,7 +651,9 @@ class InstalledGitHubClient implements InstalledClient { if (onlyConfigFiles.length != 1) { return { - successful: false + successful: false, + state: "BadConfig", + message: "Multiple configuration files are not supported at this point in time." } } @@ -664,15 +668,24 @@ class InstalledGitHubClient implements InstalledClient { if (Array.isArray(contentData) || contentData.type != "file") { return { - successful: false + successful: false, + state: "BadConfig" } } - const configuration = yaml.load(Buffer.from(contentData.content, 'base64').toString()) as OrgConfigurationOptions; - - return { - successful: true, - data: new OrgConfig(configuration) + try { + const configuration = yaml.load(Buffer.from(contentData.content, 'base64').toString()) as OrgConfigurationOptions; + return { + successful: true, + data: new OrgConfig(configuration) + } + } + catch { + return { + successful: false, + state: "BadConfig", + message: "Error parsing configuration- check configuration file for validity: https://github.com/cloudpups/github-teams-user-sync/blob/main/docs/OrganizationConfiguration.md" + } } } } \ No newline at end of file diff --git a/src/services/gitHubCache.ts b/src/services/gitHubCache.ts index 92ecc80..7d4d556 100644 --- a/src/services/gitHubCache.ts +++ b/src/services/gitHubCache.ts @@ -1,6 +1,6 @@ import { CacheClient } from "../app"; import { ILogger } from "../logging"; -import { AddMemberResponse, CopilotAddResponse, GitHubId, GitHubTeamId, InstalledClient, OrgInvite, OrgRoles, RemoveMemberResponse, Response } from "./gitHubTypes"; +import { AddMemberResponse, CopilotAddResponse, GitHubId, GitHubTeamId, InstalledClient, OrgConfigResponse, OrgInvite, OrgRoles, RemoveMemberResponse, Response } from "./gitHubTypes"; import { OrgConfig } from "./orgConfig"; export class GitHubClientCache implements InstalledClient { @@ -146,7 +146,7 @@ export class GitHubClientCache implements InstalledClient { return this.client.AddSecurityManagerTeam(team); } - GetConfigurationForInstallation(): Response { + GetConfigurationForInstallation(): OrgConfigResponse { return this.client.GetConfigurationForInstallation(); } } \ No newline at end of file diff --git a/src/services/gitHubTypes.ts b/src/services/gitHubTypes.ts index 6492709..f602b20 100644 --- a/src/services/gitHubTypes.ts +++ b/src/services/gitHubTypes.ts @@ -19,6 +19,19 @@ export type OrgInvite = { GitHubUser: string } +export type OrgConfigResponseSuccess = { + successful: true, + data: OrgConfig +} + +export type OrgConfigResponseBad = { + successful: false, + state: "NoConfig" | "BadConfig", + message?: string +} + +export type OrgConfigResponse = Promise; + export interface InstalledClient { GetCurrentOrgName(): string GetCurrentRateLimit(): Promise<{ remaining: number }> @@ -32,7 +45,7 @@ export interface InstalledClient { RemoveTeamMemberAsync(team: GitHubTeamName, user: GitHubId): RemoveMemberResponse UpdateTeamDetails(team: GitHubTeamName, description: string): Response AddSecurityManagerTeam(team: GitHubTeamName): Promise - GetConfigurationForInstallation(): Response + GetConfigurationForInstallation(): OrgConfigResponse SetOrgRole(id: GitHubId, role: OrgRoles): Response GetPendingOrgInvites():Response CancelOrgInvite(invite:OrgInvite): Response diff --git a/src/services/githubSync.ts b/src/services/githubSync.ts index 4c086ff..dd8e896 100644 --- a/src/services/githubSync.ts +++ b/src/services/githubSync.ts @@ -1,5 +1,6 @@ // REMEMBER TO REPLACE '_' with '-' for GitHub Names! 🤦‍♂️ +import e from "express"; import { Log, LogError } from "../logging"; import { AppConfig } from "./appConfig"; import { CopilotAddResponse, FailedResponse, GitHubId, InstalledClient, OrgInvite } from "./gitHubTypes"; @@ -167,7 +168,7 @@ async function SynchronizeGitHubTeam(installedGitHubClient: InstalledClient, tea type ReturnTypeOfSyncOrg = { message?: string; - status: "failed" | "completed" | "no_config"; + status: "failed" | "completed" | "no_config" | "bad_config"; orgName: string; syncedSecurityManagerTeams: string[]; orgOwnersGroup: string; @@ -261,13 +262,14 @@ async function syncOrg(installedGitHubClient: InstalledClient, appConfig: AppCon } const setOfExistingTeams = new Set(existingTeamsResponse.data.map(t => t.Name.toUpperCase())); - const orgConfigResponse = await installedGitHubClient.GetConfigurationForInstallation(); + const orgConfigResponse = await installedGitHubClient.GetConfigurationForInstallation(); - const orgConfig = orgConfigResponse.successful ? orgConfigResponse.data : undefined; + const securityManagersFromOrgConfig = orgConfigResponse.successful ? orgConfigResponse.data.AdditionalSecurityManagerGroups : []; + const securityManagersDisplayNameSourceMap = orgConfigResponse.successful ? orgConfigResponse.data.DisplayNameToSourceMap : new Map(); const securityManagerTeams = [ ...appConfig.SecurityManagerTeams, - ...orgConfig?.AdditionalSecurityManagerGroups ?? [] + ...securityManagersFromOrgConfig ]; if (securityManagerTeams.length > 0) { @@ -278,7 +280,7 @@ async function syncOrg(installedGitHubClient: InstalledClient, appConfig: AppCon securityManagerTeams, setOfExistingTeams, shortLink: appConfig.Description.ShortLink, - displayNameToSourceMap: orgConfig?.DisplayNameToSourceMap ?? new Map() + displayNameToSourceMap: securityManagersDisplayNameSourceMap }); if (!syncManagersResponse.Success) { @@ -293,15 +295,31 @@ async function syncOrg(installedGitHubClient: InstalledClient, appConfig: AppCon ...response, syncedSecurityManagerTeams: syncManagersResponse.SyncedSecurityManagerTeams } - } + } - if (orgConfig == undefined) { + if (!orgConfigResponse.successful && orgConfigResponse.state == "NoConfig") { return { ...response, message: "Cannot access/fetch organization config", status: "no_config" } } + else if(!orgConfigResponse.successful && orgConfigResponse.state == "BadConfig") { + return { + ...response, + message: orgConfigResponse.message, + status: "bad_config" + } + } + else if (!orgConfigResponse.successful) { + return { + ...response, + message: orgConfigResponse.message, + status: "bad_config" + } + } + + const orgConfig = orgConfigResponse.data; Log(JSON.stringify(orgConfig));