Refactor GitHub Integration (#6713)

* fix: refactor

* fix: tests

* fix: apply octokit plugin pattern

package.json

@@ -67,6 +67,7 @@
     "@hocuspocus/server": "1.1.2",
     "@joplin/turndown-plugin-gfm": "^1.0.49",
     "@juggle/resize-observer": "^3.4.0",
+    "@octokit/auth-app": "^6.0.4",
     "@outlinewiki/koa-passport": "^4.2.1",
     "@outlinewiki/passport-azure-ad-oauth2": "^0.1.0",
     "@renderlesskit/react": "^0.11.0",
@@ -149,8 +150,8 @@
     "natural-sort": "^1.0.0",
     "node-fetch": "2.7.0",
     "nodemailer": "^6.9.9",
-    "outline-icons": "^3.2.1",
     "octokit": "^3.1.2",
+    "outline-icons": "^3.2.1",
     "oy-vey": "^0.12.1",
     "passport": "^0.7.0",
     "passport-google-oauth2": "^0.2.0",

plugins/github/server/api/github.ts

@@ -1,4 +1,5 @@
 import Router from "koa-router";
+import find from "lodash/find";
 import { IntegrationService, IntegrationType } from "@shared/types";
 import Logger from "@server/logging/Logger";
 import auth from "@server/middlewares/authentication";
@@ -7,7 +8,7 @@ import validate from "@server/middlewares/validate";
 import { IntegrationAuthentication, Integration, Team } from "@server/models";
 import { APIContext } from "@server/types";
 import { GitHubUtils } from "../../shared/GitHubUtils";
-import { GitHubUser } from "../github";
+import { GitHub } from "../github";
 import * as T from "./schema";
 
 const router = new Router();
@@ -65,13 +66,14 @@ router.get(
       }
     }
 
-    const githubUser = new GitHubUser({ code: code!, state: teamId });
+    const client = await GitHub.authenticateAsUser(code!, teamId);
+    const installationsByUser = await client.requestAppInstallations();
+    const installation = find(
+      installationsByUser,
+      (i) => i.id === installationId
+    );
 
-    let installation;
+    if (!installation) {
-    try {
-      installation = await githubUser.getInstallation(installationId!);
-    } catch (err) {
-      Logger.error("Failed to fetch GitHub App installation", err);
       return ctx.redirect(GitHubUtils.errorUrl("unauthenticated"));
     }
 

plugins/github/server/github.ts

@@ -1,65 +1,292 @@
-import { createOAuthUserAuth } from "@octokit/auth-oauth-user";
-import find from "lodash/find";
-import { App, Octokit } from "octokit";
-import pluralize from "pluralize";
 import {
-  IntegrationService,
+  createOAuthUserAuth,
-  IntegrationType,
+  createAppAuth,
-  Unfurl,
+  type OAuthWebFlowAuthOptions,
-  UnfurlResponse,
+  type InstallationAuthOptions,
-} from "@shared/types";
+} from "@octokit/auth-app";
+import { Octokit } from "octokit";
+import pluralize from "pluralize";
+import { IntegrationService, IntegrationType, Unfurl } from "@shared/types";
 import Logger from "@server/logging/Logger";
 import { Integration, User } from "@server/models";
 import { GitHubUtils } from "../shared/GitHubUtils";
 import env from "./env";
 
-/**
+enum Resource {
- * It exposes a GitHub REST client for accessing APIs which
+  PR = "pull",
- * particulary require the client to authenticate as a GitHub App
+  Issue = "issue",
- */
+}
-class GitHubApp {
+
-  /** Required to authenticate as GitHub App */
+type PreviewData = {
-  private static id = env.GITHUB_APP_ID;
+  [Resource.PR]: {
-  private static key = env.GITHUB_APP_PRIVATE_KEY
+    url: string;
+    type: Resource.PR;
+    title: string;
+    description: string;
+    author: { name: string; avatarUrl: string };
+    createdAt: string;
+    meta: {
+      identifier: string;
+      status: { name: string; color: string };
+    };
+  };
+  [Resource.Issue]: {
+    url: string;
+    type: Resource.Issue;
+    title: string;
+    description: string;
+    author: { name: string; avatarUrl: string };
+    createdAt: string;
+    meta: {
+      identifier: string;
+      labels: Array<{ name: string; color: string }>;
+      status: { name: string; color: string };
+    };
+  };
+};
+
+const requestPlugin = (octokit: Octokit) => ({
+  requestPR: async (params: ReturnType<typeof GitHub.parseUrl>) =>
+    octokit.request(`GET /repos/{owner}/{repo}/pulls/{id}`, {
+      owner: params?.owner,
+      repo: params?.repo,
+      id: params?.id,
+      headers: {
+        Accept: "application/vnd.github.text+json",
+        "X-GitHub-Api-Version": "2022-11-28",
+      },
+    }),
+
+  requestIssue: async (params: ReturnType<typeof GitHub.parseUrl>) =>
+    octokit.request(`GET /repos/{owner}/{repo}/issues/{id}`, {
+      owner: params?.owner,
+      repo: params?.repo,
+      id: params?.id,
+      headers: {
+        Accept: "application/vnd.github.text+json",
+        "X-GitHub-Api-Version": "2022-11-28",
+      },
+    }),
+
+  /**
+   * Fetches app installations accessible to the user
+   *
+   * @returns {Array} Containing details of all app installations done by user
+   */
+  requestAppInstallations: async () =>
+    octokit.paginate("GET /user/installations"),
+
+  /**
+   * Fetches details of a GitHub resource, e.g, a pull request or an issue
+   *
+   * @param resource Contains identifiers which are used to construct resource endpoint, e.g, `/repos/{params.owner}/{params.repo}/pulls/{params.id}`
+   * @returns Response containing resource details
+   */
+  requestResource: async function requestResource(
+    resource: ReturnType<typeof GitHub.parseUrl>
+  ) {
+    switch (resource?.type) {
+      case Resource.PR:
+        return this.requestPR(resource);
+      case Resource.Issue:
+        return this.requestIssue(resource);
+      default:
+        return { data: undefined };
+    }
+  },
+
+  /**
+   * Uninstalls the GitHub app from a given target
+   *
+   * @param installationId Id of the target from where to uninstall
+   */
+  requestAppUninstall: async (installationId: number) =>
+    octokit.request("DELETE /app/installations/{id}", {
+      id: installationId,
+    }),
+});
+
+const CustomOctokit = Octokit.plugin(requestPlugin);
+
+export class GitHub {
+  private static appId = env.GITHUB_APP_ID;
+  private static appKey = env.GITHUB_APP_PRIVATE_KEY
     ? Buffer.from(env.GITHUB_APP_PRIVATE_KEY, "base64").toString("ascii")
     : undefined;
 
-  /** GitHub App instance */
+  private static clientId = env.GITHUB_CLIENT_ID;
-  private app: App;
+  private static clientSecret = env.GITHUB_CLIENT_SECRET;
 
-  constructor() {
+  private static appOctokit: Octokit;
-    if (GitHubApp.id && GitHubApp.key) {
+
-      this.app = new App({
+  private static supportedResources = Object.values(Resource);
-        appId: GitHubApp.id!,
+
-        privateKey: GitHubApp.key!,
+  private static transformPRData = (
-      });
+    resource: ReturnType<typeof GitHub.parseUrl>,
-    }
+    data: Record<string, any>
-  }
+  ): PreviewData[Resource.PR] => ({
+    url: resource!.url,
+    type: Resource.PR,
+    title: data.title,
+    description: data.body,
+    author: {
+      name: data.user.login,
+      avatarUrl: data.user.avatar_url,
+    },
+    createdAt: data.created_at,
+    meta: {
+      identifier: `#${data.number}`,
+      status: {
+        name: data.merged ? "merged" : data.state,
+        color: GitHubUtils.getColorForStatus(
+          data.merged ? "merged" : data.state
+        ),
+      },
+    },
+  });
+
+  private static transformIssueData = (
+    resource: ReturnType<typeof GitHub.parseUrl>,
+    data: Record<string, any>
+  ): PreviewData[Resource.Issue] => ({
+    url: resource!.url,
+    type: Resource.Issue,
+    title: data.title,
+    description: data.body_text,
+    author: {
+      name: data.user.login,
+      avatarUrl: data.user.avatar_url,
+    },
+    createdAt: data.created_at,
+    meta: {
+      identifier: `#${data.number}`,
+      labels: data.labels.map((label: { name: string; color: string }) => ({
+        name: label.name,
+        color: `#${label.color}`,
+      })),
+      status: {
+        name: data.state,
+        color: GitHubUtils.getColorForStatus(data.state),
+      },
+    },
+  });
 
   /**
-   * Given an `installationId`, removes that GitHub App installation
+   * Parses a given URL and returns resource identifiers for GitHub specific URLs
-   * @param installationId
+   *
+   * @param url URL to parse
+   * @returns {object} Containing resource identifiers - `owner`, `repo`, `type` and `id`.
    */
-  public async deleteInstallation(installationId: number) {
+  public static parseUrl(url: string) {
-    await this.app.octokit.request(
+    const { hostname, pathname } = new URL(url);
-      "DELETE /app/installations/{installation_id}",
+    if (hostname !== "github.com") {
-      {
+      return;
-        installation_id: installationId,
+    }
-      }
+
-    );
+    const parts = pathname.split("/");
+    const owner = parts[1];
+    const repo = parts[2];
+    const type = pluralize.singular(parts[3]) as Resource;
+    const id = parts[4];
+
+    if (!GitHub.supportedResources.includes(type)) {
+      Logger.warn(`Unsupported GitHub resource type: ${type}`);
+      return;
+    }
+
+    return { owner, repo, type, id, url };
   }
 
+  private static authenticateAsApp = () => {
+    if (!GitHub.appOctokit) {
+      GitHub.appOctokit = new CustomOctokit({
+        authStrategy: createAppAuth,
+        auth: {
+          appId: GitHub.appId,
+          privateKey: GitHub.appKey,
+          clientId: GitHub.clientId,
+          clientSecret: GitHub.clientSecret,
+        },
+      });
+    }
+
+    return GitHub.appOctokit;
+  };
+
+  /**
+   * [Authenticates as a GitHub user](https://github.com/octokit/auth-app.js/?tab=readme-ov-file#authenticate-as-installation)
+   *
+   * @param code Temporary code received in callback url after user authorizes
+   * @param state A string received in callback url to protect against CSRF
+   * @returns {Octokit} User-authenticated octokit instance
+   */
+  public static authenticateAsUser = async (
+    code: string,
+    state?: string | null
+  ) =>
+    GitHub.authenticateAsApp().auth({
+      type: "oauth-user",
+      code,
+      state,
+      factory: (options: OAuthWebFlowAuthOptions) =>
+        new CustomOctokit({
+          authStrategy: createOAuthUserAuth,
+          auth: options,
+        }),
+    }) as Promise<InstanceType<typeof CustomOctokit>>;
+
+  /**
+   * [Authenticates as a GitHub app installation](https://github.com/octokit/auth-app.js/?tab=readme-ov-file#authenticate-as-installation)
+   *
+   * @param installationId Id of an installation
+   * @returns {Octokit} Installation-authenticated octokit instance
+   */
+  public static authenticateAsInstallation = async (installationId: number) =>
+    GitHub.authenticateAsApp().auth({
+      type: "installation",
+      installationId,
+      factory: (options: InstallationAuthOptions) =>
+        new CustomOctokit({
+          authStrategy: createAppAuth,
+          auth: options,
+        }),
+    }) as Promise<InstanceType<typeof CustomOctokit>>;
+
+  /**
+   * Transforms resource data obtained from GitHub to our own pre-defined preview data
+   * which will be consumed by our API clients
+   *
+   * @param resourceType Resource type for which to transform the data, e.g, an issue
+   * @param data Resource data obtained from GitHub via REST calls
+   * @returns {PreviewData} Transformed data suitable for our API clients
+   */
+  public static transformResourceData = (
+    resource: ReturnType<typeof GitHub.parseUrl>,
+    data: Record<string, any>
+  ) => {
+    switch (resource?.type) {
+      case Resource.PR:
+        return GitHub.transformPRData(resource, data);
+      case Resource.Issue:
+        return GitHub.transformIssueData(resource, data);
+      default:
+        return;
+    }
+  };
+
   /**
    *
-   * @param url GitHub resource url - could be a url of a pull request or an issue
+   * @param url GitHub resource url
-   * @param installationId Id corresponding to the GitHub App installation
+   * @param actor User attempting to unfurl resource url
-   * @returns {object} An object container the resource details - could be a pull request
+   * @returns {object} An object containing resource details e.g, a GitHub Pull Request details
-   * details or an issue details
    */
-  unfurl = async (url: string, actor: User): Promise<Unfurl | undefined> => {
+  public static unfurl = async (
-    const { owner, repo, resourceType, resourceId } = GitHubUtils.parseUrl(url);
+    url: string,
+    actor: User
+  ): Promise<Unfurl | undefined> => {
+    const resource = GitHub.parseUrl(url);
 
-    if (!owner) {
+    if (!resource) {
       return;
     }
 
@@ -67,7 +294,7 @@ class GitHubApp {
       where: {
         service: IntegrationService.GitHub,
         teamId: actor.teamId,
-        "settings.github.installation.account.name": owner,
+        "settings.github.installation.account.name": resource.owner,
       },
     })) as Integration<IntegrationType.Embed>;
 
@@ -76,93 +303,17 @@ class GitHubApp {
     }
 
     try {
-      const octokit = await this.app.getInstallationOctokit(
+      const client = await GitHub.authenticateAsInstallation(
         integration.settings.github!.installation.id
       );
-      const { data } = await octokit.request(
+      const { data } = await client.requestResource(resource);
-        `GET /repos/{owner}/{repo}/${pluralize(resourceType)}/{ref}`,
+      if (!data) {
-        {
+        return;
-          owner,
+      }
-          repo,
+      return GitHub.transformResourceData(resource, data);
-          ref: resourceId,
-          headers: {
-            Accept: "application/vnd.github.text+json",
-            "X-GitHub-Api-Version": "2022-11-28",
-          },
-        }
-      );
-
-      const status = data.merged ? "merged" : data.state;
-
-      return {
-        url,
-        type: pluralize.singular(resourceType) as UnfurlResponse["type"],
-        title: data.title,
-        description: data.body_text,
-        author: {
-          name: data.user.login,
-          avatarUrl: data.user.avatar_url,
-        },
-        createdAt: data.created_at,
-        meta: {
-          identifier: `#${data.number}`,
-          labels: data.labels.map((label: { name: string; color: string }) => ({
-            name: label.name,
-            color: `#${label.color}`,
-          })),
-          status: {
-            name: status,
-            color: GitHubUtils.getColorForStatus(status),
-          },
-        },
-      };
     } catch (err) {
       Logger.warn("Failed to fetch resource from GitHub", err);
       return;
     }
   };
 }
-
-export const githubApp = new GitHubApp();
-
-/**
- * It exposes a GitHub REST client for accessing APIs which
- * particularly require the client to authenticate as a user
- * through the user access token
- */
-export class GitHubUser {
-  private static clientId = env.GITHUB_CLIENT_ID;
-  private static clientSecret = env.GITHUB_CLIENT_SECRET;
-  private static clientType = "github-app";
-
-  /** GitHub client for accessing its APIs */
-  private client: Octokit;
-
-  constructor(options: { code: string; state?: string | null }) {
-    this.client = new Octokit({
-      authStrategy: createOAuthUserAuth,
-      auth: {
-        clientId: GitHubUser.clientId,
-        clientSecret: GitHubUser.clientSecret,
-        clientType: GitHubUser.clientType,
-        code: options.code,
-        state: options.state,
-      },
-    });
-  }
-
-  /**
-   * @param installationId Identifies a GitHub App installation
-   * @returns {object} An object containing details about the GitHub App installation,
-   * e.g, installation target, account which installed the app etc.
-   */
-  public async getInstallation(installationId: number) {
-    const installations = await this.client.paginate("GET /user/installations");
-    const installation = find(installations, (i) => i.id === installationId);
-    if (!installation) {
-      Logger.warn("installationId mismatch!");
-      throw Error("Invalid installationId!");
-    }
-    return installation;
-  }
-}

plugins/github/server/index.ts

@@ -3,7 +3,7 @@ import { PluginManager, Hook } from "@server/utils/PluginManager";
 import config from "../plugin.json";
 import router from "./api/github";
 import env from "./env";
-import { githubApp } from "./github";
+import { GitHub } from "./github";
 import { uninstall } from "./uninstall";
 
 const enabled =
@@ -22,7 +22,7 @@ if (enabled) {
     },
     {
       type: Hook.UnfurlProvider,
-      value: { unfurl: githubApp.unfurl, cacheExpiry: Minute },
+      value: { unfurl: GitHub.unfurl, cacheExpiry: Minute },
     },
     {
       type: Hook.Uninstall,

plugins/github/server/uninstall.ts

@@ -1,6 +1,6 @@
 import { IntegrationService, IntegrationType } from "@shared/types";
 import { Integration } from "@server/models";
-import { githubApp } from "./github";
+import { GitHub } from "./github";
 
 export async function uninstall(
   integration: Integration<IntegrationType.Embed>
@@ -9,7 +9,8 @@ export async function uninstall(
     const installationId = integration.settings?.github?.installation.id;
 
     if (installationId) {
-      return githubApp.deleteInstallation(installationId);
+      const client = await GitHub.authenticateAsInstallation(installationId);
+      await client.requestAppUninstall(installationId);
     }
   }
 }

plugins/github/shared/GitHubUtils.ts

@@ -5,8 +5,6 @@ import { integrationSettingsPath } from "@shared/utils/routeHelpers";
 export class GitHubUtils {
   public static clientId = env.GITHUB_CLIENT_ID;
 
-  public static allowedResources = ["pull", "issues"];
-
   static get url() {
     return integrationSettingsPath("github");
   }
@@ -47,27 +45,6 @@ export class GitHubUtils {
     return `${this.url}?install_request=true`;
   }
 
-  /**
-   * Parses a GitHub like URL to obtain info like repo name, owner, resource type(issue or PR).
-   *
-   * @param url URL to parse
-   * @returns An object containing repository, owner, resource type(issue or pull request) and resource id
-   */
-  public static parseUrl(url: string) {
-    const { hostname, pathname } = new URL(url);
-    if (hostname !== "github.com") {
-      return {};
-    }
-
-    const [, owner, repo, resourceType, resourceId] = pathname.split("/");
-
-    if (!this.allowedResources.includes(resourceType)) {
-      return {};
-    }
-
-    return { owner, repo, resourceType, resourceId };
-  }
-
   public static getColorForStatus(status: string) {
     switch (status) {
       case "open":

yarn.lock

@@ -2084,10 +2084,10 @@
     "@octokit/types" "^12.0.0"
     "@octokit/webhooks" "^12.0.4"
 
-"@octokit/auth-app@^6.0.0":
+"@octokit/auth-app@^6.0.0", "@octokit/auth-app@^6.0.4":
-  version "6.0.3"
+  version "6.0.4"
-  resolved "https://registry.yarnpkg.com/@octokit/auth-app/-/auth-app-6.0.3.tgz#4c0ba68e8d3b1a55c34d1e68ea0ca92ef018bb7a"
+  resolved "https://registry.yarnpkg.com/@octokit/auth-app/-/auth-app-6.0.4.tgz#3c435c4c9ba9005405d889f4a5018df5e8ca93cf"
-  integrity sha512-9N7IlBAKEJR3tJgPSubCxIDYGXSdc+2xbkjYpk9nCyqREnH8qEMoMhiEB1WgoA9yTFp91El92XNXAi+AjuKnfw==
+  integrity sha512-TPmJYgd05ok3nzHj7Y6we/V7Ez1wU3ztLFW3zo/afgYFtqYZg0W7zb6Kp5ag6E85r8nCE1JfS6YZoZusa14o9g==
   dependencies:
     "@octokit/auth-oauth-app" "^7.0.0"
     "@octokit/auth-oauth-user" "^4.0.0"