From c6a04c2ebddc9df27da4e01c9e8b5d84c135e712 Mon Sep 17 00:00:00 2001
From: Will Hunt <will@half-shot.uk>
Date: Fri, 21 Apr 2023 10:27:57 +0100
Subject: [PATCH] Fix case where GitHub repos aren't connectable if the GitHub
 App was manually approved (#718)

* Fix GitHub connections failing if the installation cache is stale

* changelog
---
 changelog.d/718.bugfix        |  1 +
 src/Connections/GithubRepo.ts | 28 ++++++++++++++++++++++------
 src/Github/GrantChecker.ts    |  5 ++---
 3 files changed, 25 insertions(+), 9 deletions(-)
 create mode 100644 changelog.d/718.bugfix

diff --git a/changelog.d/718.bugfix b/changelog.d/718.bugfix
new file mode 100644
index 00000000..de97bd20
--- /dev/null
+++ b/changelog.d/718.bugfix
@@ -0,0 +1 @@
+Fix cases of GitHub repos not being bridgable if the GitHub App had to be manually approved.
diff --git a/src/Connections/GithubRepo.ts b/src/Connections/GithubRepo.ts
index 698c8512..f762cb1c 100644
--- a/src/Connections/GithubRepo.ts
+++ b/src/Connections/GithubRepo.ts
@@ -383,7 +383,7 @@ export class GitHubRepoConnection extends CommandConnection<GitHubRepoConnection
         throw new ValidatorApiError(validator.errors);
     }
 
-    static async assertUserHasAccessToRepo(userId: string, org: string, repo: string, github: GithubInstance, tokenStore: UserTokenStore) {
+    static async assertUserHasAccessToRepo(userId: string, org: string, repo: string, tokenStore: UserTokenStore) {
         const octokit = await tokenStore.getOctokitForUser(userId);
         if (!octokit) {
             throw new ApiError("User is not authenticated with GitHub", ErrCode.ForbiddenUser);
@@ -407,9 +407,25 @@ export class GitHubRepoConnection extends CommandConnection<GitHubRepoConnection
             throw Error('GitHub is not configured');
         }
         const validData = this.validateState(data);
-        await this.assertUserHasAccessToRepo(userId, validData.org, validData.repo, github, tokenStore);
-        const appOctokit = await github.getSafeOctokitForRepo(validData.org, validData.repo);
-        if (!appOctokit) {
+        await this.assertUserHasAccessToRepo(userId, validData.org, validData.repo, tokenStore);
+        const userOctokit = await tokenStore.getOctokitForUser(userId);
+        if (!userOctokit) {
+            // Given we assert the above, this is unlikely.
+            throw new ApiError("User is not authenticated with GitHub", ErrCode.ForbiddenUser);
+        }
+        const ownSelf = await userOctokit.users.getAuthenticated();
+        
+        let installationId = 0;
+
+        if (ownSelf.data.login === validData.org) {
+            installationId = (await github.appOctokit.apps.getUserInstallation({ username: ownSelf.data.login })).data.id;
+        } else {
+            // Github will error if the authed user tries to list repos of a disallowed installation, even
+            // if we got the installation ID from the app's instance.
+            installationId = (await github.appOctokit.apps.getOrgInstallation({ org: validData.org })).data.id;
+        }
+
+        if (!installationId) {
             throw new ApiError(
                 "You need to add a GitHub App to this organisation / repository before you can bridge it. Open the link to add the app, and then retry this request",
                 ErrCode.AdditionalActionRequired,
@@ -421,7 +437,7 @@ export class GitHubRepoConnection extends CommandConnection<GitHubRepoConnection
             );
         }
         const stateEventKey = `${validData.org}/${validData.repo}`;
-        await new GitHubGrantChecker(as, github, tokenStore).grantConnection(roomId, { org: validData.org, repo: validData.repo });
+        await new GitHubGrantChecker(as, tokenStore).grantConnection(roomId, { org: validData.org, repo: validData.repo });
         await intent.underlyingClient.sendStateEvent(roomId, this.CanonicalEventType, stateEventKey, validData);
         return {
             stateEventContent: validData,
@@ -533,7 +549,7 @@ export class GitHubRepoConnection extends CommandConnection<GitHubRepoConnection
 
     public debounceOnIssueLabeled = new Map<number, {labels: Set<string>, timeout: NodeJS.Timeout}>();
 
-    private readonly grantChecker = new GitHubGrantChecker(this.as, this.githubInstance, this.tokenStore);
+    private readonly grantChecker = new GitHubGrantChecker(this.as, this.tokenStore);
 
     constructor(
         roomId: string,
diff --git a/src/Github/GrantChecker.ts b/src/Github/GrantChecker.ts
index e3aa9ace..a1497f30 100644
--- a/src/Github/GrantChecker.ts
+++ b/src/Github/GrantChecker.ts
@@ -2,7 +2,6 @@ import { Appservice } from "matrix-bot-sdk";
 import { GitHubRepoConnection } from "../Connections";
 import { GrantChecker } from "../grants/GrantCheck";
 import { UserTokenStore } from "../UserTokenStore";
-import { GithubInstance } from "./GithubInstance";
 import { Logger } from 'matrix-appservice-bridge';
 
 const log = new Logger('GitHubGrantChecker');
@@ -14,7 +13,7 @@ interface GitHubGrantConnectionId {
 
 
 export class GitHubGrantChecker extends GrantChecker<GitHubGrantConnectionId> {
-    constructor(private readonly as: Appservice, private readonly github: GithubInstance, private readonly tokenStore: UserTokenStore) {
+    constructor(private readonly as: Appservice, private readonly tokenStore: UserTokenStore) {
         super(as.botIntent, "github")
     }
 
@@ -29,7 +28,7 @@ export class GitHubGrantChecker extends GrantChecker<GitHubGrantConnectionId> {
             return true;
         }
         try {
-            await GitHubRepoConnection.assertUserHasAccessToRepo(sender, connectionId.org, connectionId.repo, this.github, this.tokenStore);
+            await GitHubRepoConnection.assertUserHasAccessToRepo(sender, connectionId.org, connectionId.repo, this.tokenStore);
             return true;
         } catch (ex) {
             log.info(`Tried to check fallback for ${roomId}: ${sender} does not have access to ${connectionId.org}/${connectionId.repo}`, ex);