feat(auth): github app #570
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR adds GitHub App authentication support to the backend by introducing a GithubAppManager for handling OAuth App installation tokens, integrating GitHub App credential resolution throughout the codebase, renaming Review Agent environment variables for clarity, and adding corresponding TypeScript and JSON schema definitions. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Backend Init
participant GAM as GithubAppManager
participant Config as Config/DB
participant Octokit as Octokit App
participant Cache as Token Cache
participant GH as GitHub API
App->>GAM: getInstance().init(prisma)
activate GAM
GAM->>Config: Load githubApp configs
GAM->>Config: Resolve private keys (secret/env)
GAM->>Octokit: Create Octokit App instances
Octokit->>GH: List installations
GH-->>Octokit: installations[]
GAM->>Cache: Store installations (owner+hostname key)
deactivate GAM
Note over App,Cache: Token Request Flow
App->>GAM: getInstallationToken(owner, hostname)
activate GAM
GAM->>Cache: Lookup installation
alt Token expired
GAM->>Octokit: refreshToken(installation)
Octokit->>GH: Request new token
GH-->>Octokit: token
GAM->>Cache: Update cache
end
GAM-->>App: token
deactivate GAM
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes The changes span multiple file types (backend logic, schemas, documentation, frontend config) with consistent patterns throughout. The core logic (GithubAppManager singleton, token caching, credential resolution) is straightforward but requires verification of initialization timing and entitlement checks. Schema files are auto-generated, and environment variable renames are repetitive. Mixed complexity across files warrants moderate review time. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
packages/schemas/src/v3/app.type.ts (1)
4-6: Consider whether the loose typing is intentional.The
GithubAppConfigtype uses an index signature[k: string]: unknown, which provides no type safety and allows any properties. If this is auto-generated from the JSON schema, verify that the schema generator is correctly producing typed definitions. If the loose typing is intentional for extensibility, consider documenting this design decision.packages/backend/src/ee/githubAppManager.ts (1)
108-123: Prefer Date objects for expiry comparison.Comparing ISO 8601 date strings lexicographically works but is less explicit and harder to understand than using Date objects.
Apply this diff for clearer date comparison:
- if (installation.expiresAt < new Date().toISOString()) { + if (new Date(installation.expiresAt) < new Date()) { const octokitApp = this.octokitApps.get(installation.appId) as App; const installationOctokit = await octokitApp.getInstallationOctokit(installation.id); const auth = await installationOctokit.auth({ type: "installation" }) as { expires_at: string, token: string };packages/backend/src/github.ts (1)
60-92: Consider simplifying the authentication check.The
isAuthenticatedcheck on Line 82 appears redundant. IfGithubAppManager.getInstance().getInstallationToken()successfully returns a token,createOctokitFromTokenwill always returnisAuthenticated: true(since!!tokenwill be true). The else branch at Line 85 is unreachable in normal operation.You can simplify this to:
try { const hostname = url ? new URL(url).hostname : GITHUB_CLOUD_HOSTNAME; const token = await GithubAppManager.getInstance().getInstallationToken(owner, hostname); const { octokit: octokitFromToken } = await createOctokitFromToken({ token, url, }); - - if (isAuthenticated) { - return octokitFromToken; - } else { - logger.error(`Failed to authenticate with GitHub App for ${context}. Falling back to legacy token resolution.`); - return octokit; - } + return octokitFromToken; } catch (error) { logger.error(`Error getting GitHub App token for ${context}. Falling back to legacy token resolution.`, error); return octokit; }The catch block already handles all failure cases, making the inner check unnecessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
docs/docs/configuration/auth/providers.mdx(1 hunks)docs/docs/configuration/environment-variables.mdx(1 hunks)docs/docs/features/agents/review-agent.mdx(2 hunks)docs/snippets/schemas/v3/app.schema.mdx(1 hunks)docs/snippets/schemas/v3/githubApp.schema.mdx(1 hunks)packages/backend/src/ee/githubAppManager.ts(1 hunks)packages/backend/src/ee/repoPermissionSyncer.ts(0 hunks)packages/backend/src/github.ts(7 hunks)packages/backend/src/index.ts(2 hunks)packages/backend/src/utils.ts(2 hunks)packages/schemas/src/v3/app.schema.ts(1 hunks)packages/schemas/src/v3/app.type.ts(1 hunks)packages/schemas/src/v3/githubApp.schema.ts(1 hunks)packages/schemas/src/v3/githubApp.type.ts(1 hunks)packages/web/src/app/[domain]/agents/page.tsx(1 hunks)packages/web/src/app/[domain]/components/settingsDropdown.tsx(1 hunks)packages/web/src/app/api/(server)/webhook/route.ts(1 hunks)packages/web/src/ee/features/sso/sso.ts(1 hunks)packages/web/src/env.mjs(1 hunks)schemas/v3/app.json(1 hunks)schemas/v3/githubApp.json(1 hunks)
💤 Files with no reviewable changes (1)
- packages/backend/src/ee/repoPermissionSyncer.ts
🔇 Additional comments (19)
packages/web/src/app/[domain]/components/settingsDropdown.tsx (2)
76-76: LGTM: Dropdown positioning improvement.The addition of
align="end"andsideOffset={5}properly positions the dropdown menu relative to its trigger button, improving the UI presentation.
79-94: LGTM: Well-executed user header refactor.The refactored user header section includes several solid improvements:
- Better visual spacing with
gap-3 px-3 py-3- Explicit avatar dimensions prevent layout shifts
- Defensive fallback logic on Line 85 handles edge cases (null/undefined/empty name)
- Conditional email rendering avoids displaying undefined values
- Proper truncation with
min-w-0andtruncateclasses prevents overflowThe implementation handles edge cases well and follows React/Next.js best practices.
packages/web/src/app/api/(server)/webhook/route.ts (1)
17-26: LGTM! Consistent environment variable renames.The environment variable renames from
GITHUB_APP_*toGITHUB_REVIEW_AGENT_APP_*are applied consistently across the initialization guard, private key loading, app ID, and webhook secret configuration. The logic remains unchanged.packages/web/src/env.mjs (1)
88-94: LGTM! Schema definitions align with usage.The renamed environment variable schemas (
GITHUB_REVIEW_AGENT_APP_*) and the newREVIEW_AGENT_REVIEW_COMMANDwith a default value align correctly with their usage in the webhook route and agents page.packages/web/src/app/[domain]/agents/page.tsx (1)
11-11: LGTM! Required env vars updated correctly.The
requiredEnvVarsarray correctly reflects the renamed environment variables and aligns with the schema definitions inenv.mjs.docs/docs/configuration/auth/providers.mdx (1)
36-42: LGTM! Clear documentation of GitHub App authentication support.The documentation clearly explains that both GitHub OAuth App and GitHub App are supported for authentication, lists the required permissions for GitHub App usage, and references the correct environment variables. The conditional permission requirement for permission syncing is well documented.
schemas/v3/app.json (1)
1-9: No issues found—schema reference is valid and properly defined.The referenced file
schemas/v3/githubApp.jsonexists and is correctly structured to validate theGithubAppConfigvariant with proper schema declarations, field definitions, and constraints.packages/web/src/ee/features/sso/sso.ts (1)
26-28: The GitHub Enterprise configuration is correct—no changes needed.After verification, the implementation properly handles GitHub Enterprise Server authentication. Setting enterprise.baseUrl in Next-Auth's GitHub provider causes the provider to automatically derive the OAuth endpoints (authorization, token) and API calls from that base URL. This is the documented and intended approach rather than requiring explicit endpoint configuration. The code at lines 26–28 correctly delegates endpoint resolution to Next-Auth, which will automatically construct the proper authorization and token endpoints for the GitHub Enterprise instance.
docs/docs/features/agents/review-agent.mdx (1)
47-49: LGTM!The environment variable descriptions are accurate and correctly aligned with the renamed variables.
packages/schemas/src/v3/app.schema.ts (1)
1-113: LGTM!Auto-generated schema file with proper structure for GithubAppConfig validation.
packages/schemas/src/v3/githubApp.schema.ts (1)
1-107: LGTM!Auto-generated schema file with consistent structure.
docs/snippets/schemas/v3/githubApp.schema.mdx (1)
1-108: LGTM!Auto-generated schema documentation consistent with the TypeScript schema definitions.
packages/schemas/src/v3/githubApp.type.ts (1)
1-50: LGTM!Auto-generated TypeScript type definition. Note that the TypeScript type system cannot enforce the JSON Schema's
oneOfconstraint (requiring eitherprivateKeyorprivateKeyPath), but this is expected—runtime validation will use the JSON Schema for that enforcement.docs/snippets/schemas/v3/app.schema.mdx (1)
1-114: LGTM!Auto-generated schema documentation consistent with the AppConfig schema definition.
packages/backend/src/utils.ts (1)
130-150: The displayName "org/repo" format assumption is validated with explicit null checks.GitHub repos created via the standard API pathway have
displayNamein "org/repo" format from GitHub'sfull_namefield. The code inpackages/backend/src/ee/repoPermissionSyncer.tsproperly validates this assumption with an explicit null check (lines 176-177) before splitting the displayName. Additionally,packages/backend/src/utils.ts:132uses optional chaining as an alternative defensive pattern. Both approaches safely handle the nullable schema field while ensuring the "org/repo" format assumption holds for GitHub repositories.schemas/v3/githubApp.json (1)
34-46: LGTM!The required fields and
oneOfconstraint properly enforce that eitherprivateKeyorprivateKeyPathmust be provided (but not both), while preventing extraneous properties.packages/backend/src/ee/githubAppManager.ts (2)
24-47: LGTM!The singleton pattern and initialization guard are implemented correctly, ensuring thread-safe access and preventing use before initialization.
126-132: LGTM!The helper methods are straightforward and correctly implement their intended functionality.
packages/backend/src/github.ts (1)
217-275: LGTM!The GitHub App integration is consistently applied across all three repository-fetching functions (
getReposOwnedByUsers,getReposForOrgs,getRepos). The pattern of obtaining an authenticated Octokit instance when available and falling back to the original instance ensures backward compatibility while enabling GitHub App authentication.Also applies to: 277-322, 324-371
This comment has been minimized.
This comment has been minimized.
2 participants
Summary by CodeRabbit
New Features
Documentation
Chores
GITHUB_APP_*→GITHUB_REVIEW_AGENT_APP_*.REVIEW_AGENT_REVIEW_COMMANDenvironment variable with default value.