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..1b029641 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,11 @@ 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, err := userScopedRepoIDs(connector) + if err != nil { + return err + } cursor := "" total := 0 @@ -138,6 +144,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 +178,42 @@ 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). An +// unreadable list is an error rather than "no repositories", because the sync drops repositories not on it. +func userScopedRepoIDs(connector *ConnectorDetails) (allowed map[string]bool, scoped bool, err error) { + if connector.TokenType != storagegithub.TokenTypeGitHubApp || connector.Metadata["repo_scope"] != storagegithub.RepoScopeUser { + return nil, false, nil + } + ids, ok := connector.Metadata["allowed_repo_ids"].([]interface{}) + if !ok { + return nil, true, fmt.Errorf("connector %d is user-scoped but has no readable allowed_repo_ids", connector.ID) + } + allowed = map[string]bool{} + for _, raw := range ids { + id, ok := raw.(float64) + if !ok { + return nil, true, fmt.Errorf("connector %d has a non-numeric allowed_repo_ids entry %v", connector.ID, raw) + } + allowed[strconv.FormatInt(int64(id), 10)] = true + } + return allowed, true, nil +} + // 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..3cfe2d31 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,28 @@ 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) + // Remove every org's connector even if one fails. Retrying is safe: GitHub answers 404 for the already + // uninstalled App, which DeleteInstallation treats as done. + yes, failed := true, false + for _, r := range recs { + err := store.SetAppConnectorFlags(r.ID, &yes, nil) + if err == nil { + err = repos.DeleteRepositoriesByConnector(r.ID) + } + if err != nil { + log.Printf("[GitHubApp] uninstall installation %d: removing connector %d failed: %v", meta.InstallationID, r.ID, err) + failed = true + } + } + if failed { 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 +244,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 +396,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 +423,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 +455,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 +490,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 +657,19 @@ 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 { + PullRequest *struct { + HTMLURL string `json:"html_url"` + } `json:"pull_request"` // present only when the issue is a pull request + } `json:"issue"` + PullRequest *struct { + HTMLURL string `json:"html_url"` + } `json:"pull_request"` } func (s *Server) gitHubAppWebhookHandler(authMiddleware *auth.AuthMiddleware) echo.HandlerFunc { @@ -542,34 +694,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. A failure there must not block the org + // handling the event; the periodic PR reconciliation sweep catches those copies up. + 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) + } + } + // 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 +760,105 @@ func (s *Server) gitHubAppWebhookHandler(authMiddleware *auth.AuthMiddleware) ec } } -func (s *Server) handleGitHubAppInstallationEvent(c echo.Context, store *storagegithub.TokenStore, rec *storagegithub.IntegrationTokenRecord, env githubAppWebhookEnvelope) error { +// 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 env.Issue != nil && env.Issue.PullRequest != nil { + return env.Issue.PullRequest.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 - 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) + failed := false // keep going so one connector's failure doesn't leave the others unchanged + 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) + failed = true + } } - if err != nil { - log.Printf("[GitHubApp] installation %q event failed for connector %d: %v", env.Action, rec.ID, err) + if failed { 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 +874,14 @@ 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. + 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 { @@ -683,6 +942,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 +954,23 @@ 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) + var ownerEmails []string + if err == nil { + ownerEmails, err = store.OtherOrgOwnerEmailsForInstallation(cfg.BaseURL, meta.InstallationID, orgID) + } + if err != nil { + 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 { @@ -707,6 +984,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, OtherAccounts: otherAccounts, }) if light { continue @@ -716,7 +994,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) @@ -746,6 +1031,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 1f86c050..2091a944 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}) } - if rec := signIn(emailA); rec.Code != http.StatusFound { - t.Fatalf("first owner: status %d: %s", rec.Code, rec.Body.String()) + // 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()) } - 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()) + // 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()) } - var orgA int64 + 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, 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,106 @@ 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 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)}, + }} + allowed, scoped, err := userScopedRepoIDs(user) + if err != nil || !scoped || len(allowed) != 2 || !allowed["101"] || !allowed["202"] { + t.Fatalf("user scope: %v %v %v", allowed, scoped, err) + } + empty := &ConnectorDetails{TokenType: storagegithub.TokenTypeGitHubApp, Metadata: map[string]interface{}{ + "repo_scope": "user", "allowed_repo_ids": []interface{}{}, + }} + if allowed, scoped, err := userScopedRepoIDs(empty); err != nil || !scoped || len(allowed) != 0 { + t.Fatalf("a user with access to nothing: %v %v %v", allowed, scoped, err) + } + // An unreadable list must stop the sync, not count as "no repositories" (which would drop them all). + for name, meta := range map[string]map[string]interface{}{ + "missing": {"repo_scope": "user"}, + "strings": {"repo_scope": "user", "allowed_repo_ids": []interface{}{"101"}}, + } { + if _, _, err := userScopedRepoIDs(&ConnectorDetails{TokenType: storagegithub.TokenTypeGitHubApp, Metadata: meta}); err == nil { + t.Errorf("%s allowed_repo_ids: want an error", name) + } + } + 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) @@ -602,6 +802,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) @@ -627,7 +878,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 +924,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..aee9ec63 100644 --- a/internal/docindex/docs/routes_guide/auth/gh-login.md +++ b/internal/docindex/docs/routes_guide/auth/gh-login.md @@ -36,8 +36,21 @@ 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. -- **Other ways to sign in** — a link, with Google, LinkedIn and email icons, to - the standard Hexmos sign-in, unchanged. +- **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. +- **Or sign in with** — a row of Google, LinkedIn and Email tiles under the + GitHub button. Each goes straight to the Hexmos sign-in page + (`hexmos.com/signin?app=livereview`), which offers all of these methods, and + returns to LiveReview signed in, like the normal login. - **`?src=launch-trial`** (the Product Hunt launch link): when this sign-in creates a new organization, that organization can review on the LiveReview AI Model on the Free plan, with no AI key to add, within the Free plan's monthly LOC limit. diff --git a/internal/docindex/docs/routes_guide/get-started-github.md b/internal/docindex/docs/routes_guide/get-started-github.md index 96db16d2..4302d2b4 100644 --- a/internal/docindex/docs/routes_guide/get-started-github.md +++ b/internal/docindex/docs/routes_guide/get-started-github.md @@ -35,11 +35,22 @@ 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"); 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 + 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..fea36980 100644 --- a/internal/docindex/docs/routes_guide/git/git-providers.md +++ b/internal/docindex/docs/routes_guide/git/git-providers.md @@ -40,14 +40,27 @@ 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. 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. + ### 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..01f9dfaf 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,87 @@ 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 +} + +// 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) { + var id int64 + err := s.db.QueryRow(` + SELECT connector_id FROM reviews + WHERE connector_id = ANY($1) AND (pr_mr_url = $2 OR starts_with(pr_mr_url, $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 +345,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 +388,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 +400,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..b7bce099 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#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,9 +210,16 @@ 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#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.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) | +| pullrequests.DeleteRepositoriesByConnectorExcept | added | [DeleteRepositoriesByConnectorExcept](providers/pullrequests/upsert.go#L205) | +| pullrequests.SiblingPullRequestForUser | added | [SiblingPullRequestForUser](providers/pullrequests/upsert.go#L216) | 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) { %> + + <% } %>