From 85b72a64d6108e9f30a77f4f4148ec6966ea999d Mon Sep 17 00:00:00 2001 From: lovestaco Date: Fri, 23 Oct 2026 19:52:46 +0530 Subject: [PATCH 01/10] Share one GitHub App installation across LiveReview orgs Two people from the same GitHub organization can now each sign up with GitHub: the second person's org gets its own connector for the installation instead of "already connected to other LiveReview accounts". - One connector per (installation, org). An org gets every repository the installation covers only if the person who connected it can access all of them; otherwise just the repositories they can access (GitHub reports both lists, so a hand-edited callback URL gains nothing). The list is refreshed on every GitHub sign-in. - Webhooks fan out: installation and repository events reach every org; PR updates reach every org that has the repository, and only the oldest connector writes the PR description links; comment replies come from the org that reviewed the PR most recently. - "Start a review" and "Review history" links open the user's own org's copy of the PR. - Git Providers / get-started: separate Disconnect (this org only) and Uninstall (removes the App from GitHub for every org, with a warning saying how many other accounts use it). Notes when the App was installed by someone else. - Fix: a failed /gh-login no longer leaves a get-started redirect behind; the callback sets the landing page only on success. --- ...090000_github_app_shared_installations.sql | 14 + db/schema.sql | 7 +- internal/api/auto_webhook_installer.go | 32 ++ internal/api/github_app_auth.go | 409 ++++++++++++++---- internal/api/github_app_auth_test.go | 275 +++++++++++- internal/api/github_app_state.go | 6 +- internal/api/pull_request_review_trigger.go | 11 +- .../docs/routes_guide/auth/gh-login.md | 11 + .../docs/routes_guide/get-started-github.md | 17 +- .../docs/routes_guide/git/git-providers.md | 15 +- .../routes_guide/reviews/pr-review-link.md | 7 +- .../docs/routes_guide/reviews/reviews-list.md | 3 +- internal/githubapp/token_test.go | 96 +++- internal/jobqueue/repo_sync_worker.go | 21 +- network/network_status.md | 21 +- network/providers/github/app.go | 26 ++ network/providers/github/app_test.go | 25 ++ storage/providers/github/token_store.go | 147 ++++++- storage/providers/github/token_store_test.go | 41 ++ storage/providers/pullrequests/upsert.go | 32 ++ storage/providers/pullrequests/upsert_test.go | 87 ++++ storage/storage_status.md | 34 +- ui/src/api/githubApp.ts | 13 +- ui/src/api/pullRequests.ts | 8 +- .../GitHubApp/GitHubAppAccounts.tsx | 94 +++- ui/src/hooks/useSwitchToPullRequestOrg.ts | 16 +- ui/src/index.tsx | 17 +- ui/src/pages/Auth/GhLogin.tsx | 11 +- .../Reviews/PullRequestReviewTrigger.tsx | 4 +- ui/src/pages/Reviews/Reviews.tsx | 6 +- 30 files changed, 1284 insertions(+), 222 deletions(-) create mode 100644 db/migrations/20261023090000_github_app_shared_installations.sql diff --git a/db/migrations/20261023090000_github_app_shared_installations.sql b/db/migrations/20261023090000_github_app_shared_installations.sql new file mode 100644 index 00000000..e056f8a8 --- /dev/null +++ b/db/migrations/20261023090000_github_app_shared_installations.sql @@ -0,0 +1,14 @@ +-- migrate:up +-- One GitHub App installation can be connected to several LiveReview orgs (one connector per org), e.g. when two +-- people from the same GitHub organization each sign up. Each org still has at most one connector per installation. +DROP INDEX IF EXISTS uq_integration_tokens_github_app_installation; +CREATE UNIQUE INDEX IF NOT EXISTS uq_integration_tokens_github_app_installation_org + ON integration_tokens (provider_url, provider_app_id, org_id) + WHERE token_type = 'GITHUB_APP'; + +-- migrate:down +-- Fails while an installation is connected to more than one org; remove the extra connectors first. +DROP INDEX IF EXISTS uq_integration_tokens_github_app_installation_org; +CREATE UNIQUE INDEX IF NOT EXISTS uq_integration_tokens_github_app_installation + ON integration_tokens (provider_url, provider_app_id) + WHERE token_type = 'GITHUB_APP'; diff --git a/db/schema.sql b/db/schema.sql index 017ab289..41a16163 100644 --- a/db/schema.sql +++ b/db/schema.sql @@ -6083,10 +6083,10 @@ CREATE UNIQUE INDEX river_job_unique_idx ON public.river_job USING btree (unique -- --- Name: uq_integration_tokens_github_app_installation; Type: INDEX; Schema: public; Owner: - +-- Name: uq_integration_tokens_github_app_installation_org; Type: INDEX; Schema: public; Owner: - -- -CREATE UNIQUE INDEX uq_integration_tokens_github_app_installation ON public.integration_tokens USING btree (provider_url, provider_app_id) WHERE (token_type = 'GITHUB_APP'::text); +CREATE UNIQUE INDEX uq_integration_tokens_github_app_installation_org ON public.integration_tokens USING btree (provider_url, provider_app_id, org_id) WHERE (token_type = 'GITHUB_APP'::text); -- @@ -7309,4 +7309,5 @@ INSERT INTO public.schema_migrations (version) VALUES ('20261017100000'), ('20261020090000'), ('20261022120000'), - ('20261022130000'); + ('20261022130000'), + ('20261023090000'); diff --git a/internal/api/auto_webhook_installer.go b/internal/api/auto_webhook_installer.go index faa7bc4b..e4158ec8 100644 --- a/internal/api/auto_webhook_installer.go +++ b/internal/api/auto_webhook_installer.go @@ -6,6 +6,7 @@ import ( "encoding/json" "fmt" "log" + "strconv" "strings" "time" @@ -116,6 +117,8 @@ func (awi *AutoWebhookInstaller) processAutoInstallation(ctx context.Context, co // waiting for the first periodic reconciliation sweep. func (awi *AutoWebhookInstaller) syncRepositoriesAndQueueBackfill(ctx context.Context, connectorID int, connector *ConnectorDetails) error { store := pullrequests.NewStore(awi.db) + // A user-scoped GitHub App connector only gets the repositories its user can access. + allowed, scoped := userScopedRepoIDs(connector) cursor := "" total := 0 @@ -138,6 +141,9 @@ func (awi *AutoWebhookInstaller) syncRepositoriesAndQueueBackfill(ctx context.Co } for _, repo := range page.Repositories { + if scoped && !allowed[repo.ProviderRepoID] { + continue + } repositoryID, err := store.UpsertRepository(pullrequests.RepositoryUpsert{ OrgID: connector.OrgID, ConnectorID: int64(connectorID), @@ -169,10 +175,36 @@ func (awi *AutoWebhookInstaller) syncRepositoriesAndQueueBackfill(ctx context.Co cursor = page.NextCursor } + if scoped { + keep := make([]string, 0, len(allowed)) + for id := range allowed { + keep = append(keep, id) + } + if err := store.DeleteRepositoriesByConnectorExcept(int64(connectorID), keep); err != nil { + return fmt.Errorf("failed to drop repositories the user can no longer access: %w", err) + } + } + log.Printf("Queued initial PR/MR backfill for %d repositories (connector %d)", total, connectorID) return nil } +// userScopedRepoIDs returns the provider repository ids a user-scoped GitHub App connector may sync; scoped is +// false for every other connector (installer App connectors and PAT/OAuth ones sync everything they list). +func userScopedRepoIDs(connector *ConnectorDetails) (allowed map[string]bool, scoped bool) { + if connector.TokenType != storagegithub.TokenTypeGitHubApp || connector.Metadata["repo_scope"] != storagegithub.RepoScopeUser { + return nil, false + } + allowed = map[string]bool{} + ids, _ := connector.Metadata["allowed_repo_ids"].([]interface{}) + for _, raw := range ids { + if id, ok := raw.(float64); ok { + allowed[strconv.FormatInt(int64(id), 10)] = true + } + } + return allowed, true +} + // listAppInstallationRepositories lists one page of repositories for a GitHub App connector, using a valid // (possibly just-minted) installation token rather than whatever pat_token held when the connector was loaded. func (awi *AutoWebhookInstaller) listAppInstallationRepositories(ctx context.Context, connector *ConnectorDetails, cursor string) (*providers.RepositoryPage, error) { diff --git a/internal/api/github_app_auth.go b/internal/api/github_app_auth.go index 010968ac..a2f03c6c 100644 --- a/internal/api/github_app_auth.go +++ b/internal/api/github_app_auth.go @@ -9,6 +9,7 @@ import ( "encoding/hex" "encoding/json" "errors" + "fmt" "html/template" "io" "log" @@ -25,6 +26,7 @@ import ( "github.com/livereview/internal/api/auth" "github.com/livereview/internal/githubapp" "github.com/livereview/internal/license" + githubprovider "github.com/livereview/internal/provider_input/github" "github.com/livereview/network/hexmos" networkgithub "github.com/livereview/network/providers/github" storageaiconnectors "github.com/livereview/storage/aiconnectors" @@ -54,7 +56,8 @@ func (s *Server) registerGitHubAppRoutes(public, v1 *echo.Group, authMiddleware authMiddleware.ValidateOrgAccess(), authMiddleware.BuildPermissionContext(), ) - // Session-only (RequireAuth, not API keys): it cuts a connection, so org owners only. + // Session-only (RequireAuth, not API keys): it cuts a connection, so org owners only. ?uninstall=true also + // uninstalls the App from GitHub, for every org using it. v1.DELETE("/github-app/installations/:connector_id", s.GitHubAppDisconnect, authMiddleware.RequireAuth(), authMiddleware.BuildOrgContextFromHeader(), @@ -145,10 +148,14 @@ func (s *Server) GitHubAppInstallCheck(c echo.Context) error { return c.JSON(http.StatusOK, resp) } -// GitHubAppDisconnect uninstalls the GitHub App from the GitHub account or organization (as the App, via its own -// JWT) and stops LiveReview using it for the caller's org: the connector is marked removed (its cached token is -// cleared) and its repositories are dropped, exactly as the GitHub "installation deleted" webhook does (which -// GitHub also sends, harmlessly, after the uninstall). To use the App again, install it again from GitHub. +// GitHubAppDisconnect stops the caller's org using a GitHub App installation. Several orgs can share one +// installation, so there are two ways: +// - default: only this org's connector is marked removed (its cached token cleared, its repositories dropped); +// the App stays installed on GitHub and other orgs keep working. Plain GitHub sign-ins don't reconnect it; +// installing or configuring the App again does. +// - ?uninstall=true: also uninstalls the App from the GitHub account or organization (as the App, via its own +// JWT), which ends it for every org using it, so every org's connector is removed here too (GitHub's +// "installation deleted" webhook, which follows, then finds nothing left to do). func (s *Server) GitHubAppDisconnect(c echo.Context) error { orgID, ok := auth.GetOrgIDFromContext(c) if !ok { @@ -161,13 +168,13 @@ func (s *Server) GitHubAppDisconnect(c echo.Context) error { if err != nil || connectorID <= 0 { return c.JSON(http.StatusBadRequest, map[string]string{"error": "invalid connector id"}) } + uninstall := c.QueryParam("uninstall") == "true" store := storagegithub.NewTokenStore(s.db) rec, err := store.GetGitHubTokenByConnectorID(connectorID) // Same answer for "does not exist", "not a GitHub App connector" and "another org's": never confirm other orgs' ids. if err != nil || rec == nil || rec.OrgID != orgID || rec.TokenType != storagegithub.TokenTypeGitHubApp { return c.JSON(http.StatusNotFound, map[string]string{"error": "GitHub installation not found"}) } - // Uninstall from GitHub first: if that fails nothing has changed and the owner can simply retry. var meta struct { InstallationID int64 `json:"installation_id"` } @@ -175,6 +182,21 @@ func (s *Server) GitHubAppDisconnect(c echo.Context) error { log.Printf("[GitHubApp] disconnect connector %d: no installation id in metadata", connectorID) return c.JSON(http.StatusInternalServerError, map[string]string{"error": "failed to disconnect"}) } + repos := pullrequests.NewStore(s.db) + + if !uninstall { + if err := store.DisconnectAppConnector(connectorID); err != nil { + log.Printf("[GitHubApp] disconnect connector %d (org %d) failed: %v", connectorID, orgID, err) + return c.JSON(http.StatusInternalServerError, map[string]string{"error": "failed to disconnect"}) + } + if err := repos.DeleteRepositoriesByConnector(connectorID); err != nil { + log.Printf("[GitHubApp] disconnect connector %d: dropping repositories failed: %v", connectorID, err) + return c.JSON(http.StatusInternalServerError, map[string]string{"error": "failed to disconnect"}) + } + return c.JSON(http.StatusOK, map[string]string{"status": "disconnected"}) + } + + // Uninstall from GitHub first: if that fails nothing has changed and the owner can simply retry. cfg := s.githubApp ctx, cancel := context.WithTimeout(c.Request().Context(), 20*time.Second) defer cancel() @@ -186,16 +208,23 @@ func (s *Server) GitHubAppDisconnect(c echo.Context) error { log.Printf("[GitHubApp] disconnect connector %d (org %d): uninstalling installation %d failed: %v", connectorID, orgID, meta.InstallationID, err) return c.JSON(http.StatusBadGateway, map[string]string{"error": "Could not uninstall the app from GitHub. Please try again."}) } - yes := true - if err := store.SetAppConnectorFlags(connectorID, &yes, nil); err != nil { - log.Printf("[GitHubApp] disconnect connector %d (org %d) failed: %v", connectorID, orgID, err) + recs, err := store.ListAppConnectorsByInstallation(cfg.BaseURL, meta.InstallationID) + if err != nil { + log.Printf("[GitHubApp] uninstall installation %d: listing its connectors failed: %v", meta.InstallationID, err) return c.JSON(http.StatusInternalServerError, map[string]string{"error": "failed to disconnect"}) } - if err := pullrequests.NewStore(s.db).DeleteRepositoriesByConnector(connectorID); err != nil { - log.Printf("[GitHubApp] disconnect connector %d: dropping repositories failed: %v", connectorID, err) - return c.JSON(http.StatusInternalServerError, map[string]string{"error": "failed to disconnect"}) + yes := true + for _, r := range recs { + if err := store.SetAppConnectorFlags(r.ID, &yes, nil); err != nil { + log.Printf("[GitHubApp] uninstall installation %d: removing connector %d failed: %v", meta.InstallationID, r.ID, err) + return c.JSON(http.StatusInternalServerError, map[string]string{"error": "failed to disconnect"}) + } + if err := repos.DeleteRepositoriesByConnector(r.ID); err != nil { + log.Printf("[GitHubApp] uninstall installation %d: dropping connector %d repositories failed: %v", meta.InstallationID, r.ID, err) + return c.JSON(http.StatusInternalServerError, map[string]string{"error": "failed to disconnect"}) + } } - return c.JSON(http.StatusOK, map[string]string{"status": "disconnected"}) + return c.JSON(http.StatusOK, map[string]string{"status": "uninstalled"}) } // --------------------------------------------------------------------------------------------------------- @@ -210,15 +239,17 @@ func (s *Server) GitHubAppStart(c echo.Context) error { if id, err := strconv.ParseInt(c.QueryParam("target_id"), 10, 64); err == nil && id > 0 { targetID = id } - return s.redirectToGitHub(c, sanitizeGHSrc(c.QueryParam("src")), sanitizeGHMode(c.QueryParam("mode")), targetID) + mode := sanitizeGHMode(c.QueryParam("mode")) + // Only /gh-login starts in sign-in mode; the in-app install buttons store their own return page. + return s.redirectToGitHub(c, sanitizeGHSrc(c.QueryParam("src")), mode, targetID, mode == ghModeSignin) } -func (s *Server) redirectToGitHub(c echo.Context, src, mode string, targetID int64) error { +func (s *Server) redirectToGitHub(c echo.Context, src, mode string, targetID int64, getStarted bool) error { nonce, err := newGHNonce() if err != nil { return s.githubAppErrorPage(c, http.StatusInternalServerError, "Could not start sign-in. Please try again.") } - signed, err := signGHState(ghState{Nonce: nonce, Src: src, Mode: mode, Exp: time.Now().Add(ghStateTTL).Unix()}, s.githubAppSecret) + signed, err := signGHState(ghState{Nonce: nonce, Src: src, Mode: mode, Exp: time.Now().Add(ghStateTTL).Unix(), GetStarted: getStarted}, s.githubAppSecret) if err != nil { return s.githubAppErrorPage(c, http.StatusInternalServerError, "Could not start sign-in. Please try again.") } @@ -360,19 +391,17 @@ func (s *Server) GitHubAppCallback(c echo.Context) error { } if len(targets) == 0 { - return s.redirectToGitHub(c, state.Src, ghModeInstall, 0) + return s.redirectToGitHub(c, state.Src, ghModeInstall, 0, state.GetStarted) } - // 5. connect each installation to the org. + // 5. connect each installation to the org. Other orgs may use the same installation (each has its own + // connector), and each gets only the repositories its GitHub user can access (see connectGitHubAppInstallation). connected, failed := 0, 0 alreadyConnected := false // a connector for one of these installations existed before this sign-in for _, inst := range targets { - connectorID, created, err := s.connectGitHubAppInstallation(ctx, provisioned.OrgID, inst) - if errors.Is(err, storagegithub.ErrInstallationBoundToOtherOrg) { - if explicit { - return s.githubAppErrorPage(c, http.StatusConflict, "This GitHub installation is already connected to another LiveReview account.") - } - log.Printf("[GitHubApp] installation %d is bound to another org; skipping", inst.ID) + connectorID, created, resync, err := s.connectGitHubAppInstallation(ctx, provisioned.OrgID, inst, userToken, explicit) + if errors.Is(err, storagegithub.ErrAppConnectorDisconnected) { + // The org disconnected it on purpose; installing or configuring the App again reconnects it. continue } if err != nil { @@ -389,14 +418,11 @@ func (s *Server) GitHubAppCallback(c echo.Context) error { if !created { alreadyConnected = true } - s.syncGitHubAppConnectorAsync(connectorID, created) + s.syncGitHubAppConnectorAsync(connectorID, created || resync) } if connected == 0 && failed > 0 { return s.githubAppErrorPage(c, http.StatusBadGateway, "Could not connect your GitHub installation. Please try again.") } - if connected == 0 { - return s.githubAppErrorPage(c, http.StatusConflict, "Your GitHub installations are already connected to other LiveReview accounts.") - } // 6. hand over to the normal Cloud login, using the same ?data= contract the Hexmos onelogin uses. data, err := json.Marshal(map[string]interface{}{ @@ -424,10 +450,18 @@ func (s *Server) GitHubAppCallback(c echo.Context) error { }) // A returning user (plain GitHub sign-in, nothing new installed, their org and connection already existed) has // finished onboarding: send them to the GitHub App card on Git Providers, where their accounts are managed, instead of get-started. - if state.Mode == ghModeSignin && !explicit && alreadyConnected && !provisioned.CreatedOrg { + // Everyone else who started on /gh-login lands on get-started. + next := "" + switch { + case state.Mode == ghModeSignin && !explicit && (alreadyConnected || connected == 0) && !provisioned.CreatedOrg: + next = ghNextGitProviders + case state.GetStarted: + next = ghNextGetStarted + } + if next != "" { // nosemgrep: go.lang.security.audit.net.cookie-missing-secure.cookie-missing-secure, go.lang.security.audit.net.cookie-missing-httponly.cookie-missing-httponly -- the UI must read it from JS (index.tsx); holds only a page hint; Secure whenever HTTPS c.SetCookie(&http.Cookie{ - Name: ghNextCookieName, Value: ghNextGitProviders, Path: "/", + Name: ghNextCookieName, Value: next, Path: "/", MaxAge: 300, Secure: c.Scheme() == "https", SameSite: http.SameSiteLaxMode, }) } @@ -451,20 +485,120 @@ func (s *Server) grantLaunchTrialManagedAI(ctx context.Context, orgID int64) { } // connectGitHubAppInstallation mints an installation token and records the installation as a connector of the org. -func (s *Server) connectGitHubAppInstallation(ctx context.Context, orgID int64, inst networkgithub.Installation) (connectorID int64, created bool, err error) { +// The org gets every repository the installation covers only when this GitHub user can access all of them (whoever +// installed or manages it); otherwise only the ones the user can access, since anyone who merely sees the +// installation (an outside collaborator, say) must not get the rest. GitHub supplies both lists, so nothing here +// trusts the callback URL. explicit (the user just installed or configured it) also reconnects an installation the +// org had disconnected. resync is true when the connector's repositories need syncing again. +func (s *Server) connectGitHubAppInstallation(ctx context.Context, orgID int64, inst networkgithub.Installation, userToken string, explicit bool) (connectorID int64, created, resync bool, err error) { cfg := s.githubApp + store := storagegithub.NewTokenStore(s.db) + var before appConnectorScope // the existing connector's scope, if any + if prev, err := store.GetAppConnectorByInstallation(cfg.BaseURL, inst.ID, orgID); err == nil { + _ = json.Unmarshal(prev.Metadata, &before) + } else if !errors.Is(err, sql.ErrNoRows) { + return 0, false, false, err + } + appJWT, err := networkgithub.MintAppJWT(cfg.AppID, cfg.PrivateKey, time.Now()) if err != nil { - return 0, false, err + return 0, false, false, err } tok, err := networkgithub.CreateInstallationToken(ctx, cfg.APIBaseURL(), appJWT, inst.ID) if err != nil { - return 0, false, err + return 0, false, false, err } - return storagegithub.NewTokenStore(s.db).UpsertAppInstallationConnector(storagegithub.AppConnectorUpsert{ + userRepos, err := networkgithub.ListUserInstallationRepositoryIDs(ctx, cfg.APIBaseURL(), userToken, inst.ID) + if err != nil { + return 0, false, false, fmt.Errorf("listing the user's repositories: %w", err) + } + all, err := installationRepositoryIDs(ctx, cfg.APIBaseURL(), tok.Token) + if err != nil { + return 0, false, false, fmt.Errorf("listing the installation's repositories: %w", err) + } + + in := storagegithub.AppConnectorUpsert{ OrgID: orgID, BaseURL: cfg.BaseURL, InstallationID: inst.ID, AccountLogin: inst.AccountLogin, AccountType: inst.AccountType, RepositorySelection: inst.RepositorySelection, AppSlug: cfg.Slug, Token: tok.Token, ExpiresAt: tok.ExpiresAt, - }) + RepoScope: storagegithub.RepoScopeInstaller, KeepDisconnected: !explicit, + } + if !containsAllIDs(userRepos, all) { + in.RepoScope, in.AllowedRepoIDs = storagegithub.RepoScopeUser, userRepos + } + connectorID, created, err = store.UpsertAppInstallationConnector(in) + if err != nil { + return connectorID, false, false, err + } + switch { + case explicit || before.Removed: + // Just installed or configured (the repositories it grants may have changed), or reconnected. + return connectorID, created, true, nil + case before.RepoScope == storagegithub.RepoScopeUser: + // Widened to every repository, or the user's accessible ones changed. + return connectorID, created, in.RepoScope != storagegithub.RepoScopeUser || !sameIDSet(before.AllowedRepoIDs, in.AllowedRepoIDs), nil + default: + // New (created already syncs) or a live installer connector, which keeps every repository. + return connectorID, created, false, nil + } +} + +// installationRepositoryIDs lists the ids of every repository an installation covers. +func installationRepositoryIDs(ctx context.Context, apiBase, installationToken string) ([]int64, error) { + var ids []int64 + cursor := "" + for page := 0; ; page++ { + if page == 100 { + return nil, errors.New("installation lists more than 10000 repositories") + } + repos, next, err := networkgithub.ListInstallationRepositories(ctx, apiBase, installationToken, cursor) + if err != nil { + return nil, err + } + for _, r := range repos { + ids = append(ids, r.ID) + } + if next == "" { + return ids, nil + } + cursor = next + } +} + +// containsAllIDs reports whether have includes every id in want. +func containsAllIDs(have, want []int64) bool { + set := make(map[int64]bool, len(have)) + for _, id := range have { + set[id] = true + } + for _, id := range want { + if !set[id] { + return false + } + } + return true +} + +// appConnectorScope reads a GitHub App connector's repository scope from its metadata. +type appConnectorScope struct { + RepoScope string `json:"repo_scope"` + AllowedRepoIDs []int64 `json:"allowed_repo_ids"` + Removed bool `json:"removed"` +} + +func sameIDSet(a, b []int64) bool { + if len(a) != len(b) { + return false + } + seen := make(map[int64]bool, len(a)) + for _, id := range a { + seen[id] = true + } + for _, id := range b { + if !seen[id] { + return false + } + } + return true } // githubAppSyncTimeout bounds one background repository sync (listing repos, registering them, PR backfill). @@ -518,6 +652,17 @@ type githubAppWebhookEnvelope struct { RepositoriesRemoved []struct { ID int64 `json:"id"` } `json:"repositories_removed"` + Repository *struct { + ID int64 `json:"id"` + } `json:"repository"` + // The PR a comment belongs to: issue_comment carries it as the issue (whose html_url is the PR's for PR + // comments), review events as pull_request. + Issue *struct { + HTMLURL string `json:"html_url"` + } `json:"issue"` + PullRequest *struct { + HTMLURL string `json:"html_url"` + } `json:"pull_request"` } func (s *Server) gitHubAppWebhookHandler(authMiddleware *auth.AuthMiddleware) echo.HandlerFunc { @@ -542,34 +687,65 @@ func (s *Server) gitHubAppWebhookHandler(authMiddleware *auth.AuthMiddleware) ec return ignored("no_installation") } + // Several orgs can share one installation, each with its own connector. store := storagegithub.NewTokenStore(s.db) - rec, err := store.GetAppConnectorByInstallation(s.githubApp.BaseURL, env.Installation.ID) - if errors.Is(err, sql.ErrNoRows) { - return ignored("unknown_installation") // 200 so GitHub does not retry - } + recs, err := store.ListAppConnectorsByInstallation(s.githubApp.BaseURL, env.Installation.ID) if err != nil { log.Printf("[GitHubApp] webhook connector lookup failed: %v", err) return c.JSON(http.StatusInternalServerError, map[string]string{"error": "internal_error"}) } + if len(recs) == 0 { + return ignored("unknown_installation") // 200 so GitHub does not retry + } switch event { case "installation": - return s.handleGitHubAppInstallationEvent(c, store, rec, env) + return s.handleGitHubAppInstallationEvent(c, store, recs, env) case "installation_repositories": - return s.handleGitHubAppRepositoriesEvent(c, rec, env) + return s.handleGitHubAppRepositoriesEvent(c, recs, env) } - var meta struct { - Removed bool `json:"removed"` - Suspended bool `json:"suspended"` + // The orgs this event is for: live connectors that cover the repository (a user-scoped one only covers + // the repositories its user can access), oldest first. + var targets []storagegithub.IntegrationTokenRecord + for _, rec := range recs { + if appConnectorCovers(rec.Metadata, env.Repository) { + targets = append(targets, rec) + } } - _ = json.Unmarshal(rec.Metadata, &meta) - if meta.Removed || meta.Suspended { + if len(targets) == 0 { return ignored("installation_inactive") } + // One org handles the event through the normal pipeline: the oldest connector, or for comments the org + // that reviewed this PR most recently (so replies come from that org and its AI provider). + target := targets[0] + if prURL := env.prURL(); prURL != "" && event != "pull_request" && len(targets) > 1 { + ids := make([]int64, len(targets)) + for i, rec := range targets { + ids[i] = rec.ID + } + id, err := store.LatestReviewConnector(ids, prURL) + if err != nil && !errors.Is(err, sql.ErrNoRows) { + log.Printf("[GitHubApp] webhook: finding the reviewing org failed: %v", err) + return c.JSON(http.StatusInternalServerError, map[string]string{"error": "internal_error"}) + } + for _, rec := range targets { + if rec.ID == id { + target = rec + } + } + } + // The other orgs only need their copy of the PR list updated. + if event == "pull_request" && len(targets) > 1 { + if err := s.queueSharedPullRequestState(c, body, targets[1:]); err != nil { + log.Printf("[GitHubApp] webhook: updating other orgs' PR failed: %v", err) + return c.JSON(http.StatusInternalServerError, map[string]string{"error": "internal_error"}) + } + } + // Everything else (pull_request, comments, ...) is processed exactly like a PAT connector's webhook. - if err := authMiddleware.SetConnectorOrgContext(c, rec.ID); err != nil { + if err := authMiddleware.SetConnectorOrgContext(c, target.ID); err != nil { return err } c.Request().Body = io.NopCloser(bytes.NewReader(body)) // the orchestrator reads the body again @@ -577,37 +753,101 @@ func (s *Server) gitHubAppWebhookHandler(authMiddleware *auth.AuthMiddleware) ec } } -func (s *Server) handleGitHubAppInstallationEvent(c echo.Context, store *storagegithub.TokenStore, rec *storagegithub.IntegrationTokenRecord, env githubAppWebhookEnvelope) error { - yes, no := true, false - var err error - switch env.Action { - case "deleted": - if err = store.SetAppConnectorFlags(rec.ID, &yes, nil); err == nil { - // Nothing can sync a removed installation; drop its repositories rather than failing every sweep. - err = pullrequests.NewStore(s.db).DeleteRepositoriesByConnector(rec.ID) - } - case "suspend": - err = store.SetAppConnectorFlags(rec.ID, nil, &yes) - case "unsuspend": - err = store.SetAppConnectorFlags(rec.ID, nil, &no) +// prURL is the pull request a comment or review event belongs to ("" for anything else). +func (env githubAppWebhookEnvelope) prURL() string { + if env.PullRequest != nil && env.PullRequest.HTMLURL != "" { + return env.PullRequest.HTMLURL } - if err != nil { - log.Printf("[GitHubApp] installation %q event failed for connector %d: %v", env.Action, rec.ID, err) - return c.JSON(http.StatusInternalServerError, map[string]string{"error": "internal_error"}) + if env.Issue != nil && strings.Contains(env.Issue.HTMLURL, "/pull/") { + return env.Issue.HTMLURL + } + return "" +} + +// appConnectorCovers reports whether a GitHub App connector is live and may act on the repository: installer +// connectors cover every repository, user-scoped ones only their allowed list. Events without a repository are +// covered by every live connector. +func appConnectorCovers(metadata []byte, repo *struct { + ID int64 `json:"id"` +}) bool { + var meta struct { + appConnectorScope + Suspended bool `json:"suspended"` + } + if json.Unmarshal(metadata, &meta) != nil || meta.Removed || meta.Suspended { + return false + } + if repo == nil || meta.RepoScope != storagegithub.RepoScopeUser { + return true + } + for _, id := range meta.AllowedRepoIDs { + if id == repo.ID { + return true + } + } + return false +} + +// queueSharedPullRequestState updates the PR in each of these orgs (connectors sharing the installation with the one +// that handles the event). +func (s *Server) queueSharedPullRequestState(c echo.Context, body []byte, recs []storagegithub.IntegrationTokenRecord) error { + if s.jobQueue == nil { + return errors.New("job queue not initialized") + } + ev, matched, err := (&githubprovider.GitHubV2Provider{}).ConvertPullRequestStateEvent( + map[string]string{"X-GitHub-Event": "pull_request"}, body) + if err != nil || !matched { + return err // not a state change (e.g. review_requested): nothing to update + } + for _, rec := range recs { + if err := s.jobQueue.QueueSharedPRStateSyncJob(c.Request().Context(), rec.OrgID, rec.ID, "github", *ev); err != nil { + return err + } + } + return nil +} + +// handleGitHubAppInstallationEvent applies an installation's deletion or suspension to every org's connector for it. +func (s *Server) handleGitHubAppInstallationEvent(c echo.Context, store *storagegithub.TokenStore, recs []storagegithub.IntegrationTokenRecord, env githubAppWebhookEnvelope) error { + yes, no := true, false + for _, rec := range recs { + var err error + switch env.Action { + case "deleted": + if err = store.SetAppConnectorFlags(rec.ID, &yes, nil); err == nil { + // Nothing can sync a removed installation; drop its repositories rather than failing every sweep. + err = pullrequests.NewStore(s.db).DeleteRepositoriesByConnector(rec.ID) + } + case "suspend": + err = store.SetAppConnectorFlags(rec.ID, nil, &yes) + case "unsuspend": + err = store.SetAppConnectorFlags(rec.ID, nil, &no) + } + if err != nil { + log.Printf("[GitHubApp] installation %q event failed for connector %d: %v", env.Action, rec.ID, err) + return c.JSON(http.StatusInternalServerError, map[string]string{"error": "internal_error"}) + } } return c.JSON(http.StatusOK, map[string]string{"status": "ok", "event": "installation." + env.Action}) } -func (s *Server) handleGitHubAppRepositoriesEvent(c echo.Context, rec *storagegithub.IntegrationTokenRecord, env githubAppWebhookEnvelope) error { - switch env.Action { - case "added": - s.syncGitHubAppConnectorAsync(rec.ID, true) - case "removed": - store := pullrequests.NewStore(s.db) - for _, r := range env.RepositoriesRemoved { - if err := store.DeleteRepositoryByProviderID(rec.ID, strconv.FormatInt(r.ID, 10)); err != nil { - log.Printf("[GitHubApp] removing repository %d from connector %d failed: %v", r.ID, rec.ID, err) - return c.JSON(http.StatusInternalServerError, map[string]string{"error": "internal_error"}) +// handleGitHubAppRepositoriesEvent applies repositories added to or removed from the installation to every org +// using it (a user-scoped org only picks up added ones its user may access). +func (s *Server) handleGitHubAppRepositoriesEvent(c echo.Context, recs []storagegithub.IntegrationTokenRecord, env githubAppWebhookEnvelope) error { + store := pullrequests.NewStore(s.db) + for _, rec := range recs { + if !appConnectorCovers(rec.Metadata, nil) { + continue + } + switch env.Action { + case "added": + s.syncGitHubAppConnectorAsync(rec.ID, true) + case "removed": + for _, r := range env.RepositoriesRemoved { + if err := store.DeleteRepositoryByProviderID(rec.ID, strconv.FormatInt(r.ID, 10)); err != nil { + log.Printf("[GitHubApp] removing repository %d from connector %d failed: %v", r.ID, rec.ID, err) + return c.JSON(http.StatusInternalServerError, map[string]string{"error": "internal_error"}) + } } } } @@ -623,6 +863,11 @@ type githubAppDemoInstallation struct { AccountType string `json:"account_type"` RepositorySelection string `json:"repository_selection"` SettingsURL string `json:"settings_url"` + // RepoScope is "user" when this org only gets the repositories its GitHub user can access (the App was + // installed by someone else), otherwise "installer". + RepoScope string `json:"repo_scope"` + // OtherOrgs counts the other LiveReview orgs using this installation (a number only, nothing about them). + OtherOrgs int `json:"other_orgs"` } type githubAppDemoRepo struct { @@ -683,6 +928,7 @@ func (s *Server) GitHubAppDemo(c echo.Context) error { for i := range records { rec := &records[i] var meta struct { + appConnectorScope InstallationID int64 `json:"installation_id"` AccountLogin string `json:"account_login"` AccountType string `json:"account_type"` @@ -694,6 +940,15 @@ func (s *Server) GitHubAppDemo(c echo.Context) error { // Not usable until unsuspended on GitHub; the full answer skips it too (no token can be minted). continue } + scope := storagegithub.RepoScopeInstaller + if meta.RepoScope == storagegithub.RepoScopeUser { + scope = storagegithub.RepoScopeUser + } + others, err := store.CountOtherOrgsUsingInstallation(cfg.BaseURL, meta.InstallationID, orgID) + if err != nil { + log.Printf("[GitHubApp] demo: counting other orgs for connector %d failed: %v", rec.ID, err) + return c.JSON(http.StatusInternalServerError, map[string]string{"error": "failed to load GitHub connections"}) + } var token string if !light { @@ -707,6 +962,7 @@ func (s *Server) GitHubAppDemo(c echo.Context) error { resp.Installations = append(resp.Installations, githubAppDemoInstallation{ ConnectorID: rec.ID, AccountLogin: meta.AccountLogin, AccountType: meta.AccountType, RepositorySelection: meta.RepositorySelection, SettingsURL: cfg.InstallationSettingsURL(meta.AccountType, meta.AccountLogin, meta.InstallationID), + RepoScope: scope, OtherOrgs: others, }) if light { continue @@ -716,7 +972,14 @@ func (s *Server) GitHubAppDemo(c echo.Context) error { log.Printf("[GitHubApp] demo: listing repositories for connector %d failed: %v", rec.ID, err) continue } + allowed := map[int64]bool{} + for _, id := range meta.AllowedRepoIDs { + allowed[id] = true + } for _, r := range repos { + if scope == storagegithub.RepoScopeUser && !allowed[r.ID] { + continue + } resp.Repositories = append(resp.Repositories, githubAppDemoRepo{FullName: r.FullName, WebURL: r.HTMLURL, Private: r.Private}) if len(reposForPulls) < githubAppDemoMaxRepos { reposForPulls = append(reposForPulls, r.FullName) diff --git a/internal/api/github_app_auth_test.go b/internal/api/github_app_auth_test.go index 1f86c050..54233ac8 100644 --- a/internal/api/github_app_auth_test.go +++ b/internal/api/github_app_auth_test.go @@ -37,6 +37,8 @@ type fakeGitHub struct { mu sync.Mutex emails string // JSON for GET /user/emails installations string // JSON array body of "installations" + userRepos string // JSON array body of "repositories" for GET /user/installations/{id}/repositories + instRepos string // JSON array body of "repositories" for GET /installation/repositories deleted []string // installation ids the App was asked to uninstall failDelete bool srv *httptest.Server @@ -47,6 +49,8 @@ func newFakeGitHub(t *testing.T) *fakeGitHub { f := &fakeGitHub{ emails: `[{"email":"Gh.User@Example.test","primary":true,"verified":true}]`, installations: "[]", + userRepos: `[{"id":1}]`, + instRepos: `[{"id":1,"name":"api","full_name":"acme/api","html_url":"http://x/acme/api","private":true}]`, } f.srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { f.mu.Lock() @@ -63,6 +67,8 @@ func newFakeGitHub(t *testing.T) *fakeGitHub { _, _ = fmt.Fprint(w, f.emails) case r.URL.Path == "/api/v3/user/installations": _, _ = fmt.Fprintf(w, `{"installations":%s}`, f.installations) + case strings.HasPrefix(r.URL.Path, "/api/v3/user/installations/") && strings.HasSuffix(r.URL.Path, "/repositories"): + _, _ = fmt.Fprintf(w, `{"repositories":%s}`, f.userRepos) case r.Method == http.MethodDelete && strings.HasPrefix(r.URL.Path, "/api/v3/app/installations/"): f.deleted = append(f.deleted, strings.TrimPrefix(r.URL.Path, "/api/v3/app/installations/")) if f.failDelete { @@ -76,7 +82,7 @@ func newFakeGitHub(t *testing.T) *fakeGitHub { case r.URL.Path == "/api/v3/users/newco": _, _ = fmt.Fprint(w, `{"id":777,"login":"newco"}`) case r.URL.Path == "/api/v3/installation/repositories": - _, _ = fmt.Fprint(w, `{"repositories":[{"id":1,"name":"api","full_name":"acme/api","html_url":"http://x/acme/api","private":true}]}`) + _, _ = fmt.Fprintf(w, `{"repositories":%s}`, f.instRepos) case r.URL.Path == "/api/v3/repos/acme/api/pulls": _, _ = fmt.Fprint(w, `[{"number":7,"title":"Fix","html_url":"http://x/acme/api/pull/7","updated_at":"2026-10-19T10:00:00Z","user":{"login":"octo"}}]`) default: @@ -343,7 +349,7 @@ func TestGitHubAppCallback_HappyPathCreatesAccountAndConnector(t *testing.T) { if orgCookie != fmt.Sprint(orgID) { t.Fatalf("org cookie %q, want the connected org %d", orgCookie, orgID) } - rec2, err := storagegithub.NewTokenStore(db).GetAppConnectorByInstallation(f.srv.URL, inst) + rec2, err := storagegithub.NewTokenStore(db).GetAppConnectorByInstallation(f.srv.URL, inst, orgID) if err != nil { t.Fatalf("connector missing: %v", err) } @@ -426,7 +432,17 @@ func TestGitHubAppCallback_SignInWithNothingInstalledCreatesAccountThenGoesToIns } } -func TestGitHubAppCallback_InstallationBoundToAnotherOrgIsRejected(t *testing.T) { +// connectorMeta reads the scope fields of a GitHub App connector. +func connectorMeta(t *testing.T, rec *storagegithub.IntegrationTokenRecord) appConnectorScope { + t.Helper() + var sc appConnectorScope + if err := json.Unmarshal(rec.Metadata, &sc); err != nil { + t.Fatal(err) + } + return sc +} + +func TestGitHubAppCallback_SharedInstallationConnectsSecondOrgWithItsUsersRepos(t *testing.T) { f := newFakeGitHub(t) s, db := newGitHubAppTestServer(t, f) emailA := fmt.Sprintf("gh-owner-a-%d@example.test", time.Now().UnixNano()) @@ -436,24 +452,92 @@ func TestGitHubAppCallback_InstallationBoundToAnotherOrgIsRejected(t *testing.T) inst := time.Now().UnixNano() % 1_000_000_000_000 f.installations = f.installation(inst, "acme") - signIn := func(email string) *httptest.ResponseRecorder { + callback := func(email, mode, installation string) *httptest.ResponseRecorder { f.emails = fmt.Sprintf(`[{"email":%q,"primary":true,"verified":true}]`, email) newFakeParse(t, email, false) - state, nonce := s.validState(t, ghModeInstall) - return s.doCallback(t, callbackParams{state: state, cookieNonce: nonce, code: "good-code", installation: fmt.Sprint(inst)}) + state, nonce := s.validState(t, mode) + return s.doCallback(t, callbackParams{state: state, cookieNonce: nonce, code: "good-code", installation: installation}) + } + // The installation covers repositories 1 and 2. A installs the App and can access both. + f.instRepos = `[{"id":1,"name":"api","full_name":"acme/api"},{"id":2,"name":"web","full_name":"acme/web"}]` + f.userRepos = `[{"id":1},{"id":2}]` + if rec := callback(emailA, ghModeInstall, fmt.Sprint(inst)); rec.Code != http.StatusFound { + t.Fatalf("installer: status %d: %s", rec.Code, rec.Body.String()) } - if rec := signIn(emailA); rec.Code != http.StatusFound { - t.Fatalf("first owner: status %d: %s", rec.Code, rec.Body.String()) + // B signs up on their own, sees the installation, and can only access repository 1. Adding installation_id to + // the callback URL by hand must not get B the rest. + f.userRepos = `[{"id":1}]` + if rec := callback(emailB, ghModeSignin, ""); rec.Code != http.StatusFound { + t.Fatalf("second org: status %d: %s", rec.Code, rec.Body.String()) } - rec := signIn(emailB) - if rec.Code != http.StatusConflict || !strings.Contains(rec.Body.String(), "another LiveReview account") { - t.Fatalf("second owner: status %d body %q, want 409", rec.Code, rec.Body.String()) + if rec := callback(emailB, ghModeInstall, fmt.Sprint(inst)); rec.Code != http.StatusFound { + t.Fatalf("second org, crafted installation_id: status %d: %s", rec.Code, rec.Body.String()) } - var orgA int64 + + var orgA, orgB int64 _ = db.QueryRow(`SELECT id FROM orgs WHERE name = $1`, emailA).Scan(&orgA) - conn, err := storagegithub.NewTokenStore(db).GetAppConnectorByInstallation(f.srv.URL, inst) - if err != nil || conn.OrgID != orgA { - t.Fatalf("installation moved away from org A: %+v %v", conn, err) + _ = db.QueryRow(`SELECT id FROM orgs WHERE name = $1`, emailB).Scan(&orgB) + store := storagegithub.NewTokenStore(db) + connA, err := store.GetAppConnectorByInstallation(f.srv.URL, inst, orgA) + if err != nil || connectorMeta(t, connA).RepoScope != storagegithub.RepoScopeInstaller { + t.Fatalf("installer org connector: %+v %v", connA, err) + } + connB, err := store.GetAppConnectorByInstallation(f.srv.URL, inst, orgB) + if err != nil { + t.Fatalf("second org has no connector: %v", err) + } + if sc := connectorMeta(t, connB); sc.RepoScope != storagegithub.RepoScopeUser || len(sc.AllowedRepoIDs) != 1 || sc.AllowedRepoIDs[0] != 1 { + t.Fatalf("second org connector scope = %+v, want user [1]", sc) + } + + // The installer signing in again stays installer, even if GitHub now reports fewer repositories for them. + if rec := callback(emailA, ghModeSignin, ""); rec.Code != http.StatusFound { + t.Fatalf("installer sign-in: status %d", rec.Code) + } + if connA, _ = store.GetAppConnectorByInstallation(f.srv.URL, inst, orgA); connectorMeta(t, connA).RepoScope != storagegithub.RepoScopeInstaller { + t.Fatal("a plain sign-in narrowed the installer's connector") + } +} + +func TestGitHubAppCallback_GetStartedOnlyAfterSuccess(t *testing.T) { + f := newFakeGitHub(t) + s, db := newGitHubAppTestServer(t, f) + email := fmt.Sprintf("gh-landing-%d@example.test", time.Now().UnixNano()) + f.emails = fmt.Sprintf(`[{"email":%q,"primary":true,"verified":true}]`, email) + inst := time.Now().UnixNano() % 1_000_000_000_000 + f.installations = f.installation(inst, "acme") + newFakeParse(t, email, false) + cleanupGitHubAppTenant(t, db, email) + + nonce := "nonce-" + fmt.Sprint(time.Now().UnixNano()) + state, err := signGHState(ghState{Nonce: nonce, Src: "test", Mode: ghModeSignin, Exp: time.Now().Add(time.Minute).Unix(), GetStarted: true}, s.githubAppSecret) + if err != nil { + t.Fatal(err) + } + rec := s.doCallback(t, callbackParams{state: state, cookieNonce: nonce, code: "good-code"}) + if rec.Code != http.StatusFound { + t.Fatalf("status %d: %s", rec.Code, rec.Body.String()) + } + next := "" + for _, c := range rec.Result().Cookies() { + if c.Name == ghNextCookieName { + next = c.Value + } + } + if next != ghNextGetStarted { + t.Fatalf("a new /gh-login sign-in should land on get-started; next cookie = %q", next) + } + + // Starting from /gh-login carries the flag in the signed state, not in the browser. + req := httptest.NewRequest(http.MethodGet, "/api/v1/auth/github-app/start?mode=signin&src=test", nil) + startRec := httptest.NewRecorder() + if err := s.GitHubAppStart(echo.New().NewContext(req, startRec)); err != nil { + t.Fatal(err) + } + loc, _ := url.Parse(startRec.Header().Get("Location")) + st, err := verifyGHState(loc.Query().Get("state"), s.githubAppSecret, time.Now()) + if err != nil || !st.GetStarted { + t.Fatalf("sign-in start state = %+v, %v; want GetStarted", st, err) } } @@ -472,7 +556,7 @@ func TestGitHubAppCallback_ParseFailureStopsBeforeAnyConnector(t *testing.T) { if rec.Code != http.StatusBadGateway { t.Fatalf("status %d, want 502", rec.Code) } - if _, err := storagegithub.NewTokenStore(db).GetAppConnectorByInstallation(f.srv.URL, inst); err == nil { + if recs, _ := storagegithub.NewTokenStore(db).ListAppConnectorsByInstallation(f.srv.URL, inst); len(recs) != 0 { t.Fatal("a connector was created even though the Hexmos signup failed") } } @@ -530,13 +614,29 @@ func TestGitHubAppWebhook_SignatureUnknownInstallationAndUninstall(t *testing.T) if err != nil { t.Fatal(err) } + // A second org shares the installation; the uninstall reaches it too. + other := fmt.Sprintf("gh-hook-other-%d@example.test", time.Now().UnixNano()) + cleanupGitHubAppTenant(t, db, other) + var otherOrg int64 + if err := db.QueryRow(`INSERT INTO orgs (name, description) VALUES ($1, 'gh hook test') RETURNING id`, other).Scan(&otherOrg); err != nil { + t.Fatal(err) + } + otherConn, _, err := storagegithub.NewTokenStore(db).UpsertAppInstallationConnector(storagegithub.AppConnectorUpsert{ + OrgID: otherOrg, BaseURL: f.srv.URL, InstallationID: inst, AccountLogin: "acme", AccountType: "User", Token: "ghs_other", ExpiresAt: time.Now().Add(time.Hour), + RepoScope: storagegithub.RepoScopeUser, AllowedRepoIDs: []int64{1}, + }) + if err != nil { + t.Fatal(err) + } body = fmt.Sprintf(`{"action":"deleted","installation":{"id":%d}}`, inst) if rec := s.doWebhook(t, "installation", body, "valid"); rec.Code != http.StatusOK { t.Fatalf("uninstall: %d %s", rec.Code, rec.Body.String()) } - got, _ := storagegithub.NewTokenStore(db).GetGitHubTokenByConnectorID(connID) - if got.PatToken != "" || !strings.Contains(string(got.Metadata), `"removed": true`) { - t.Fatalf("connector not marked removed: pat=%q meta=%s", got.PatToken, got.Metadata) + for _, id := range []int64{connID, otherConn} { + got, _ := storagegithub.NewTokenStore(db).GetGitHubTokenByConnectorID(id) + if got.PatToken != "" || !strings.Contains(string(got.Metadata), `"removed": true`) { + t.Fatalf("connector %d not marked removed: pat=%q meta=%s", id, got.PatToken, got.Metadata) + } } // Events for a removed installation are ignored rather than processed. if rec := s.doWebhook(t, "issue_comment", fmt.Sprintf(`{"action":"created","installation":{"id":%d}}`, inst), "valid"); rec.Code != http.StatusOK || !strings.Contains(rec.Body.String(), "installation_inactive") { @@ -544,6 +644,78 @@ func TestGitHubAppWebhook_SignatureUnknownInstallationAndUninstall(t *testing.T) } } +func TestAppConnectorCovers_RespectsScopeAndState(t *testing.T) { + repo := func(id int64) *struct { + ID int64 `json:"id"` + } { + return &struct { + ID int64 `json:"id"` + }{ID: id} + } + cases := []struct { + name string + meta string + repo int64 + want bool + }{ + {"installer covers any repo", `{"repo_scope":"installer"}`, 9, true}, + {"no scope means installer", `{}`, 9, true}, + {"user scope covers its repo", `{"repo_scope":"user","allowed_repo_ids":[1,2]}`, 2, true}, + {"user scope skips other repos", `{"repo_scope":"user","allowed_repo_ids":[1,2]}`, 3, false}, + {"removed covers nothing", `{"removed":true}`, 1, false}, + {"suspended covers nothing", `{"suspended":true}`, 1, false}, + } + for _, tc := range cases { + if got := appConnectorCovers([]byte(tc.meta), repo(tc.repo)); got != tc.want { + t.Errorf("%s: got %v, want %v", tc.name, got, tc.want) + } + } + if !appConnectorCovers([]byte(`{"repo_scope":"user","allowed_repo_ids":[]}`), nil) { + t.Error("an event without a repository is for every live connector") + } +} + +func TestUserScopedRepoIDs(t *testing.T) { + user := &ConnectorDetails{TokenType: storagegithub.TokenTypeGitHubApp, Metadata: map[string]interface{}{ + "repo_scope": "user", "allowed_repo_ids": []interface{}{float64(101), float64(202)}, + }} + allowed, scoped := userScopedRepoIDs(user) + if !scoped || len(allowed) != 2 || !allowed["101"] || !allowed["202"] { + t.Fatalf("user scope: %v %v", allowed, scoped) + } + installer := &ConnectorDetails{TokenType: storagegithub.TokenTypeGitHubApp, Metadata: map[string]interface{}{"repo_scope": "installer"}} + if _, scoped := userScopedRepoIDs(installer); scoped { + t.Error("an installer connector syncs every repository") + } + pat := &ConnectorDetails{TokenType: "PAT", Metadata: map[string]interface{}{"repo_scope": "user"}} + if _, scoped := userScopedRepoIDs(pat); scoped { + t.Error("PAT connectors are never user-scoped") + } +} + +func TestGitHubAppWebhook_RepositoryNoOrgCoversIsIgnored(t *testing.T) { + f := newFakeGitHub(t) + s, db := newGitHubAppTestServer(t, f) + email := fmt.Sprintf("gh-hook-scope-%d@example.test", time.Now().UnixNano()) + cleanupGitHubAppTenant(t, db, email) + var orgID int64 + if err := db.QueryRow(`INSERT INTO orgs (name, description) VALUES ($1, 'gh hook test') RETURNING id`, email).Scan(&orgID); err != nil { + t.Fatal(err) + } + inst := time.Now().UnixNano() % 1_000_000_000_000 + if _, _, err := storagegithub.NewTokenStore(db).UpsertAppInstallationConnector(storagegithub.AppConnectorUpsert{ + OrgID: orgID, BaseURL: f.srv.URL, InstallationID: inst, AccountLogin: "acme", AccountType: "Organization", Token: "t", + ExpiresAt: time.Now().Add(time.Hour), RepoScope: storagegithub.RepoScopeUser, AllowedRepoIDs: []int64{1}, + }); err != nil { + t.Fatal(err) + } + // Repository 2 isn't one this org's user can access, so nothing is processed (and no stub repo is created). + body := fmt.Sprintf(`{"action":"opened","installation":{"id":%d},"repository":{"id":2},"pull_request":{"html_url":"http://x/acme/b/pull/1"}}`, inst) + if rec := s.doWebhook(t, "pull_request", body, "valid"); rec.Code != http.StatusOK || !strings.Contains(rec.Body.String(), "installation_inactive") { + t.Fatalf("uncovered repository: %d %s", rec.Code, rec.Body.String()) + } +} + func TestGitHubAppDemo_ScopedToOrg(t *testing.T) { f := newFakeGitHub(t) s, db := newGitHubAppTestServer(t, f) @@ -627,7 +799,7 @@ func TestGitHubAppDisconnect_OwnersOnlyAndOrgScoped(t *testing.T) { } call := func(org int64, owner bool) *httptest.ResponseRecorder { - req := httptest.NewRequest(http.MethodDelete, "/api/v1/github-app/installations/"+fmt.Sprint(connectorID), nil) + req := httptest.NewRequest(http.MethodDelete, "/api/v1/github-app/installations/"+fmt.Sprint(connectorID)+"?uninstall=true", nil) rec := httptest.NewRecorder() c := echo.New().NewContext(req, rec) c.SetParamNames("connector_id") @@ -673,6 +845,69 @@ func TestGitHubAppDisconnect_OwnersOnlyAndOrgScoped(t *testing.T) { } } +func TestGitHubAppDisconnect_SharedInstallation(t *testing.T) { + f := newFakeGitHub(t) + s, db := newGitHubAppTestServer(t, f) + store := storagegithub.NewTokenStore(db) + inst := time.Now().UnixNano() % 1_000_000_000_000 + mk := func(prefix string) (orgID, connectorID int64) { + name := fmt.Sprintf("%s-%d@example.test", prefix, time.Now().UnixNano()) + cleanupGitHubAppTenant(t, db, name) + if err := db.QueryRow(`INSERT INTO orgs (name, description) VALUES ($1, 'shared disconnect test') RETURNING id`, name).Scan(&orgID); err != nil { + t.Fatal(err) + } + connectorID, _, err := store.UpsertAppInstallationConnector(storagegithub.AppConnectorUpsert{ + OrgID: orgID, BaseURL: f.srv.URL, InstallationID: inst, AccountLogin: "acme", AccountType: "Organization", + Token: "ghs_live", ExpiresAt: time.Now().Add(time.Hour), + }) + if err != nil { + t.Fatal(err) + } + return orgID, connectorID + } + orgA, connA := mk("gh-shared-a") + orgB, connB := mk("gh-shared-b") + orgC, connC := mk("gh-shared-c") + _ = orgC + + call := func(org, connectorID int64, uninstall bool) *httptest.ResponseRecorder { + target := "/api/v1/github-app/installations/" + fmt.Sprint(connectorID) + if uninstall { + target += "?uninstall=true" + } + rec := httptest.NewRecorder() + c := echo.New().NewContext(httptest.NewRequest(http.MethodDelete, target, nil), rec) + c.SetParamNames("connector_id") + c.SetParamValues(fmt.Sprint(connectorID)) + c.Set("org_id", org) + c.Set(string(auth.PermissionContextKey), &auth.PermissionContext{IsOwner: true, OrgID: org}) + if err := s.GitHubAppDisconnect(c); err != nil { + t.Fatal(err) + } + return rec + } + removed := func(id int64) bool { + var r string + _ = db.QueryRow(`SELECT COALESCE(metadata->>'removed','false') FROM integration_tokens WHERE id = $1`, id).Scan(&r) + return r == "true" + } + + // Disconnect only: this org stops, GitHub and the other orgs are untouched. + if rec := call(orgA, connA, false); rec.Code != http.StatusOK { + t.Fatalf("disconnect: %d %s", rec.Code, rec.Body.String()) + } + if !removed(connA) || removed(connB) || removed(connC) || len(f.deleted) != 0 { + t.Fatalf("disconnect: A=%v B=%v C=%v github uninstalls=%v", removed(connA), removed(connB), removed(connC), f.deleted) + } + // Uninstall: GitHub uninstalls it, and every org's connector goes. + if rec := call(orgB, connB, true); rec.Code != http.StatusOK { + t.Fatalf("uninstall: %d %s", rec.Code, rec.Body.String()) + } + if !removed(connB) || !removed(connC) || len(f.deleted) != 1 || f.deleted[0] != fmt.Sprint(inst) { + t.Fatalf("uninstall: B=%v C=%v github uninstalls=%v", removed(connB), removed(connC), f.deleted) + } +} + func TestGitHubAppStart_TargetIDOpensInstallPageOnThatAccount(t *testing.T) { f := newFakeGitHub(t) s, _ := newGitHubAppTestServer(t, f) diff --git a/internal/api/github_app_state.go b/internal/api/github_app_state.go index 93149c28..83bb7a51 100644 --- a/internal/api/github_app_state.go +++ b/internal/api/github_app_state.go @@ -21,8 +21,9 @@ const ( ghStateTTL = 10 * time.Minute ghStateCookieName = "lr_gh_state" ghOrgCookieName = "lr_gh_org" // org id the UI should open after the GitHub callback (see index.tsx) - ghNextCookieName = "lr_gh_next" // where a returning user lands after login (see index.tsx) + ghNextCookieName = "lr_gh_next" // where the user lands after login (see index.tsx) ghNextGitProviders = "git" + ghNextGetStarted = "get-started" ghModeInstall = "install" ghModeSignin = "signin" ) @@ -34,6 +35,9 @@ type ghState struct { Src string `json:"s"` Mode string `json:"m"` Exp int64 `json:"e"` + // GetStarted: the round trip began on /gh-login, so a successful sign-in lands on get-started. Only the + // callback decides this (nothing is stored before leaving), so a failed attempt leaves no stale destination. + GetStarted bool `json:"g,omitempty"` } // sanitizeGHSrc keeps the campaign tag short and harmless; anything else becomes "unknown". diff --git a/internal/api/pull_request_review_trigger.go b/internal/api/pull_request_review_trigger.go index 5f6500a5..d8b92f43 100644 --- a/internal/api/pull_request_review_trigger.go +++ b/internal/api/pull_request_review_trigger.go @@ -10,6 +10,7 @@ import ( "github.com/labstack/echo/v4" "github.com/livereview/internal/api/auth" "github.com/livereview/pkg/models" + "github.com/livereview/storage/providers/pullrequests" ) // CreateReviewRequest is the body of POST /api/v1/reviews - trigger a new @@ -118,6 +119,8 @@ func (s *Server) createReview(c echo.Context) error { // getPullRequestOrg handles GET /api/v1/pull-requests/:id/org for the PR comment's // "Start a review" link, which doesn't know the PR's org yet. Auth-only route, so it // checks membership itself and returns 404 for both "no such PR" and "not a member". +// pull_request_id is the id to use in that org (another org's copy of the same PR when +// orgs share a GitHub App installation). func (s *Server) getPullRequestOrg(c echo.Context) error { user, ok := c.Get(string(auth.UserContextKey)).(*models.User) if !ok || user == nil { @@ -138,13 +141,19 @@ func (s *Server) getPullRequestOrg(c echo.Context) error { )`, prID, user.ID, auth.RoleSuperAdmin, ).Scan(&orgID) + resolvedID := prID + if err == sql.ErrNoRows { + // Orgs sharing a GitHub App installation each keep their own copy of the PR, and the link names one of + // them: open the copy in an org the user belongs to. + orgID, resolvedID, err = pullrequests.NewStore(s.db).SiblingPullRequestForUser(prID, user.ID) + } if err == sql.ErrNoRows { return c.JSON(http.StatusNotFound, ErrorResponse{Error: "pull request not found"}) } if err != nil { return c.JSON(http.StatusInternalServerError, ErrorResponse{Error: "failed to look up pull request"}) } - return JSONWithEnvelope(c, http.StatusOK, map[string]interface{}{"org_id": orgID}) + return JSONWithEnvelope(c, http.StatusOK, map[string]interface{}{"org_id": orgID, "pull_request_id": resolvedID}) } // markForPolicyCheck flags a manual PR review so the worker runs the approval policy check once it completes. diff --git a/internal/docindex/docs/routes_guide/auth/gh-login.md b/internal/docindex/docs/routes_guide/auth/gh-login.md index beab4127..1c7b1e6a 100644 --- a/internal/docindex/docs/routes_guide/auth/gh-login.md +++ b/internal/docindex/docs/routes_guide/auth/gh-login.md @@ -36,6 +36,17 @@ to [Get started with GitHub](../get-started-github.md). - **Returning users:** a plain GitHub sign-in by someone whose organization and GitHub connection already exist lands on the GitHub App card of [Git Providers](../git/git-providers.md) (`/git/github-app`), where their connected accounts are managed, instead of the first-run page. +- **The app is already installed by someone else** (for example a teammate + installed it on the GitHub organization from their own LiveReview account): + the new user's own LiveReview organization is connected to the same + installation, with no invite needed. It gets only the repositories that the + user's GitHub account can access; GitHub reports these at sign-in and the + list is refreshed on every GitHub sign-in. Reviews, the pull request list and + bot replies then work in their organization with their own AI provider. The + get-started page and the GitHub App card say the app was already installed. +- **A failed GitHub sign-in leaves nothing behind:** the landing page is decided + only when the sign-in succeeds, so choosing **Other ways to sign in** after an + error lands on the dashboard as usual, not on get-started. - **Other ways to sign in** — a link, with Google, LinkedIn and email icons, to the standard Hexmos sign-in, unchanged. - **`?src=launch-trial`** (the Product Hunt launch link): when this sign-in creates diff --git a/internal/docindex/docs/routes_guide/get-started-github.md b/internal/docindex/docs/routes_guide/get-started-github.md index 96db16d2..fba7b156 100644 --- a/internal/docindex/docs/routes_guide/get-started-github.md +++ b/internal/docindex/docs/routes_guide/get-started-github.md @@ -35,11 +35,20 @@ organization has a GitHub App connection. opens GitHub's install page so the app can be added to another account or organization (it lists the ones you can install on); after installing, the user returns here and the new account appears in the list. -- **Disconnect** (organization owners only) — next to each account: uninstalls +- When the app was already installed on the account by someone else, the + account says so and shows only the repositories the user's GitHub account can + access. When other LiveReview accounts use the same installation, it says how + many ("Also used by 1 other LiveReview account"). +- **Disconnect** (organization owners only) — next to each account: stops + LiveReview using it for this organization only and drops its repositories + here. The app stays installed on GitHub, so other LiveReview accounts using it + keep working. To connect it again, use **Add repositories or organization**. +- **Uninstall** (organization owners only) — next to each account: uninstalls the LiveReview GitHub App from that GitHub account or organization (using the - app's own credentials, so the user does not need to be a GitHub owner), stops - LiveReview using it for the organization, and drops its repositories. To use - it again, install the app on GitHub again. + app's own credentials, so the user does not need to be a GitHub owner). This + removes it for every LiveReview account using it; the confirmation says how + many other accounts are affected. To use it again, install the app on GitHub + again. - **Configure AI** — opens AI Providers in a new tab when no provider is configured. A link to Google AI Studio explains where to get a free Gemini API key. diff --git a/internal/docindex/docs/routes_guide/git/git-providers.md b/internal/docindex/docs/routes_guide/git/git-providers.md index 353a06d4..0f98222d 100644 --- a/internal/docindex/docs/routes_guide/git/git-providers.md +++ b/internal/docindex/docs/routes_guide/git/git-providers.md @@ -40,14 +40,25 @@ repositories come from the installation, and webhooks are delivered by the app, so the manual-trigger buttons are replaced by a note. Repositories are added or removed on GitHub, not here. +Several LiveReview organizations can use the same installation (for example two +people from one GitHub organization who each signed up). Each has its own +connection. An organization that joined an installation someone else installed +only gets the repositories its user's GitHub account can access. Pull request +updates reach every organization that has the repository; a reply to a comment +on GitHub comes from the organization that reviewed that pull request most +recently. + ### The GitHub App card On LiveReview Cloud the **Select Git Provider** grid starts with a **GitHub App** card (marked Recommended). It opens `/git/github-app`, which is titled **GitHub App** and lists every connected GitHub account or organization as a panel (name, repository count, repositories as chips; a toggle at the top right of the repositories switches to a searchable list that shows five rows and scrolls), a **Manage on GitHub** button (opens that account's installation settings; -GitHub shows it only to owners), and, for organization owners, **Disconnect**, -which uninstalls the app from that account and drops its repositories. +GitHub shows it only to owners), and, for organization owners, **Disconnect** +(stops using it in this organization only; the app stays installed on GitHub +for anyone else using it) and **Uninstall** (uninstalls the app from that GitHub +account, which removes it for every LiveReview account using it; the +confirmation says how many other accounts are affected). **Add repositories or organization** opens GitHub's install page; afterwards the user returns to `/git/github-app` and the new account appears. With nothing connected yet the page shows **Install the diff --git a/internal/docindex/docs/routes_guide/reviews/pr-review-link.md b/internal/docindex/docs/routes_guide/reviews/pr-review-link.md index f776f703..f6a80708 100644 --- a/internal/docindex/docs/routes_guide/reviews/pr-review-link.md +++ b/internal/docindex/docs/routes_guide/reviews/pr-review-link.md @@ -28,8 +28,11 @@ already existed, reopens, or pushes). Any authenticated member of the org the PR belongs to. Logged-out users are asked to log in and then sent back to the link. If the user's active org is a -different one, LiveReview switches to the PR's org automatically. Users who -aren't members of that org see "pull request wasn't found". +different one, LiveReview switches to the PR's org automatically. When several +organizations share one GitHub App installation, each keeps its own copy of the +PR and the link names one of them; a user from another of those organizations +is taken to their own organization's copy, and the review runs there. Users who +aren't members of any organization with that PR see "pull request wasn't found". ## Key actions diff --git a/internal/docindex/docs/routes_guide/reviews/reviews-list.md b/internal/docindex/docs/routes_guide/reviews/reviews-list.md index 332252f4..14717ef0 100644 --- a/internal/docindex/docs/routes_guide/reviews/reviews-list.md +++ b/internal/docindex/docs/routes_guide/reviews/reviews-list.md @@ -22,7 +22,8 @@ Any authenticated org member. - Navigate to scheduled reviews (`/reviews/scheduled`). - View one pull request's review history: `/reviews?pull_request_id=` (the "View review history" link LiveReview adds to the description of new GitHub/GitLab - PRs). Switches to the PR's org if needed. A "Filtered to one pull request" + PRs). Switches to the PR's org if needed; when organizations share a GitHub + App installation, it opens the user's own organization's copy of that PR. A "Filtered to one pull request" chip shows above the table; click **Clear** to see all reviews again. ## Related pages diff --git a/internal/githubapp/token_test.go b/internal/githubapp/token_test.go index f50eca26..d6629812 100644 --- a/internal/githubapp/token_test.go +++ b/internal/githubapp/token_test.go @@ -5,6 +5,7 @@ import ( "crypto/rand" "crypto/rsa" "database/sql" + "encoding/json" "fmt" "net/http" "net/http/httptest" @@ -53,7 +54,7 @@ func ghes(t *testing.T, handler http.HandlerFunc) (*Config, *httptest.Server) { func uniqueInstallation() int64 { return time.Now().UnixNano() % 1_000_000_000_000 } -func TestUpsertAppInstallationConnector_OneOrgPerInstallation(t *testing.T) { +func TestUpsertAppInstallationConnector_OneConnectorPerOrg(t *testing.T) { db := testDB(t) store := storagegithub.NewTokenStore(db) suffix := time.Now().UnixNano() @@ -79,12 +80,95 @@ func TestUpsertAppInstallationConnector_OneOrgPerInstallation(t *testing.T) { t.Fatalf("stored row wrong: %+v err=%v", rec, err) } - in.OrgID = orgB - if _, _, err := store.UpsertAppInstallationConnector(in); err != storagegithub.ErrInstallationBoundToOtherOrg { - t.Fatalf("foreign org upsert err = %v, want ErrInstallationBoundToOtherOrg", err) + // Another org gets its own connector for the same installation; the first one is untouched. + in.OrgID, in.Token = orgB, "ghs_b" + idB, created, err := store.UpsertAppInstallationConnector(in) + if err != nil || !created || idB == id { + t.Fatalf("other org upsert = (%d, %v, %v), want a new connector", idB, created, err) } - if rec, _ = store.GetGitHubTokenByConnectorID(id); rec.OrgID != orgA { - t.Fatalf("installation moved to org %d", rec.OrgID) + if rec, _ = store.GetGitHubTokenByConnectorID(id); rec.OrgID != orgA || rec.PatToken != "ghs_two" { + t.Fatalf("first org's connector changed: %+v", rec) + } + recs, err := store.ListAppConnectorsByInstallation("https://ghes.test", inst) + if err != nil || len(recs) != 2 || recs[0].ID != id || recs[1].ID != idB { + t.Fatalf("ListAppConnectorsByInstallation = %v, %v; want [%d %d]", recs, err, id, idB) + } + if n, err := store.CountOtherOrgsUsingInstallation("https://ghes.test", inst, orgA); err != nil || n != 1 { + t.Fatalf("CountOtherOrgsUsingInstallation = %d, %v; want 1", n, err) + } +} + +func scopeOf(t *testing.T, store *storagegithub.TokenStore, id int64) (scope string, allowed []int64, removed bool) { + t.Helper() + rec, err := store.GetGitHubTokenByConnectorID(id) + if err != nil { + t.Fatal(err) + } + var meta struct { + RepoScope string `json:"repo_scope"` + AllowedRepoIDs []int64 `json:"allowed_repo_ids"` + Removed bool `json:"removed"` + } + if err := json.Unmarshal(rec.Metadata, &meta); err != nil { + t.Fatal(err) + } + return meta.RepoScope, meta.AllowedRepoIDs, meta.Removed +} + +func TestUpsertAppInstallationConnector_RepoScope(t *testing.T) { + db := testDB(t) + store := storagegithub.NewTokenStore(db) + org := testOrg(t, db, fmt.Sprintf("ghapp-scope-%d", time.Now().UnixNano())) + in := storagegithub.AppConnectorUpsert{ + OrgID: org, BaseURL: "https://ghes.test", InstallationID: uniqueInstallation(), AccountLogin: "acme", + AccountType: "Organization", Token: "t", ExpiresAt: time.Now().Add(time.Hour), + RepoScope: storagegithub.RepoScopeUser, AllowedRepoIDs: []int64{11, 22}, KeepDisconnected: true, + } + id, _, err := store.UpsertAppInstallationConnector(in) + if err != nil { + t.Fatal(err) + } + if scope, allowed, _ := scopeOf(t, store, id); scope != storagegithub.RepoScopeUser || len(allowed) != 2 { + t.Fatalf("scope = %q %v, want user [11 22]", scope, allowed) + } + + // A later sign-in refreshes the list. + in.AllowedRepoIDs = []int64{22} + if _, _, err := store.UpsertAppInstallationConnector(in); err != nil { + t.Fatal(err) + } + if _, allowed, _ := scopeOf(t, store, id); len(allowed) != 1 || allowed[0] != 22 { + t.Fatalf("allowed = %v, want [22]", allowed) + } + + // Installing/configuring widens it to installer; a plain sign-in afterwards doesn't narrow it again. + installer := in + installer.RepoScope, installer.AllowedRepoIDs, installer.KeepDisconnected = storagegithub.RepoScopeInstaller, nil, false + if _, _, err := store.UpsertAppInstallationConnector(installer); err != nil { + t.Fatal(err) + } + if _, _, err := store.UpsertAppInstallationConnector(in); err != nil { + t.Fatal(err) + } + if scope, allowed, _ := scopeOf(t, store, id); scope != storagegithub.RepoScopeInstaller || allowed != nil { + t.Fatalf("scope = %q %v, want installer with no allow list", scope, allowed) + } + + // Disconnected by the org: plain sign-ins leave it alone, installing again reconnects it. + if err := store.DisconnectAppConnector(id); err != nil { + t.Fatal(err) + } + if _, _, err := store.UpsertAppInstallationConnector(in); err != storagegithub.ErrAppConnectorDisconnected { + t.Fatalf("plain sign-in after disconnect: err = %v, want ErrAppConnectorDisconnected", err) + } + if _, _, removed := scopeOf(t, store, id); !removed { + t.Fatal("a plain sign-in reconnected a disconnected installation") + } + if _, _, err := store.UpsertAppInstallationConnector(installer); err != nil { + t.Fatal(err) + } + if _, _, removed := scopeOf(t, store, id); removed { + t.Fatal("installing again did not reconnect it") } } diff --git a/internal/jobqueue/repo_sync_worker.go b/internal/jobqueue/repo_sync_worker.go index d9723061..accbba93 100644 --- a/internal/jobqueue/repo_sync_worker.go +++ b/internal/jobqueue/repo_sync_worker.go @@ -207,6 +207,8 @@ type PRStateSyncJobArgs struct { ConnectorID int64 `json:"connector_id"` Provider string `json:"provider"` Event prsync.PullRequestStateEvent `json:"event"` + // SkipLinks: another org's connector for the same GitHub App installation writes the PR description links. + SkipLinks bool `json:"skip_links,omitempty"` } func (PRStateSyncJobArgs) Kind() string { return "pr_state_sync" } @@ -271,7 +273,7 @@ func (w *PRStateSyncWorker) Work(ctx context.Context, job *river.Job[PRStateSync return fmt.Errorf("failed to upsert pull request from webhook event: %w", err) } - if action, _ := event.Metadata["action"].(string); action == "opened" || action == "open" { + if action, _ := event.Metadata["action"].(string); !args.SkipLinks && (action == "opened" || action == "open") { return w.postLinksComment(ctx, repositoryID, pullRequestID, event.Number) } return nil @@ -565,12 +567,17 @@ func (jq *JobQueue) QueueRepoPRSyncJob(ctx context.Context, repositoryID int64, // QueuePRStateSyncJob enqueues an upsert for a single PR/MR whose state a // webhook delivery just reported, on the "repo_sync" queue. func (jq *JobQueue) QueuePRStateSyncJob(ctx context.Context, orgID, connectorID int64, provider string, event prsync.PullRequestStateEvent) error { - _, err := jq.client.Insert(ctx, PRStateSyncJobArgs{ - OrgID: orgID, - ConnectorID: connectorID, - Provider: provider, - Event: event, - }, &river.InsertOpts{Queue: "repo_sync", MaxAttempts: 5}) + return jq.queuePRStateSync(ctx, PRStateSyncJobArgs{OrgID: orgID, ConnectorID: connectorID, Provider: provider, Event: event}) +} + +// QueueSharedPRStateSyncJob updates one more org's copy of a PR whose GitHub App installation several orgs share; +// it never writes the PR description links (the installation's primary connector does that once). +func (jq *JobQueue) QueueSharedPRStateSyncJob(ctx context.Context, orgID, connectorID int64, provider string, event prsync.PullRequestStateEvent) error { + return jq.queuePRStateSync(ctx, PRStateSyncJobArgs{OrgID: orgID, ConnectorID: connectorID, Provider: provider, Event: event, SkipLinks: true}) +} + +func (jq *JobQueue) queuePRStateSync(ctx context.Context, args PRStateSyncJobArgs) error { + _, err := jq.client.Insert(ctx, args, &river.InsertOpts{Queue: "repo_sync", MaxAttempts: 5}) if err != nil { return fmt.Errorf("failed to queue PR state sync job: %w", err) } diff --git a/network/network_status.md b/network/network_status.md index bb5f7445..2eddbefb 100644 --- a/network/network_status.md +++ b/network/network_status.md @@ -71,15 +71,16 @@ Latest milestone batch note (MF-051, MF-059, MF-073, MF-074, MF-076, MF-083, MF- | api.selectLeaderAIConfig | added | [selectLeaderAIConfig](../internal/api/reviews_api.go#L378) | | api.selectHelperAIConfig | added | [selectHelperAIConfig](../internal/api/reviews_api.go#L414) | | api.GetReviewAccounting | updated | [GetReviewAccounting](../internal/api/review_events_endpoints.go#L207) | -| providersgithub.MintAppJWT | added | [MintAppJWT](providers/github/app.go#L101) | -| providersgithub.CreateInstallationToken | added | [CreateInstallationToken](providers/github/app.go#L116) | -| providersgithub.ExchangeUserCode | added | [ExchangeUserCode](providers/github/app.go#L137) | -| providersgithub.ListUserEmails | added | [ListUserEmails](providers/github/app.go#L167) | -| providersgithub.GetAuthenticatedUser | added | [GetAuthenticatedUser](providers/github/app.go#L183) | -| providersgithub.ListUserInstallations | added | [ListUserInstallations](providers/github/app.go#L201) | -| providersgithub.ListInstallationRepositories | added | [ListInstallationRepositories](providers/github/app.go#L246) | -| providersgithub.DeleteInstallation | added | [DeleteInstallation](providers/github/app.go#L310) | -| providersgithub.ListOpenPulls | added | [ListOpenPulls](providers/github/app.go#L269) | -| providersgithub.GetUserByLogin | added | [GetUserByLogin](providers/github/app.go#L299) | +| providersgithub.MintAppJWT | added | [MintAppJWT](providers/github/app.go#L102) | +| providersgithub.CreateInstallationToken | added | [CreateInstallationToken](providers/github/app.go#L117) | +| providersgithub.ExchangeUserCode | added | [ExchangeUserCode](providers/github/app.go#L138) | +| providersgithub.ListUserEmails | added | [ListUserEmails](providers/github/app.go#L168) | +| providersgithub.GetAuthenticatedUser | added | [GetAuthenticatedUser](providers/github/app.go#L184) | +| providersgithub.ListUserInstallations | added | [ListUserInstallations](providers/github/app.go#L202) | +| providersgithub.ListUserInstallationRepositoryIDs | added | [ListUserInstallationRepositoryIDs](providers/github/app.go#L235) | +| providersgithub.ListInstallationRepositories | added | [ListInstallationRepositories](providers/github/app.go#L273) | +| providersgithub.DeleteInstallation | added | [DeleteInstallation](providers/github/app.go#L336) | +| providersgithub.ListOpenPulls | added | [ListOpenPulls](providers/github/app.go#L296) | +| providersgithub.GetUserByLogin | added | [GetUserByLogin](providers/github/app.go#L326) | | hexmos.ExternalProviderSignup | added | [ExternalProviderSignup](hexmos/parse_client.go#L42) | | hexmos.ParseAppID | added | [ParseAppID](hexmos/parse_client.go#L17) | diff --git a/network/providers/github/app.go b/network/providers/github/app.go index 9a667472..5275628e 100644 --- a/network/providers/github/app.go +++ b/network/providers/github/app.go @@ -230,6 +230,32 @@ func ListUserInstallations(ctx context.Context, apiBase, userToken string) ([]In return all, nil } +// ListUserInstallationRepositoryIDs returns the ids of the installation's repositories that the user can access, +// following pagination (GitHub decides access: org base permission, teams, collaborator grants). +func ListUserInstallationRepositoryIDs(ctx context.Context, apiBase, userToken string, installationID int64) ([]int64, error) { + ids := []int64{} + next := fmt.Sprintf("%s/user/installations/%d/repositories?per_page=100", apiBase, installationID) + for page := 0; next != ""; page++ { + if page == 100 { + return nil, fmt.Errorf("installation %d lists more than 10000 repositories for this user", installationID) + } + var out struct { + Repositories []struct { + ID int64 `json:"id"` + } `json:"repositories"` + } + resp, err := doJSON(ctx, http.MethodGet, next, "Bearer "+userToken, nil, http.StatusOK, &out) + if err != nil { + return nil, err + } + for _, r := range out.Repositories { + ids = append(ids, r.ID) + } + next = nextLink(resp, apiBase) + } + return ids, nil +} + type InstallationRepo struct { ID int64 `json:"id"` Name string `json:"name"` diff --git a/network/providers/github/app_test.go b/network/providers/github/app_test.go index a36c89d7..6663688d 100644 --- a/network/providers/github/app_test.go +++ b/network/providers/github/app_test.go @@ -95,6 +95,31 @@ func TestListUserInstallations_FollowsPagesAndMapsFields(t *testing.T) { } } +func TestListUserInstallationRepositoryIDs_FollowsPages(t *testing.T) { + var srv *httptest.Server + srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("Authorization") != "Bearer ghu_x" || r.URL.Path != "/user/installations/42/repositories" { + http.Error(w, "unexpected request", http.StatusBadRequest) + return + } + if r.URL.Query().Get("page") == "2" { + _, _ = fmt.Fprint(w, `{"total_count":3,"repositories":[{"id":30}]}`) + return + } + w.Header().Set("Link", fmt.Sprintf(`<%s/user/installations/42/repositories?per_page=100&page=2>; rel="next"`, srv.URL)) + _, _ = fmt.Fprint(w, `{"total_count":3,"repositories":[{"id":10},{"id":20}]}`) + })) + defer srv.Close() + + got, err := ListUserInstallationRepositoryIDs(context.Background(), srv.URL, "ghu_x", 42) + if err != nil { + t.Fatal(err) + } + if len(got) != 3 || got[0] != 10 || got[1] != 20 || got[2] != 30 { + t.Fatalf("got %v, want [10 20 30]", got) + } +} + func TestNextLink_IgnoresOtherHosts(t *testing.T) { resp := &http.Response{Header: http.Header{"Link": []string{`; rel="next"`}}} if got := nextLink(resp, "https://api.github.com"); got != "" { diff --git a/storage/providers/github/token_store.go b/storage/providers/github/token_store.go index f769fe03..7edab92c 100644 --- a/storage/providers/github/token_store.go +++ b/storage/providers/github/token_store.go @@ -6,13 +6,27 @@ import ( "errors" "fmt" "time" + + "github.com/lib/pq" ) // TokenTypeGitHubApp marks an integration_tokens row that represents a GitHub App installation. const TokenTypeGitHubApp = "GITHUB_APP" -// ErrInstallationBoundToOtherOrg is returned when an installation already belongs to a different org. -var ErrInstallationBoundToOtherOrg = errors.New("github app installation is connected to another LiveReview organization") +// Repository scope of a GitHub App connector (metadata "repo_scope"). One installation can be connected to several +// orgs, one connector each. +const ( + // RepoScopeInstaller syncs every repository the installation covers. The install/configure callback creates it + // (only someone allowed to install the App gets there), and connectors without a scope are installer ones. + RepoScopeInstaller = "installer" + // RepoScopeUser syncs only "allowed_repo_ids": the repositories the GitHub user who connected it can access. + // Plain sign-ins that merely see an installation create it. + RepoScopeUser = "user" +) + +// ErrAppConnectorDisconnected is returned when an org disconnected this installation itself and the upsert asked +// not to reconnect it (plain sign-ins); only installing or configuring the App again reconnects it. +var ErrAppConnectorDisconnected = errors.New("github app installation was disconnected from this organization") type IntegrationTokenRecord struct { ID int64 @@ -77,11 +91,17 @@ type AppConnectorUpsert struct { AppSlug string Token string ExpiresAt time.Time + // RepoScope is RepoScopeInstaller ("" means the same) or RepoScopeUser with AllowedRepoIDs. An installer + // connector is never narrowed to a user one; a user connector becomes installer when the org installs or + // configures the App. + RepoScope string + AllowedRepoIDs []int64 + // KeepDisconnected leaves a connector the org disconnected alone (ErrAppConnectorDisconnected). + KeepDisconnected bool } -// UpsertAppInstallationConnector inserts or refreshes the GITHUB_APP row for (BaseURL, InstallationID). -// An installation belongs to exactly one org: if the row exists for a different org it returns -// ErrInstallationBoundToOtherOrg and changes nothing. +// UpsertAppInstallationConnector inserts or refreshes the org's GITHUB_APP row for (BaseURL, InstallationID). +// Other orgs may have their own connector for the same installation. func (s *TokenStore) UpsertAppInstallationConnector(in AppConnectorUpsert) (connectorID int64, created bool, err error) { tx, err := s.db.Begin() if err != nil { @@ -90,7 +110,7 @@ func (s *TokenStore) UpsertAppInstallationConnector(in AppConnectorUpsert) (conn defer tx.Rollback() appID := fmt.Sprintf("%d", in.InstallationID) - meta, err := json.Marshal(map[string]interface{}{ + fields := map[string]interface{}{ "github_app": true, "installation_id": in.InstallationID, "account_login": in.AccountLogin, @@ -99,33 +119,53 @@ func (s *TokenStore) UpsertAppInstallationConnector(in AppConnectorUpsert) (conn "app_slug": in.AppSlug, "removed": false, "suspended": false, - }) - if err != nil { - return 0, false, err + "disconnected": false, } - var existingOrg int64 + var existingMeta []byte err = tx.QueryRow(` - SELECT id, org_id FROM integration_tokens - WHERE token_type = $1 AND provider_url = $2 AND provider_app_id = $3 + SELECT id, COALESCE(metadata, '{}') FROM integration_tokens + WHERE token_type = $1 AND provider_url = $2 AND provider_app_id = $3 AND org_id = $4 FOR UPDATE - `, TokenTypeGitHubApp, in.BaseURL, appID).Scan(&connectorID, &existingOrg) + `, TokenTypeGitHubApp, in.BaseURL, appID, in.OrgID).Scan(&connectorID, &existingMeta) switch { case err == nil: - if existingOrg != in.OrgID { - return 0, false, ErrInstallationBoundToOtherOrg + var prev struct { + RepoScope string `json:"repo_scope"` + Removed bool `json:"removed"` + Disconnected bool `json:"disconnected"` + } + _ = json.Unmarshal(existingMeta, &prev) + if in.KeepDisconnected && prev.Removed && prev.Disconnected { + return connectorID, false, ErrAppConnectorDisconnected + } + // A user-scoped refresh of a live installer connector keeps it installer. A removed one starts over, so + // a scope it had before an uninstall doesn't come back. + scope := in.RepoScope + if scope == RepoScopeUser && !prev.Removed && (prev.RepoScope == "" || prev.RepoScope == RepoScopeInstaller) { + scope = RepoScopeInstaller + } + setScope(fields, scope, in.AllowedRepoIDs) + meta, err := json.Marshal(fields) + if err != nil { + return 0, false, err } _, err = tx.Exec(` UPDATE integration_tokens SET pat_token = $1, expires_at = $2, connection_name = $3, - metadata = COALESCE(metadata, '{}'::jsonb) || $4::jsonb, updated_at = NOW() + metadata = (COALESCE(metadata, '{}'::jsonb) - 'allowed_repo_ids') || $4::jsonb, updated_at = NOW() WHERE id = $5 `, in.Token, in.ExpiresAt, "GitHub App · "+in.AccountLogin, string(meta), connectorID) if err != nil { return 0, false, err } case errors.Is(err, sql.ErrNoRows): - // The unique index uq_integration_tokens_github_app_installation turns a racing insert into an error. + setScope(fields, in.RepoScope, in.AllowedRepoIDs) + meta, err := json.Marshal(fields) + if err != nil { + return 0, false, err + } + // The unique index uq_integration_tokens_github_app_installation_org turns a racing insert into an error. err = tx.QueryRow(` INSERT INTO integration_tokens (provider, provider_app_id, access_token, token_type, pat_token, expires_at, connection_name, @@ -147,12 +187,59 @@ func (s *TokenStore) UpsertAppInstallationConnector(in AppConnectorUpsert) (conn return connectorID, created, nil } -// GetAppConnectorByInstallation finds the connector for an installation id on a GitHub host. -func (s *TokenStore) GetAppConnectorByInstallation(baseURL string, installationID int64) (*IntegrationTokenRecord, error) { +// setScope writes repo_scope (and allowed_repo_ids for a user scope) into the metadata fields. +func setScope(fields map[string]interface{}, scope string, allowed []int64) { + if scope != RepoScopeUser { + fields["repo_scope"] = RepoScopeInstaller + return + } + if allowed == nil { + allowed = []int64{} + } + fields["repo_scope"] = RepoScopeUser + fields["allowed_repo_ids"] = allowed +} + +// GetAppConnectorByInstallation finds the org's connector for an installation id on a GitHub host. +func (s *TokenStore) GetAppConnectorByInstallation(baseURL string, installationID, orgID int64) (*IntegrationTokenRecord, error) { return scanRecord(s.db.QueryRow(` + SELECT `+appConnectorColumns+` FROM integration_tokens + WHERE token_type = $1 AND provider_url = $2 AND provider_app_id = $3 AND org_id = $4 + `, TokenTypeGitHubApp, baseURL, fmt.Sprintf("%d", installationID), orgID)) +} + +// ListAppConnectorsByInstallation returns every org's connector for an installation id on a GitHub host, oldest +// first (removed and suspended ones included; callers filter). +func (s *TokenStore) ListAppConnectorsByInstallation(baseURL string, installationID int64) ([]IntegrationTokenRecord, error) { + return s.listRecords(` SELECT `+appConnectorColumns+` FROM integration_tokens WHERE token_type = $1 AND provider_url = $2 AND provider_app_id = $3 - `, TokenTypeGitHubApp, baseURL, fmt.Sprintf("%d", installationID))) + ORDER BY id + `, TokenTypeGitHubApp, baseURL, fmt.Sprintf("%d", installationID)) +} + +// CountOtherOrgsUsingInstallation counts the other orgs with a live (not removed) connector for the installation. +func (s *TokenStore) CountOtherOrgsUsingInstallation(baseURL string, installationID, orgID int64) (int, error) { + var n int + err := s.db.QueryRow(` + SELECT COUNT(*) FROM integration_tokens + WHERE token_type = $1 AND provider_url = $2 AND provider_app_id = $3 AND org_id <> $4 + AND COALESCE(metadata->>'removed', 'false') <> 'true' + `, TokenTypeGitHubApp, baseURL, fmt.Sprintf("%d", installationID), orgID).Scan(&n) + return n, err +} + +// LatestReviewConnector returns which of the connectors ran the most recent review of the PR/MR at prURL; +// sql.ErrNoRows when none of them reviewed it. +func (s *TokenStore) LatestReviewConnector(connectorIDs []int64, prURL string) (int64, error) { + var id int64 + err := s.db.QueryRow(` + SELECT connector_id FROM reviews + WHERE connector_id = ANY($1) AND (pr_mr_url = $2 OR pr_mr_url LIKE $2 || '/%') + ORDER BY created_at DESC, id DESC + LIMIT 1 + `, pq.Array(connectorIDs), prURL).Scan(&id) + return id, err } // UpdateCachedInstallationToken stores a freshly minted installation token on its connector. @@ -230,6 +317,18 @@ func (s *TokenStore) SetAppConnectorFlags(connectorID int64, removed, suspended return err } +// DisconnectAppConnector stops one org using an installation without uninstalling it: the connector is marked +// removed and disconnected (plain sign-ins then leave it alone) and its cached token is cleared. +func (s *TokenStore) DisconnectAppConnector(connectorID int64) error { + _, err := s.db.Exec(` + UPDATE integration_tokens + SET metadata = COALESCE(metadata, '{}'::jsonb) || '{"removed": true, "disconnected": true}'::jsonb, + pat_token = '', updated_at = NOW() + WHERE id = $1 AND token_type = $2 + `, connectorID, TokenTypeGitHubApp) + return err +} + // FindPreferredConnectorForRepo returns the org's GitHub connector that has the repository, preferring a // GitHub App connector over a PAT one (the App wins on overlap). sql.ErrNoRows means no connector lists it. func (s *TokenStore) FindPreferredConnectorForRepo(orgID int64, repoFullName string) (int64, error) { @@ -261,9 +360,10 @@ func (s *TokenStore) HasAppConnectorForRepo(orgID int64, repoFullName string) (b return exists, err } -// HasAppConnectorForOwnerAllRepos reports whether the org has a live GitHub App connector installed on the account -// with access to ALL of its repositories. The repositories table is filled by a background sync after install, so -// this answers right away, before the sync lists a repository. +// HasAppConnectorForOwnerAllRepos reports whether the org has a live installer GitHub App connector installed on the +// account with access to ALL of its repositories (a user-scoped one only covers what its user can access). The +// repositories table is filled by a background sync after install, so this answers right away, before the sync +// lists a repository. func (s *TokenStore) HasAppConnectorForOwnerAllRepos(orgID int64, owner string) (bool, error) { var exists bool err := s.db.QueryRow(` @@ -272,6 +372,7 @@ func (s *TokenStore) HasAppConnectorForOwnerAllRepos(orgID int64, owner string) WHERE it.org_id = $1 AND it.token_type = $2 AND LOWER(it.metadata->>'account_login') = LOWER($3) AND it.metadata->>'repository_selection' = 'all' + AND COALESCE(it.metadata->>'repo_scope', 'installer') <> 'user' AND COALESCE(it.metadata->>'removed', 'false') <> 'true' AND COALESCE(it.metadata->>'suspended', 'false') <> 'true' ) diff --git a/storage/providers/github/token_store_test.go b/storage/providers/github/token_store_test.go index 3b90dd98..76818c34 100644 --- a/storage/providers/github/token_store_test.go +++ b/storage/providers/github/token_store_test.go @@ -131,3 +131,44 @@ func TestListAppConnectorsExpiringBefore_SkipsRemovedAndSuspended(t *testing.T) } } } + +func TestLatestReviewConnector_PicksTheMostRecentReviewAmongTheConnectors(t *testing.T) { + db := testDB(t) + store := NewTokenStore(db) + orgA, orgB := testOrg(t, db), testOrg(t, db) + inst := time.Now().UnixNano() % 1_000_000_000_000 + mk := func(org int64) int64 { + id, _, err := store.UpsertAppInstallationConnector(AppConnectorUpsert{ + OrgID: org, BaseURL: "https://github.com.test", InstallationID: inst, AccountLogin: "acme", AccountType: "Organization", + Token: "t", ExpiresAt: time.Now().Add(time.Hour), + }) + if err != nil { + t.Fatal(err) + } + return id + } + connA, connB := mk(orgA), mk(orgB) + prURL := fmt.Sprintf("https://github.test/acme/api/pull/%d", inst) + review := func(org, connector int64, url string, at time.Time) { + var id int64 + if err := db.QueryRow(` + INSERT INTO reviews (repository, pr_mr_url, connector_id, org_id, created_at) VALUES ('acme/api', $1, $2, $3, $4) RETURNING id`, + url, connector, org, at).Scan(&id); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _, _ = db.Exec(`DELETE FROM reviews WHERE id = $1`, id) }) + } + + if _, err := store.LatestReviewConnector([]int64{connA, connB}, prURL); err != sql.ErrNoRows { + t.Fatalf("no reviews yet: err = %v, want sql.ErrNoRows", err) + } + now := time.Now() + review(orgA, connA, prURL, now.Add(-time.Hour)) + review(orgB, connB, prURL+"/files", now) // a URL pasted with a sub-page still counts + if got, err := store.LatestReviewConnector([]int64{connA, connB}, prURL); err != nil || got != connB { + t.Fatalf("got (%d, %v), want the latest reviewer %d", got, err, connB) + } + if got, err := store.LatestReviewConnector([]int64{connA}, prURL); err != nil || got != connA { + t.Fatalf("restricted to A: got (%d, %v), want %d", got, err, connA) + } +} diff --git a/storage/providers/pullrequests/upsert.go b/storage/providers/pullrequests/upsert.go index dfde0076..674cfbdf 100644 --- a/storage/providers/pullrequests/upsert.go +++ b/storage/providers/pullrequests/upsert.go @@ -8,6 +8,8 @@ import ( "database/sql" "encoding/json" "time" + + "github.com/lib/pq" ) type Store struct { @@ -198,6 +200,36 @@ func (s *Store) DeleteRepositoryByProviderID(connectorID int64, providerRepoID s return err } +// DeleteRepositoriesByConnectorExcept removes the connector's repositories whose provider id isn't in keep (a +// user-scoped GitHub App connector whose user lost access to some). +func (s *Store) DeleteRepositoriesByConnectorExcept(connectorID int64, keep []string) error { + if keep == nil { + keep = []string{} + } + _, err := s.db.Exec(`DELETE FROM repositories WHERE connector_id = $1 AND NOT (provider_repo_id = ANY($2))`, connectorID, pq.Array(keep)) + return err +} + +// SiblingPullRequestForUser finds the same pull request (same provider host and provider PR id) as prID in another +// org the user belongs to, for links that carry one org's pull_requests id while orgs share a GitHub App +// installation. It only finds PRs the user's org already synced, so it grants no new access. sql.ErrNoRows when none. +func (s *Store) SiblingPullRequestForUser(prID, userID int64) (orgID, siblingID int64, err error) { + err = s.db.QueryRow(` + SELECT sib.org_id, sib.id + FROM pull_requests pr + JOIN repositories r ON r.id = pr.repository_id + JOIN integration_tokens t ON t.id = r.connector_id + JOIN pull_requests sib ON sib.provider = pr.provider AND sib.provider_pr_id = pr.provider_pr_id AND sib.id <> pr.id + JOIN repositories sr ON sr.id = sib.repository_id + JOIN integration_tokens st ON st.id = sr.connector_id AND st.provider_url = t.provider_url + WHERE pr.id = $1 + AND EXISTS (SELECT 1 FROM user_roles ur WHERE ur.user_id = $2 AND ur.org_id = sib.org_id) + ORDER BY sib.id + LIMIT 1 + `, prID, userID).Scan(&orgID, &siblingID) + return orgID, siblingID, err +} + // ListRepositoryFullNamesByConnector returns the full names of a connector's repositories, sorted. func (s *Store) ListRepositoryFullNamesByConnector(connectorID int64) ([]string, error) { rows, err := s.db.Query(`SELECT full_name FROM repositories WHERE connector_id = $1 ORDER BY full_name`, connectorID) diff --git a/storage/providers/pullrequests/upsert_test.go b/storage/providers/pullrequests/upsert_test.go index 08e60c76..ce826d48 100644 --- a/storage/providers/pullrequests/upsert_test.go +++ b/storage/providers/pullrequests/upsert_test.go @@ -258,3 +258,90 @@ func TestUpsertPullRequest_ConcurrentUpsertsDoNotDuplicate(t *testing.T) { t.Fatalf("expected exactly 1 row after concurrent upserts, got %d", count) } } + +func TestSiblingPullRequestForUser_OnlyInTheUsersOrgs(t *testing.T) { + db := testDB(t) + store := NewStore(db) + + mkOrg := func(name string) int64 { + var id int64 + if err := db.QueryRow(`INSERT INTO orgs (name, description) VALUES ($1, 'sibling test') RETURNING id`, name).Scan(&id); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _, _ = db.Exec(`DELETE FROM orgs WHERE id = $1`, id) }) + return id + } + mkConnector := func(orgID int64) int64 { + var id int64 + if err := db.QueryRow(` + INSERT INTO integration_tokens (provider, provider_app_id, access_token, connection_name, provider_url, org_id) + VALUES ('github', 'sibling-test', 'na', 'c', 'https://github.test', $1) RETURNING id`, orgID).Scan(&id); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _, _ = db.Exec(`DELETE FROM integration_tokens WHERE id = $1`, id) }) + return id + } + mkPR := func(orgID, connectorID int64) int64 { + repoID, err := store.UpsertRepository(RepositoryUpsert{ + OrgID: orgID, ConnectorID: connectorID, Provider: "github", ProviderRepoID: "repo-1", + FullName: "acme/api", Name: "api", WebURL: "https://github.test/acme/api", + }) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _, _ = db.Exec(`DELETE FROM repositories WHERE id = $1`, repoID) }) + prID, err := store.UpsertPullRequest(PullRequestUpsert{ + RepositoryID: repoID, OrgID: orgID, Provider: "github", ProviderPRID: "gh-pr-777", Number: 7, + Title: "t", State: "open", WebURL: "https://github.test/acme/api/pull/7", LastSyncedSource: "webhook", + ProviderUpdatedAt: time.Now(), + }) + if err != nil { + t.Fatal(err) + } + return prID + } + orgA, orgB := mkOrg("sibling-a-"+time.Now().Format("150405.000000")), mkOrg("sibling-b-"+time.Now().Format("150405.000000")) + prA, prB := mkPR(orgA, mkConnector(orgA)), mkPR(orgB, mkConnector(orgB)) + + var userID int64 + email := "sibling-" + time.Now().Format("150405.000000") + "@example.test" + if err := db.QueryRow(`INSERT INTO users (email, password_hash) VALUES ($1, 'x') RETURNING id`, email).Scan(&userID); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + _, _ = db.Exec(`DELETE FROM user_roles WHERE user_id = $1`, userID) + _, _ = db.Exec(`DELETE FROM users WHERE id = $1`, userID) + }) + + if _, _, err := store.SiblingPullRequestForUser(prA, userID); err != sql.ErrNoRows { + t.Fatalf("a user in neither org: err = %v, want sql.ErrNoRows", err) + } + if _, err := db.Exec(`INSERT INTO user_roles (user_id, org_id, role_id) VALUES ($1, $2, (SELECT id FROM roles WHERE name = 'member'))`, userID, orgB); err != nil { + t.Fatal(err) + } + gotOrg, gotPR, err := store.SiblingPullRequestForUser(prA, userID) + if err != nil || gotOrg != orgB || gotPR != prB { + t.Fatalf("got (%d, %d, %v), want org %d's copy %d", gotOrg, gotPR, err, orgB, prB) + } +} + +func TestDeleteRepositoriesByConnectorExcept(t *testing.T) { + db := testDB(t) + store := NewStore(db) + connectorID := insertTestConnector(t, db, "github") + keep := insertTestRepository(t, db, connectorID, "keep-1") + drop := insertTestRepository(t, db, connectorID, "drop-1") + + if err := store.DeleteRepositoriesByConnectorExcept(connectorID, []string{"keep-1"}); err != nil { + t.Fatal(err) + } + var n int + _ = db.QueryRow(`SELECT COUNT(*) FROM repositories WHERE id = $1`, keep).Scan(&n) + if n != 1 { + t.Error("the allowed repository was deleted") + } + _ = db.QueryRow(`SELECT COUNT(*) FROM repositories WHERE id = $1`, drop).Scan(&n) + if n != 0 { + t.Error("the repository that is no longer allowed was kept") + } +} diff --git a/storage/storage_status.md b/storage/storage_status.md index 4f7ad8df..2800d383 100644 --- a/storage/storage_status.md +++ b/storage/storage_status.md @@ -41,8 +41,8 @@ Latest milestone batch note (MF-LOC-007, MF-LOC-008, MF-PRORATION-003, MF-ATTRIB | providersbitbucket.NewIntegrationTokenStore | moved | [NewIntegrationTokenStore](providers/bitbucket/integration_token_store.go#L18) | | providersbitbucket.GetTokenByRepoFullName | moved | [GetTokenByRepoFullName](providers/bitbucket/integration_token_store.go#L22) | | providersbitbucket.GetLatestBitbucketToken | moved | [GetLatestBitbucketToken](providers/bitbucket/integration_token_store.go#L41) | -| providersgithub.NewTokenStore | moved | [NewTokenStore](providers/github/token_store.go#L32) | -| providersgithub.GetGitHubTokenByConnectorID | added | [GetGitHubTokenByConnectorID](providers/github/token_store.go#L38) | +| providersgithub.NewTokenStore | moved | [NewTokenStore](providers/github/token_store.go#L46) | +| providersgithub.GetGitHubTokenByConnectorID | added | [GetGitHubTokenByConnectorID](providers/github/token_store.go#L52) | | reviews.NewReviewStore | moved | [NewReviewStore](reviews/review_store.go#L9) | | reviews.QueryRow | moved | [QueryRow](reviews/review_store.go#L13) | | reviews.Exec | moved | [Exec](reviews/review_store.go#L17) | @@ -179,12 +179,12 @@ Latest milestone batch note (MF-LOC-007, MF-LOC-008, MF-PRORATION-003, MF-ATTRIB | seed_demo.CloneReview | added | [CloneReview](seed_demo/store.go#L158) | | seed_demo.DeleteReview | added | [DeleteReview](seed_demo/store.go#L220) | | seed_demo.CurrentPlan | added | [CurrentPlan](seed_demo/store.go#L243) | -| providersgithub.UpsertAppInstallationConnector | added | [UpsertAppInstallationConnector](providers/github/token_store.go#L85) | -| providersgithub.GetAppConnectorByInstallation | added | [GetAppConnectorByInstallation](providers/github/token_store.go#L151) | -| providersgithub.UpdateCachedInstallationToken | added | [UpdateCachedInstallationToken](providers/github/token_store.go#L159) | -| providersgithub.ListAppConnectorsExpiringBefore | added | [ListAppConnectorsExpiringBefore](providers/github/token_store.go#L169) | -| providersgithub.ListAppConnectorsForOrg | added | [ListAppConnectorsForOrg](providers/github/token_store.go#L180) | -| providersgithub.SetAppConnectorFlags | added | [SetAppConnectorFlags](providers/github/token_store.go#L207) | +| providersgithub.UpsertAppInstallationConnector | added | [UpsertAppInstallationConnector](providers/github/token_store.go#L105) | +| providersgithub.GetAppConnectorByInstallation | added | [GetAppConnectorByInstallation](providers/github/token_store.go#L204) | +| providersgithub.UpdateCachedInstallationToken | added | [UpdateCachedInstallationToken](providers/github/token_store.go#L246) | +| providersgithub.ListAppConnectorsExpiringBefore | added | [ListAppConnectorsExpiringBefore](providers/github/token_store.go#L256) | +| providersgithub.ListAppConnectorsForOrg | added | [ListAppConnectorsForOrg](providers/github/token_store.go#L267) | +| providersgithub.SetAppConnectorFlags | added | [SetAppConnectorFlags](providers/github/token_store.go#L294) | | users.FindUserByEmail | added | [FindUserByEmail](users/cloud_user_store.go#L12) | | users.InsertCloudUser | added | [InsertCloudUser](users/cloud_user_store.go#L19) | | users.FindOrgIDByName | added | [FindOrgIDByName](users/cloud_user_store.go#L29) | @@ -210,9 +210,15 @@ Latest milestone batch note (MF-LOC-007, MF-LOC-008, MF-PRORATION-003, MF-ATTRIB | aiconnectors.EnsureManagedConnector | added | [EnsureManagedConnector](aiconnectors/managed_ai_store.go#L156) | | aiconnectors.GrantFreeManagedAI | added | [GrantFreeManagedAI](aiconnectors/managed_ai_store.go#L186) | | aiconnectors.HasFreeManagedAI | added | [HasFreeManagedAI](aiconnectors/managed_ai_store.go#L209) | -| providersgithub.FindPreferredConnectorForRepo | added | [FindPreferredConnectorForRepo](providers/github/token_store.go#L235) | -| providersgithub.HasAppConnectorForRepo | added | [HasAppConnectorForRepo](providers/github/token_store.go#L250) | -| providersgithub.HasAppConnectorForOwnerAllRepos | added | [HasAppConnectorForOwnerAllRepos](providers/github/token_store.go#L267) | -| pullrequests.DeleteRepositoryByProviderID | added | [DeleteRepositoryByProviderID](providers/pullrequests/upsert.go#L196) | -| pullrequests.ListRepositoryFullNamesByConnector | added | [ListRepositoryFullNamesByConnector](providers/pullrequests/upsert.go#L202) | -| pullrequests.DeleteRepositoriesByConnector | added | [DeleteRepositoriesByConnector](providers/pullrequests/upsert.go#L221) | +| providersgithub.FindPreferredConnectorForRepo | added | [FindPreferredConnectorForRepo](providers/github/token_store.go#L334) | +| providersgithub.HasAppConnectorForRepo | added | [HasAppConnectorForRepo](providers/github/token_store.go#L349) | +| providersgithub.HasAppConnectorForOwnerAllRepos | added | [HasAppConnectorForOwnerAllRepos](providers/github/token_store.go#L367) | +| providersgithub.ListAppConnectorsByInstallation | added | [ListAppConnectorsByInstallation](providers/github/token_store.go#L213) | +| providersgithub.CountOtherOrgsUsingInstallation | added | [CountOtherOrgsUsingInstallation](providers/github/token_store.go#L222) | +| providersgithub.LatestReviewConnector | added | [LatestReviewConnector](providers/github/token_store.go#L234) | +| providersgithub.DisconnectAppConnector | added | [DisconnectAppConnector](providers/github/token_store.go#L322) | +| pullrequests.DeleteRepositoryByProviderID | added | [DeleteRepositoryByProviderID](providers/pullrequests/upsert.go#L198) | +| pullrequests.ListRepositoryFullNamesByConnector | added | [ListRepositoryFullNamesByConnector](providers/pullrequests/upsert.go#L234) | +| pullrequests.DeleteRepositoriesByConnector | added | [DeleteRepositoriesByConnector](providers/pullrequests/upsert.go#L253) | +| pullrequests.DeleteRepositoriesByConnectorExcept | added | [DeleteRepositoriesByConnectorExcept](providers/pullrequests/upsert.go#L205) | +| pullrequests.SiblingPullRequestForUser | added | [SiblingPullRequestForUser](providers/pullrequests/upsert.go#L216) | diff --git a/ui/src/api/githubApp.ts b/ui/src/api/githubApp.ts index 01883e29..75e3e37d 100644 --- a/ui/src/api/githubApp.ts +++ b/ui/src/api/githubApp.ts @@ -6,6 +6,10 @@ export interface GitHubAppInstallation { account_type: string; repository_selection: string; settings_url: string; + /** "user": the App was installed by someone else, so this org gets only the repos its GitHub user can access. */ + repo_scope: 'installer' | 'user'; + /** How many other LiveReview accounts use this installation. */ + other_orgs: number; } export interface GitHubAppRepository { @@ -38,11 +42,12 @@ export const getGitHubAppDemo = (light = false): Promise => apiClient.get(`/api/v1/github-app/demo${light ? '?light=1' : ''}`); /** - * Stops LiveReview using one GitHub App installation for the current org (org owners only). The App stays - * installed on GitHub; signing in with GitHub again reconnects it. + * Stops LiveReview using one GitHub App installation for the current org (org owners only). By default only this + * org is disconnected: the App stays installed on GitHub and other LiveReview accounts using it keep working. + * `uninstall` also uninstalls the App from GitHub, which removes it for every account using it. */ -export const disconnectGitHubInstallation = (connectorId: number): Promise<{ status: string }> => - apiClient.delete<{ status: string }>(`/api/v1/github-app/installations/${connectorId}`); +export const disconnectGitHubInstallation = (connectorId: number, uninstall = false): Promise<{ status: string }> => + apiClient.delete<{ status: string }>(`/api/v1/github-app/installations/${connectorId}${uninstall ? '?uninstall=true' : ''}`); export interface PublicUIConfig { isCloud: boolean; diff --git a/ui/src/api/pullRequests.ts b/ui/src/api/pullRequests.ts index 023f1170..ccda282f 100644 --- a/ui/src/api/pullRequests.ts +++ b/ui/src/api/pullRequests.ts @@ -60,10 +60,12 @@ export const triggerReviewForPullRequest = async (pullRequestId: number): Promis /** * Resolve which org a PR belongs to (404 if missing or the user isn't a member). - * Used by the PR comment's "Start a review" link before the org is known. + * Used by the PR comment's "Start a review" link before the org is known. pull_request_id + * is the id to use in that org (another org's copy of the same PR when orgs share a + * GitHub App installation). */ -export const getPullRequestOrg = async (pullRequestId: number): Promise<{ org_id: number }> => { - return apiClient.get<{ org_id: number }>(`/api/v1/pull-requests/${pullRequestId}/org`); +export const getPullRequestOrg = async (pullRequestId: number): Promise<{ org_id: number; pull_request_id: number }> => { + return apiClient.get<{ org_id: number; pull_request_id: number }>(`/api/v1/pull-requests/${pullRequestId}/org`); }; /** diff --git a/ui/src/components/GitHubApp/GitHubAppAccounts.tsx b/ui/src/components/GitHubApp/GitHubAppAccounts.tsx index 12b2395c..1032ddb5 100644 --- a/ui/src/components/GitHubApp/GitHubAppAccounts.tsx +++ b/ui/src/components/GitHubApp/GitHubAppAccounts.tsx @@ -8,16 +8,26 @@ import { disconnectGitHubInstallation, GitHubAppInstallation, GitHubAppRepositor interface GitHubAppAccountsProps { installations: GitHubAppInstallation[]; repos: GitHubAppRepository[]; - // Only organization owners can uninstall; the server enforces it too. + // Only organization owners can disconnect or uninstall; the server enforces it too. canDisconnect: boolean; onInstallAnother: () => void; installBusy?: boolean; - // Called after an account was uninstalled; the parent refreshes its data. + // Called after an account was disconnected or uninstalled; the parent refreshes its data. onDisconnected: () => void | Promise; } +// What the confirm dialog is for: disconnect only this LiveReview account, or uninstall the App from GitHub. +interface PendingAction { + inst: GitHubAppInstallation; + uninstall: boolean; +} + +const accountsLabel = (n: number) => `${n} other LiveReview ${n === 1 ? 'account' : 'accounts'}`; + // The org's connected GitHub accounts: each one is a heading with its repositories indented below it, a -// "Manage on GitHub" button and (for owners) "Disconnect"; plus "Add repositories or organization". +// "Manage on GitHub" button and (for owners) "Disconnect" and "Uninstall"; plus "Add repositories or organization". +// Several LiveReview accounts can share one installation, so Disconnect only affects this account while Uninstall +// removes the App from GitHub for all of them. // Shared by the Git Providers page and the get-started page. const GitHubAppAccounts: React.FC = ({ installations, @@ -27,19 +37,22 @@ const GitHubAppAccounts: React.FC = ({ installBusy = false, onDisconnected, }) => { - const [disconnecting, setDisconnecting] = useState(null); + const [pending, setPending] = useState(null); const [busy, setBusy] = useState(false); const reposByOwner = (owner: string) => repos.filter((r) => r.full_name.split('/')[0].toLowerCase() === owner.toLowerCase()); - const confirmDisconnect = async () => { - if (!disconnecting) return; + const confirmAction = async () => { + if (!pending) return; + const { inst, uninstall } = pending; setBusy(true); try { - await disconnectGitHubInstallation(disconnecting.connector_id); - notify.success(`Uninstalled the app from ${disconnecting.account_login} successfully`); - setDisconnecting(null); + await disconnectGitHubInstallation(inst.connector_id, uninstall); + notify.success(uninstall + ? `Uninstalled the app from ${inst.account_login} successfully` + : `Disconnected ${inst.account_login} successfully`); + setPending(null); await onDisconnected(); } catch (err: any) { notify.error(err?.message || 'Failed to disconnect GitHub installation'); @@ -65,7 +78,12 @@ const GitHubAppAccounts: React.FC = ({ {orgRepos.length} {orgRepos.length === 1 ? 'repository' : 'repositories'} -

Connected through the GitHub App

+

+ {inst.repo_scope === 'user' + ? 'The GitHub App was already installed here. Showing the repositories your GitHub account can access.' + : 'Connected through the GitHub App'} + {inst.other_orgs > 0 && ` Also used by ${accountsLabel(inst.other_orgs)}.`} +

@@ -80,14 +98,24 @@ const GitHubAppAccounts: React.FC = ({ )} {canDisconnect && ( - + <> + + + )}
@@ -107,17 +135,35 @@ const GitHubAppAccounts: React.FC = ({ Add repositories or organization setDisconnecting(null)} + title={pending?.uninstall + ? `Uninstall from ${pending.inst.account_login}?` + : `Disconnect ${pending?.inst.account_login ?? 'this account'}?`} + message={confirmMessage(pending)} + confirmText={pending?.uninstall ? 'Uninstall and disconnect' : 'Disconnect'} + onConfirm={confirmAction} + onCancel={() => setPending(null)} /> ); }; +function confirmMessage(pending: PendingAction | null): string { + if (!pending) return ''; + const { inst, uninstall } = pending; + if (!uninstall) { + return `LiveReview stops reviewing ${inst.account_login}'s pull requests for your account and removes its repositories here. ` + + 'The app stays installed on GitHub' + + (inst.other_orgs > 0 ? `, so the ${accountsLabel(inst.other_orgs)} using it keep working` : '') + + '. To connect it again, use "Add repositories or organization".'; + } + return `This uninstalls the LiveReview GitHub App from ${inst.account_login} on GitHub and stops LiveReview reviewing its pull requests. ` + + (inst.other_orgs > 0 + ? `${accountsLabel(inst.other_orgs)} ${inst.other_orgs === 1 ? 'also uses' : 'also use'} LiveReview on ${inst.account_login}; uninstalling removes it for all of them. ` + : '') + + 'To use it again, install the app on GitHub again.'; +} + export default GitHubAppAccounts; diff --git a/ui/src/hooks/useSwitchToPullRequestOrg.ts b/ui/src/hooks/useSwitchToPullRequestOrg.ts index 874bf6c0..7083c422 100644 --- a/ui/src/hooks/useSwitchToPullRequestOrg.ts +++ b/ui/src/hooks/useSwitchToPullRequestOrg.ts @@ -6,17 +6,21 @@ export const PR_NOT_FOUND_MESSAGE = "This pull request wasn't found, or you don't have access to its organization."; // PR comment links don't carry an org, so look up the PR's org and switch to it. +// Resolves to the pull request id to use there: orgs sharing a GitHub App installation each +// have their own copy of the PR, and the link may name another org's copy. // Throws the API error (status 404 when missing or not a member). export const useSwitchToPullRequestOrg = () => { const dispatch = useAppDispatch(); const { currentOrgId, userOrganizations } = useAppSelector((state) => state.Organizations); - return async (pullRequestId: number) => { - const { org_id: orgId } = await getPullRequestOrg(pullRequestId); - if (currentOrgId === orgId) return; - if (!userOrganizations.some((o) => o.id === orgId)) { - await dispatch(loadUserOrganizations()).unwrap(); + return async (pullRequestId: number): Promise => { + const { org_id: orgId, pull_request_id: resolvedId } = await getPullRequestOrg(pullRequestId); + if (currentOrgId !== orgId) { + if (!userOrganizations.some((o) => o.id === orgId)) { + await dispatch(loadUserOrganizations()).unwrap(); + } + await dispatch(switchOrganization({ orgId })).unwrap(); } - await dispatch(switchOrganization({ orgId })).unwrap(); + return resolvedId; }; }; diff --git a/ui/src/index.tsx b/ui/src/index.tsx index cf2d268b..4297b0a7 100644 --- a/ui/src/index.tsx +++ b/ui/src/index.tsx @@ -25,17 +25,22 @@ if (typeof window !== 'undefined' && /^\/gh-login\/?$/.test(window.location.path window.location.replace('/#/gh-login' + window.location.search); } -// A returning GitHub sign-in is sent to the GitHub App card on Git Providers (where their connected accounts are managed) rather than -// the first-run get-started page: the callback says so with a short-lived cookie, which overrides the destination -// that /gh-login stored before leaving for GitHub. +// Where a successful GitHub sign-in from /gh-login lands, from a short-lived cookie the callback sets: get-started +// for new users, the GitHub App card on Git Providers (where connected accounts are managed) for returning ones. +const GH_NEXT_PAGES: Record = { + 'get-started': '#/get-started/github', + git: '#/git/github-app', +}; if (typeof document !== 'undefined') { try { - if (/(?:^|; )lr_gh_next=git(?:;|$)/.test(document.cookie)) { - sessionStorage.setItem('redirectAfterLogin', '#/git/github-app'); + const match = document.cookie.match(/(?:^|; )lr_gh_next=([a-z-]+)/); + if (match) { + const page = GH_NEXT_PAGES[match[1]]; + if (page) sessionStorage.setItem('redirectAfterLogin', page); document.cookie = 'lr_gh_next=; Max-Age=0; Path=/'; } } catch { - // Cookies or sessionStorage unavailable: the user lands on the page /gh-login chose. + // Cookies or sessionStorage unavailable: the user lands on the dashboard. } } diff --git a/ui/src/pages/Auth/GhLogin.tsx b/ui/src/pages/Auth/GhLogin.tsx index e582a913..5b254d30 100644 --- a/ui/src/pages/Auth/GhLogin.tsx +++ b/ui/src/pages/Auth/GhLogin.tsx @@ -14,10 +14,6 @@ import { isCloudMode } from '../../utils/deploymentMode'; const SRC_PATTERN = /^[a-z0-9_-]{1,32}$/; -// Same destination the server redirects to after login; kept in sessionStorage because the GitHub round trip -// is a full-page navigation. The helper in utils/authHelpers reads it back after login. -export const GET_STARTED_PATH = '#/get-started/github'; - const POINTS = [ 'Risk scoring decides which PRs are safe to ship without a human', 'Your team only spends review time on risky and critical changes', @@ -59,12 +55,9 @@ const GhLogin: React.FC = () => { // Always starts with GitHub sign-in: returning users are recognised from their installations, and the server // sends users with no installation on to the install page (see GitHubAppCallback), so no separate // "already installed" path is needed here. + // Nothing is stored before leaving: the callback says where to land (get-started) only when sign-in succeeds, + // so a failed attempt followed by another sign-in method doesn't end up on get-started. const start = () => { - try { - sessionStorage.setItem('redirectAfterLogin', GET_STARTED_PATH); - } catch { - // sessionStorage can be unavailable (private mode); the user still lands on the dashboard after login. - } window.location.href = githubAppStartUrl('signin', src); }; diff --git a/ui/src/pages/Reviews/PullRequestReviewTrigger.tsx b/ui/src/pages/Reviews/PullRequestReviewTrigger.tsx index afd89bbd..d436aa90 100644 --- a/ui/src/pages/Reviews/PullRequestReviewTrigger.tsx +++ b/ui/src/pages/Reviews/PullRequestReviewTrigger.tsx @@ -21,8 +21,8 @@ const PullRequestReviewTrigger: React.FC = () => { return; } try { - await switchToPullRequestOrg(pullRequestId); - const res = await startOrReuseReviewForPullRequest(pullRequestId); + const orgPullRequestId = await switchToPullRequestOrg(pullRequestId); + const res = await startOrReuseReviewForPullRequest(orgPullRequestId); navigate(`/reviews/${res.reviewId}`, { replace: true }); } catch (err: any) { setError(err?.status === 404 ? PR_NOT_FOUND_MESSAGE : err?.message || 'Failed to start the review.'); diff --git a/ui/src/pages/Reviews/Reviews.tsx b/ui/src/pages/Reviews/Reviews.tsx index 2762794c..bcc7e727 100644 --- a/ui/src/pages/Reviews/Reviews.tsx +++ b/ui/src/pages/Reviews/Reviews.tsx @@ -175,7 +175,11 @@ const Reviews: React.FC = () => { (async () => { if (pullRequestId) { try { - await switchToPullRequestOrg(pullRequestId); + const orgPullRequestId = await switchToPullRequestOrg(pullRequestId); + if (orgPullRequestId !== pullRequestId) { + pullRequestIdRef.current = orgPullRequestId; + setPullRequestId(orgPullRequestId); + } } catch (err: any) { setLoading(false); setError(err?.status === 404 From 80be6131aa89d446bfb8d2b3f30f4ac03aa45ac7 Mon Sep 17 00:00:00 2001 From: lovestaco Date: Fri, 23 Oct 2026 20:19:50 +0530 Subject: [PATCH 02/10] List the other LiveReview accounts sharing a GitHub installation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "Also used by N other LiveReview accounts" on the GitHub App card and get-started is now a link: it opens a popup listing those accounts' owners by masked email (first two letters and the domain, e.g. at•••@gmail.com). The server masks them; full addresses are never sent. --- internal/api/github_app_auth.go | 32 +++++++++- internal/api/github_app_auth_test.go | 64 +++++++++++++++++++ .../docs/routes_guide/get-started-github.md | 4 +- .../docs/routes_guide/git/git-providers.md | 4 +- storage/providers/github/token_store.go | 28 ++++++++ storage/storage_status.md | 19 +++--- ui/src/api/githubApp.ts | 2 + .../GitHubApp/GitHubAppAccounts.tsx | 54 +++++++++++++++- 8 files changed, 192 insertions(+), 15 deletions(-) diff --git a/internal/api/github_app_auth.go b/internal/api/github_app_auth.go index a2f03c6c..ce69b30a 100644 --- a/internal/api/github_app_auth.go +++ b/internal/api/github_app_auth.go @@ -866,8 +866,11 @@ type githubAppDemoInstallation struct { // RepoScope is "user" when this org only gets the repositories its GitHub user can access (the App was // installed by someone else), otherwise "installer". RepoScope string `json:"repo_scope"` - // OtherOrgs counts the other LiveReview orgs using this installation (a number only, nothing about them). + // OtherOrgs counts the other LiveReview orgs using this installation. OtherOrgs int `json:"other_orgs"` + // OtherAccounts lists those orgs' owners, masked ("at•••@gmail.com"): enough to recognise a teammate, not + // to contact them. Full addresses never leave the server. + OtherAccounts []string `json:"other_accounts"` } type githubAppDemoRepo struct { @@ -945,10 +948,18 @@ func (s *Server) GitHubAppDemo(c echo.Context) error { scope = storagegithub.RepoScopeUser } others, err := store.CountOtherOrgsUsingInstallation(cfg.BaseURL, meta.InstallationID, orgID) + var ownerEmails []string + if err == nil { + ownerEmails, err = store.OtherOrgOwnerEmailsForInstallation(cfg.BaseURL, meta.InstallationID, orgID) + } if err != nil { - log.Printf("[GitHubApp] demo: counting other orgs for connector %d failed: %v", rec.ID, err) + log.Printf("[GitHubApp] demo: looking up other orgs for connector %d failed: %v", rec.ID, err) return c.JSON(http.StatusInternalServerError, map[string]string{"error": "failed to load GitHub connections"}) } + otherAccounts := make([]string, 0, len(ownerEmails)) + for _, e := range ownerEmails { + otherAccounts = append(otherAccounts, maskEmail(e)) + } var token string if !light { @@ -962,7 +973,7 @@ func (s *Server) GitHubAppDemo(c echo.Context) error { resp.Installations = append(resp.Installations, githubAppDemoInstallation{ ConnectorID: rec.ID, AccountLogin: meta.AccountLogin, AccountType: meta.AccountType, RepositorySelection: meta.RepositorySelection, SettingsURL: cfg.InstallationSettingsURL(meta.AccountType, meta.AccountLogin, meta.InstallationID), - RepoScope: scope, OtherOrgs: others, + RepoScope: scope, OtherOrgs: others, OtherAccounts: otherAccounts, }) if light { continue @@ -1009,6 +1020,21 @@ func (s *Server) GitHubAppDemo(c echo.Context) error { return c.JSON(http.StatusOK, resp) } +// maskEmail keeps the first two characters of the name and the domain: "athreyac4@gmail.com" -> "at•••@gmail.com" +// (one character when the name is that short). +func maskEmail(email string) string { + at := strings.LastIndex(email, "@") + if at < 1 { + return "•••" + } + name := []rune(email[:at]) + keep := 2 + if len(name) <= 2 { + keep = 1 + } + return string(name[:keep]) + "•••" + email[at:] +} + // --------------------------------------------------------------------------------------------------------- // error page diff --git a/internal/api/github_app_auth_test.go b/internal/api/github_app_auth_test.go index 54233ac8..8a79a8bb 100644 --- a/internal/api/github_app_auth_test.go +++ b/internal/api/github_app_auth_test.go @@ -675,6 +675,19 @@ func TestAppConnectorCovers_RespectsScopeAndState(t *testing.T) { } } +func TestMaskEmail(t *testing.T) { + for in, want := range map[string]string{ + "athreyac4@gmail.com": "at•••@gmail.com", + "ab@hexmos.com": "a•••@hexmos.com", + "x@y.io": "x•••@y.io", + "no-at-sign": "•••", + } { + if got := maskEmail(in); got != want { + t.Errorf("maskEmail(%q) = %q, want %q", in, got, want) + } + } +} + func TestUserScopedRepoIDs(t *testing.T) { user := &ConnectorDetails{TokenType: storagegithub.TokenTypeGitHubApp, Metadata: map[string]interface{}{ "repo_scope": "user", "allowed_repo_ids": []interface{}{float64(101), float64(202)}, @@ -774,6 +787,57 @@ func TestGitHubAppDemo_ScopedToOrg(t *testing.T) { } } +func TestGitHubAppDemo_ListsOtherAccountsMasked(t *testing.T) { + f := newFakeGitHub(t) + s, db := newGitHubAppTestServer(t, f) + store := storagegithub.NewTokenStore(db) + inst := time.Now().UnixNano() % 1_000_000_000_000 + mkOrg := func(name string) int64 { + cleanupGitHubAppTenant(t, db, name) + var id int64 + if err := db.QueryRow(`INSERT INTO orgs (name, description) VALUES ($1, 'demo shared test') RETURNING id`, name).Scan(&id); err != nil { + t.Fatal(err) + } + if _, _, err := store.UpsertAppInstallationConnector(storagegithub.AppConnectorUpsert{ + OrgID: id, BaseURL: f.srv.URL, InstallationID: inst, AccountLogin: "acme", AccountType: "Organization", + Token: "ghs_live", ExpiresAt: time.Now().Add(time.Hour), + }); err != nil { + t.Fatal(err) + } + return id + } + suffix := time.Now().UnixNano() + mine := mkOrg(fmt.Sprintf("gh-demo-mine-%d@example.test", suffix)) + theirsEmail := fmt.Sprintf("teammate-%d@example.test", suffix) + theirs := mkOrg(theirsEmail) + var userID int64 + if err := db.QueryRow(`INSERT INTO users (email, password_hash) VALUES ($1, 'x') RETURNING id`, theirsEmail).Scan(&userID); err != nil { + t.Fatal(err) + } + if _, err := db.Exec(`INSERT INTO user_roles (user_id, org_id, role_id) VALUES ($1, $2, (SELECT id FROM roles WHERE name = 'owner'))`, userID, theirs); err != nil { + t.Fatal(err) + } + + req := httptest.NewRequest(http.MethodGet, "/api/v1/github-app/demo?light=1", nil) + rec := httptest.NewRecorder() + c := echo.New().NewContext(req, rec) + c.Set("org_id", mine) + if err := s.GitHubAppDemo(c); err != nil || rec.Code != http.StatusOK { + t.Fatalf("demo: %v %d", err, rec.Code) + } + if strings.Contains(rec.Body.String(), theirsEmail) { + t.Fatal("the other account's full email was sent") + } + var out githubAppDemoResponse + if err := json.Unmarshal(rec.Body.Bytes(), &out); err != nil { + t.Fatal(err) + } + if len(out.Installations) != 1 || out.Installations[0].OtherOrgs != 1 || + len(out.Installations[0].OtherAccounts) != 1 || out.Installations[0].OtherAccounts[0] != "te•••@example.test" { + t.Fatalf("installation = %+v, want 1 other account te•••@example.test", out.Installations) + } +} + func TestGitHubAppDisconnect_OwnersOnlyAndOrgScoped(t *testing.T) { f := newFakeGitHub(t) s, db := newGitHubAppTestServer(t, f) diff --git a/internal/docindex/docs/routes_guide/get-started-github.md b/internal/docindex/docs/routes_guide/get-started-github.md index fba7b156..4302d2b4 100644 --- a/internal/docindex/docs/routes_guide/get-started-github.md +++ b/internal/docindex/docs/routes_guide/get-started-github.md @@ -38,7 +38,9 @@ organization has a GitHub App connection. - When the app was already installed on the account by someone else, the account says so and shows only the repositories the user's GitHub account can access. When other LiveReview accounts use the same installation, it says how - many ("Also used by 1 other LiveReview account"). + many ("Also used by 1 other LiveReview account"); clicking that opens a popup + listing those accounts' owners by masked email (first two letters and the + domain, e.g. `at•••@gmail.com`). - **Disconnect** (organization owners only) — next to each account: stops LiveReview using it for this organization only and drops its repositories here. The app stays installed on GitHub, so other LiveReview accounts using it diff --git a/internal/docindex/docs/routes_guide/git/git-providers.md b/internal/docindex/docs/routes_guide/git/git-providers.md index 0f98222d..fea36980 100644 --- a/internal/docindex/docs/routes_guide/git/git-providers.md +++ b/internal/docindex/docs/routes_guide/git/git-providers.md @@ -43,7 +43,9 @@ added or removed on GitHub, not here. Several LiveReview organizations can use the same installation (for example two people from one GitHub organization who each signed up). Each has its own connection. An organization that joined an installation someone else installed -only gets the repositories its user's GitHub account can access. Pull request +only gets the repositories its user's GitHub account can access. Each account +on the card shows "Also used by N other LiveReview accounts" when others share +it; clicking that lists them by their owners' masked emails (`at•••@gmail.com`). Pull request updates reach every organization that has the repository; a reply to a comment on GitHub comes from the organization that reviewed that pull request most recently. diff --git a/storage/providers/github/token_store.go b/storage/providers/github/token_store.go index 7edab92c..69e64f2d 100644 --- a/storage/providers/github/token_store.go +++ b/storage/providers/github/token_store.go @@ -229,6 +229,34 @@ func (s *TokenStore) CountOtherOrgsUsingInstallation(baseURL string, installatio return n, err } +// OtherOrgOwnerEmailsForInstallation returns the emails of the active owners of the other orgs with a live connector +// for the installation, sorted and without duplicates. Callers show them masked only. +func (s *TokenStore) OtherOrgOwnerEmailsForInstallation(baseURL string, installationID, orgID int64) ([]string, error) { + rows, err := s.db.Query(` + SELECT DISTINCT LOWER(u.email) + FROM integration_tokens it + JOIN user_roles ur ON ur.org_id = it.org_id + JOIN roles r ON r.id = ur.role_id AND r.name = 'owner' + JOIN users u ON u.id = ur.user_id AND u.is_active + WHERE it.token_type = $1 AND it.provider_url = $2 AND it.provider_app_id = $3 AND it.org_id <> $4 + AND COALESCE(it.metadata->>'removed', 'false') <> 'true' + ORDER BY 1 + `, TokenTypeGitHubApp, baseURL, fmt.Sprintf("%d", installationID), orgID) + if err != nil { + return nil, err + } + defer rows.Close() + emails := []string{} + for rows.Next() { + var e string + if err := rows.Scan(&e); err != nil { + return nil, err + } + emails = append(emails, e) + } + return emails, rows.Err() +} + // LatestReviewConnector returns which of the connectors ran the most recent review of the PR/MR at prURL; // sql.ErrNoRows when none of them reviewed it. func (s *TokenStore) LatestReviewConnector(connectorIDs []int64, prURL string) (int64, error) { diff --git a/storage/storage_status.md b/storage/storage_status.md index 2800d383..b7bce099 100644 --- a/storage/storage_status.md +++ b/storage/storage_status.md @@ -181,10 +181,10 @@ Latest milestone batch note (MF-LOC-007, MF-LOC-008, MF-PRORATION-003, MF-ATTRIB | seed_demo.CurrentPlan | added | [CurrentPlan](seed_demo/store.go#L243) | | providersgithub.UpsertAppInstallationConnector | added | [UpsertAppInstallationConnector](providers/github/token_store.go#L105) | | providersgithub.GetAppConnectorByInstallation | added | [GetAppConnectorByInstallation](providers/github/token_store.go#L204) | -| providersgithub.UpdateCachedInstallationToken | added | [UpdateCachedInstallationToken](providers/github/token_store.go#L246) | -| providersgithub.ListAppConnectorsExpiringBefore | added | [ListAppConnectorsExpiringBefore](providers/github/token_store.go#L256) | -| providersgithub.ListAppConnectorsForOrg | added | [ListAppConnectorsForOrg](providers/github/token_store.go#L267) | -| providersgithub.SetAppConnectorFlags | added | [SetAppConnectorFlags](providers/github/token_store.go#L294) | +| providersgithub.UpdateCachedInstallationToken | added | [UpdateCachedInstallationToken](providers/github/token_store.go#L274) | +| providersgithub.ListAppConnectorsExpiringBefore | added | [ListAppConnectorsExpiringBefore](providers/github/token_store.go#L284) | +| providersgithub.ListAppConnectorsForOrg | added | [ListAppConnectorsForOrg](providers/github/token_store.go#L295) | +| providersgithub.SetAppConnectorFlags | added | [SetAppConnectorFlags](providers/github/token_store.go#L322) | | users.FindUserByEmail | added | [FindUserByEmail](users/cloud_user_store.go#L12) | | users.InsertCloudUser | added | [InsertCloudUser](users/cloud_user_store.go#L19) | | users.FindOrgIDByName | added | [FindOrgIDByName](users/cloud_user_store.go#L29) | @@ -210,13 +210,14 @@ Latest milestone batch note (MF-LOC-007, MF-LOC-008, MF-PRORATION-003, MF-ATTRIB | aiconnectors.EnsureManagedConnector | added | [EnsureManagedConnector](aiconnectors/managed_ai_store.go#L156) | | aiconnectors.GrantFreeManagedAI | added | [GrantFreeManagedAI](aiconnectors/managed_ai_store.go#L186) | | aiconnectors.HasFreeManagedAI | added | [HasFreeManagedAI](aiconnectors/managed_ai_store.go#L209) | -| providersgithub.FindPreferredConnectorForRepo | added | [FindPreferredConnectorForRepo](providers/github/token_store.go#L334) | -| providersgithub.HasAppConnectorForRepo | added | [HasAppConnectorForRepo](providers/github/token_store.go#L349) | -| providersgithub.HasAppConnectorForOwnerAllRepos | added | [HasAppConnectorForOwnerAllRepos](providers/github/token_store.go#L367) | +| providersgithub.FindPreferredConnectorForRepo | added | [FindPreferredConnectorForRepo](providers/github/token_store.go#L362) | +| providersgithub.HasAppConnectorForRepo | added | [HasAppConnectorForRepo](providers/github/token_store.go#L377) | +| providersgithub.HasAppConnectorForOwnerAllRepos | added | [HasAppConnectorForOwnerAllRepos](providers/github/token_store.go#L395) | | providersgithub.ListAppConnectorsByInstallation | added | [ListAppConnectorsByInstallation](providers/github/token_store.go#L213) | | providersgithub.CountOtherOrgsUsingInstallation | added | [CountOtherOrgsUsingInstallation](providers/github/token_store.go#L222) | -| providersgithub.LatestReviewConnector | added | [LatestReviewConnector](providers/github/token_store.go#L234) | -| providersgithub.DisconnectAppConnector | added | [DisconnectAppConnector](providers/github/token_store.go#L322) | +| providersgithub.OtherOrgOwnerEmailsForInstallation | added | [OtherOrgOwnerEmailsForInstallation](providers/github/token_store.go#L234) | +| providersgithub.LatestReviewConnector | added | [LatestReviewConnector](providers/github/token_store.go#L262) | +| providersgithub.DisconnectAppConnector | added | [DisconnectAppConnector](providers/github/token_store.go#L350) | | pullrequests.DeleteRepositoryByProviderID | added | [DeleteRepositoryByProviderID](providers/pullrequests/upsert.go#L198) | | pullrequests.ListRepositoryFullNamesByConnector | added | [ListRepositoryFullNamesByConnector](providers/pullrequests/upsert.go#L234) | | pullrequests.DeleteRepositoriesByConnector | added | [DeleteRepositoriesByConnector](providers/pullrequests/upsert.go#L253) | diff --git a/ui/src/api/githubApp.ts b/ui/src/api/githubApp.ts index 75e3e37d..a9a0088b 100644 --- a/ui/src/api/githubApp.ts +++ b/ui/src/api/githubApp.ts @@ -10,6 +10,8 @@ export interface GitHubAppInstallation { repo_scope: 'installer' | 'user'; /** How many other LiveReview accounts use this installation. */ other_orgs: number; + /** Their owners' emails, masked by the server (e.g. "at•••@gmail.com"). */ + other_accounts: string[]; } export interface GitHubAppRepository { diff --git a/ui/src/components/GitHubApp/GitHubAppAccounts.tsx b/ui/src/components/GitHubApp/GitHubAppAccounts.tsx index 1032ddb5..eae22fa0 100644 --- a/ui/src/components/GitHubApp/GitHubAppAccounts.tsx +++ b/ui/src/components/GitHubApp/GitHubAppAccounts.tsx @@ -1,4 +1,5 @@ import React, { useState } from 'react'; +import { createPortal } from 'react-dom'; import { Avatar, Button, Icons } from '../UIPrimitives'; import GitHubAppRepositories from './GitHubAppRepositories'; import { ConfirmModal } from '../ConfirmModal'; @@ -38,6 +39,7 @@ const GitHubAppAccounts: React.FC = ({ onDisconnected, }) => { const [pending, setPending] = useState(null); + const [showingOthers, setShowingOthers] = useState(null); const [busy, setBusy] = useState(false); const reposByOwner = (owner: string) => @@ -82,7 +84,19 @@ const GitHubAppAccounts: React.FC = ({ {inst.repo_scope === 'user' ? 'The GitHub App was already installed here. Showing the repositories your GitHub account can access.' : 'Connected through the GitHub App'} - {inst.other_orgs > 0 && ` Also used by ${accountsLabel(inst.other_orgs)}.`} + {inst.other_orgs > 0 && ( + <> + {' '}Also used by{' '} + + . + + )}

@@ -146,10 +160,48 @@ const GitHubAppAccounts: React.FC = ({ onConfirm={confirmAction} onCancel={() => setPending(null)} /> + {showingOthers && ( + setShowingOthers(null)} /> + )} ); }; +// Lists the other LiveReview accounts using an installation, by their owners' masked emails. +const OtherAccountsModal: React.FC<{ inst: GitHubAppInstallation; onClose: () => void }> = ({ inst, onClose }) => { + const unnamed = inst.other_orgs - inst.other_accounts.length; + return createPortal( +
+
+
+
+

Also using LiveReview on {inst.account_login}

+

+ Each of these LiveReview accounts has its own connection to this installation. +

+
    + {inst.other_accounts.map((email) => ( +
  • + + {email} +
    • + ))} + {unnamed > 0 && ( +
  • + {unnamed} more {unnamed === 1 ? 'account' : 'accounts'} without an active owner +
    • + )} +
+
+ +
+
+
+
, + document.body + ); +}; + function confirmMessage(pending: PendingAction | null): string { if (!pending) return ''; const { inst, uninstall } = pending; From 8e50853f052dd83009402427c660b8a3ca752779 Mon Sep 17 00:00:00 2001 From: lovestaco Date: Fri, 23 Oct 2026 21:13:02 +0530 Subject: [PATCH 03/10] Fix page title, description and link-preview tags Shared LiveReview links (e.g. /gh-login?src=launch-trial) previewed as "LiveReview - A powerful tool for automated code reviews" with the React boilerplate's og:url and a map-collage thumbnail. - Title and description match the launch messaging. - Open Graph tags use the property attribute (Facebook and LinkedIn ignore name="og:..."), plus og:type, og:site_name and image size/alt; Twitter card tags too. - New 1200x630 og-image.png replaces the boilerplate thumbnail.jpg and the SVG og:image social sites don't render. - Cloud builds: og:url and canonical https://livereview.hexmos.com, indexable. Self-hosted builds: noindex, no canonical. --- ui/meta.config.js | 76 +++++++++++++++++++++++----------------- ui/package.json | 4 +-- ui/public/index.html | 4 ++- ui/public/og-image.png | Bin 0 -> 76984 bytes ui/public/thumbnail.jpg | Bin 98935 -> 0 bytes ui/webpack.config.js | 6 +++- 6 files changed, 53 insertions(+), 37 deletions(-) create mode 100644 ui/public/og-image.png delete mode 100644 ui/public/thumbnail.jpg diff --git a/ui/meta.config.js b/ui/meta.config.js index 9e755f66..78788874 100644 --- a/ui/meta.config.js +++ b/ui/meta.config.js @@ -1,35 +1,45 @@ -const package = require('./package.json'); +// Page title, description and link-preview tags baked into index.html (html-webpack-plugin `meta`). Link previews +// (Slack, X, LinkedIn, WhatsApp) and search engines read only this static HTML, never the React app, so these are +// what people see when a LiveReview URL is shared, e.g. https://livereview.hexmos.com/gh-login?src=launch-trial. +// +// Cloud builds get the canonical URL and are indexable. Self-hosted builds keep the same title and description but +// ask search engines not to index them: a company's own instance is private. -const title = 'LiveReview' || package.name; -const description = `A powerful tool for automated code reviews` || package.description; -const homepage = '' || package.homepage; -const thumbnail = `${homepage}/public/thumbnail.jpg`; +const siteURL = 'https://livereview.hexmos.com'; +const title = 'LiveReview: Auto-approve up to 50% of your pull requests, safely'; +// Search engines show about 155 characters of the description. +const description = + 'Risk-aware AI code review: LiveReview scores every pull request, auto-approves the safe ones and sends only risky changes to your reviewers.'; +const image = `${siteURL}/public/og-image.png`; // 1200x630 +const imageAlt = 'LiveReview: auto-approve up to 50% of your pull requests, safely'; -module.exports = { - title, - meta: { - language: 'en', - description, - keywords: 'code review, automation, gitlab, github, continuous integration', - author: 'LiveReview Team', - /** - * Open Graph tags optimize how your pages appear when shared on social media platform - */ - 'og:title': title, - 'og:description': description, - 'og:url': homepage, - 'og:image': thumbnail, - /** - * Similar to Open Graph tags, but specifically for Twitter. They enhance the way your content appears when shared on Twitter. - */ - 'twitter:card': 'summary_large_image', - 'twitter:title': title, - 'twitter:description': description, - 'twitter:image': thumbnail, - /** - * Use index (default) to allow indexing, or noindex for sensitive/non-public pages. - * Use follow to allow crawling of links on the page. - */ - 'robots': 'index, follow', - }, -}; \ No newline at end of file +// Open Graph tags must use the property attribute (Facebook and LinkedIn ignore name="og:..."). +const og = (property, content) => ({ property, content }); + +module.exports = function buildMetaConfig(isCloud) { + return { + title, + siteURL: isCloud ? siteURL : '', + meta: { + description, + keywords: + 'AI code review, pull request review, auto-approve pull requests, risk scoring, GitHub App, GitLab merge request review, code review automation', + author: 'Hexmos', + robots: isCloud ? 'index, follow' : 'noindex, nofollow', + 'og:type': og('og:type', 'website'), + 'og:site_name': og('og:site_name', 'LiveReview'), + 'og:title': og('og:title', title), + 'og:description': og('og:description', description), + ...(isCloud ? { 'og:url': og('og:url', siteURL) } : {}), + 'og:image': og('og:image', image), + 'og:image:width': og('og:image:width', '1200'), + 'og:image:height': og('og:image:height', '630'), + 'og:image:alt': og('og:image:alt', imageAlt), + 'twitter:card': 'summary_large_image', + 'twitter:title': title, + 'twitter:description': description, + 'twitter:image': image, + 'twitter:image:alt': imageAlt, + }, + }; +}; diff --git a/ui/package.json b/ui/package.json index 5457b3b1..b53e9db6 100644 --- a/ui/package.json +++ b/ui/package.json @@ -1,7 +1,7 @@ { "name": "livereview-ui", "version": "1.0.0", - "description": "UI for LiveReview - A powerful tool for automated code reviews", + "description": "UI for LiveReview: risk-aware AI code review that auto-approves safe pull requests", "main": "index.js", "scripts": { "test": "jest --passWithNoTests", @@ -27,7 +27,7 @@ "bugs": { "url": "https://github.com/vannizhang/react-redux-boilerplate/issues" }, - "homepage": "https://github.com/vannizhang/react-redux-boilerplate", + "homepage": "https://livereview.hexmos.com", "devDependencies": { "@babel/core": "^7.29.6", "@babel/plugin-bugfix-safari-rest-destructuring-rhs-array": "^7.29.3", diff --git a/ui/public/index.html b/ui/public/index.html index fdfd0c23..d70e982b 100644 --- a/ui/public/index.html +++ b/ui/public/index.html @@ -8,7 +8,9 @@ <% if (htmlWebpackPlugin.options.favicon) { %> <% } %> - + <% if (htmlWebpackPlugin.options.siteURL) { %> + + <% } %>