Support public repositories and creation-time guest access for app identities - #1342
Draft
ikhoon wants to merge 1 commit into
Draft
Support public repositories and creation-time guest access for app identities#1342ikhoon wants to merge 1 commit into
ikhoon wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds project-level control over public repositories, public/private repository creation and role changes, guest-access scopes for app identities, UI indicators and settings, xDS policy enforcement, and backend/frontend coverage. ChangesPublic repository access
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Owner
participant WebApp
participant MetadataApiService
participant MetadataService
participant RepositoryServiceV1
Owner->>WebApp: enable public repositories
WebApp->>MetadataApiService: PUT project settings
MetadataApiService->>MetadataService: update project policy
RepositoryServiceV1->>MetadataService: create public repository
MetadataService-->>RepositoryServiceV1: assign guest READ role
RepositoryServiceV1-->>WebApp: return repository result
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
webapp/src/dogma/common/components/RepoIcon.tsx (1)
60-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared visibility badge component. Both sites render the same "Public"/"Private"
Badge(fontSize="x-small",variant="outline",borderRadius="full",px={2}, teal/gray bycolorScheme) inline, so any future styling tweak needs to be applied in two places.
webapp/src/dogma/common/components/RepoIcon.tsx#L60-L74: extract the conditional "Public" badge into a sharedVisibilityBadge/PublicBadgecomponent.webapp/src/dogma/features/repo/RepoRoleList.tsx#L45-L55: reuse the same shared component for the Public/Private cell instead of duplicating the Badge JSX.♻️ Example shared component
// webapp/src/dogma/common/components/VisibilityBadge.tsx import { Badge } from '`@chakra-ui/react`'; export const VisibilityBadge = ({ isPublic }: { isPublic: boolean }) => isPublic ? ( <Badge fontSize="x-small" colorScheme="teal" variant="outline" borderRadius="full" px={2}> Public </Badge> ) : ( <Badge fontSize="x-small" colorScheme="gray" variant="outline" borderRadius="full" px={2}> Private </Badge> );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/dogma/common/components/RepoIcon.tsx` around lines 60 - 74, Extract the duplicated visibility Badge markup into a shared VisibilityBadge/PublicBadge component. In webapp/src/dogma/common/components/RepoIcon.tsx lines 60-74, replace the inline conditional Public badge with the shared component; in webapp/src/dogma/features/repo/RepoRoleList.tsx lines 45-55, replace the duplicated Public/Private Badge JSX with the same component, preserving teal styling for public and gray styling for private repositories.server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java (1)
407-453: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated "fast-path vs precheck-then-push" pattern across three methods.
addRepo,updateRepositoryProjectRoles, andupdateAllowPublicRepositorieseach independently implement the same shape: skip the extra read when no public-repository transition is involved, otherwisefetchMetadatafor a friendly 400 message, then push (with the transformer doing the real atomic re-validation). The logic is correct in all three places today, but the duplication means a future change to one copy (e.g. adjusting the race-safety contract) can silently diverge from the other two.Consider extracting a small shared helper, e.g.
pushOrPrecheckPublicRepoPolicy(projectName, commitSummary, transformer, needsPrecheck, precheckAndMaybeThrow), that the three call sites delegate to.♻️ Sketch of a shared helper
+ private CompletableFuture<Revision> pushWithPublicRepoPrecheck( + String projectName, String commitSummary, ProjectMetadataTransformer transformer, + boolean skipPrecheck, Consumer<ProjectMetadata> precheck) { + if (skipPrecheck) { + return metadataRepo.push(projectName, Project.REPO_DOGMA, author, commitSummary, transformer); + } + return fetchMetadata(projectName).thenCompose(projectMetadata -> { + precheck.accept(projectMetadata); // throws IllegalArgumentException with a specific message + return metadataRepo.push(projectName, Project.REPO_DOGMA, author, commitSummary, transformer); + }); + }Also applies to: 544-648
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java` around lines 407 - 453, Extract the duplicated fast-path versus precheck-then-push flow from addRepo, updateRepositoryProjectRoles, and updateAllowPublicRepositories into a shared helper such as pushOrPrecheckPublicRepoPolicy. Have the helper accept the project name, commit summary, transformer, whether a precheck is needed, and the precheck validation, while preserving the existing friendly error and atomic transformer re-validation behavior at all three call sites.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@server/src/test/java/com/linecorp/centraldogma/server/metadata/MetadataApiServiceTest.java`:
- Around line 539-606: Wrap the final project-metadata GET and its
allowsPublicRepositories assertion in the existing Awaitility polling pattern
used by the sibling public-repository precheck test. Poll until the freshly
updated metadata reports false, while keeping the existing request and assertion
semantics unchanged.
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/internal/ControlPlanePlugin.java`:
- Around line 59-77: Update disallowPublicRepositories to use an atomic metadata
operation that conditionally changes allowPublicRepositories only from null to
false. Ensure the operation no-ops when the current value is either explicit
true or false, preserving owner decisions made after the initial metadata read,
and retain the existing retry warning behavior on failure.
---
Nitpick comments:
In
`@server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java`:
- Around line 407-453: Extract the duplicated fast-path versus
precheck-then-push flow from addRepo, updateRepositoryProjectRoles, and
updateAllowPublicRepositories into a shared helper such as
pushOrPrecheckPublicRepoPolicy. Have the helper accept the project name, commit
summary, transformer, whether a precheck is needed, and the precheck validation,
while preserving the existing friendly error and atomic transformer
re-validation behavior at all three call sites.
In `@webapp/src/dogma/common/components/RepoIcon.tsx`:
- Around line 60-74: Extract the duplicated visibility Badge markup into a
shared VisibilityBadge/PublicBadge component. In
webapp/src/dogma/common/components/RepoIcon.tsx lines 60-74, replace the inline
conditional Public badge with the shared component; in
webapp/src/dogma/features/repo/RepoRoleList.tsx lines 45-55, replace the
duplicated Public/Private Badge JSX with the same component, preserving teal
styling for public and gray styling for private repositories.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a14ffdc1-8816-40d4-89e1-e8348281679b
📒 Files selected for processing (40)
.gitignorecommon/src/main/java/com/linecorp/centraldogma/internal/api/v1/CreateRepositoryRequest.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/MetadataApiService.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceUtil.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/AppIdentityRegistryService.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/storage/project/DefaultProject.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/AppIdentityService.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/ProjectMetadata.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/RepositoryMetadata.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/RepositoryMetadataTransformer.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/admin/model/SerializationTest.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/api/AppIdentityRegistryServiceTest.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1Test.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/MetadataApiServiceTest.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/MetadataServiceTest.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/ProjectMetadataTest.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/TokenGuestPermissionTest.javasite/src/sphinx/auth.rstwebapp/src/dogma/common/components/RepoIcon.tsxwebapp/src/dogma/features/api/apiSlice.tswebapp/src/dogma/features/app-identity/NewAppIdentity.tsxwebapp/src/dogma/features/project/ProjectMetadataDto.tswebapp/src/dogma/features/project/settings/AllowPublicRepositoriesToggle.tsxwebapp/src/dogma/features/project/settings/repositories/RepoMetaList.tsxwebapp/src/dogma/features/repo/NewRepo.tsxwebapp/src/dogma/features/repo/RepoList.tsxwebapp/src/dogma/features/repo/RepoRoleList.tsxwebapp/src/dogma/features/repo/RepositoriesMetadataDto.tswebapp/src/dogma/features/repo/roles/ConfirmUpdateRepositoryProjectRoles.tsxwebapp/src/dogma/features/repo/roles/ProjectRolesForm.tsxwebapp/src/pages/app/projects/[projectName]/index.tsxwebapp/src/pages/app/projects/[projectName]/repos/[repoName]/settings/index.tsxwebapp/src/pages/app/projects/[projectName]/settings/index.tsxwebapp/src/pages/app/settings/app-identities/index.tsxwebapp/tests/dogma/feature/repo/RepoList.test.tsxwebapp/tests/dogma/feature/repo/roles/ProjectRolesForm.test.tsxxds/src/main/java/com/linecorp/centraldogma/xds/internal/ControlPlanePlugin.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/ControlPlanePluginTest.java
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@site/src/sphinx/auth.rst`:
- Around line 341-348: Update the repository configuration bullets immediately
preceding the role definitions to describe setting repository roles rather than
setting permissions. Align their terminology with the READ, WRITE, and ADMIN
roles used in the surrounding authentication section, without changing the
access-control behavior or other documentation.
- Around line 388-391: Update the token-scope documentation near the guest
access explanation to also cover certificate-backed identities, specifying how
they obtain public-repository access—either by configuring guest access for the
certificate identity or by inheriting allowGuestAccess from the associated app
identity. Keep the existing application-token behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a27f491d-14a4-4a1f-95f8-9ec7411a56fb
📒 Files selected for processing (40)
.gitignorecommon/src/main/java/com/linecorp/centraldogma/internal/api/v1/CreateRepositoryRequest.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/MetadataApiService.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceUtil.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/AppIdentityRegistryService.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/storage/project/DefaultProject.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/AppIdentityService.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/ProjectMetadata.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/RepositoryMetadata.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/RepositoryMetadataTransformer.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/admin/model/SerializationTest.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/api/AppIdentityRegistryServiceTest.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1Test.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/MetadataApiServiceTest.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/MetadataServiceTest.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/ProjectMetadataTest.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/TokenGuestPermissionTest.javasite/src/sphinx/auth.rstwebapp/src/dogma/common/components/RepoIcon.tsxwebapp/src/dogma/features/api/apiSlice.tswebapp/src/dogma/features/app-identity/NewAppIdentity.tsxwebapp/src/dogma/features/project/ProjectMetadataDto.tswebapp/src/dogma/features/project/settings/AllowPublicRepositoriesToggle.tsxwebapp/src/dogma/features/project/settings/repositories/RepoMetaList.tsxwebapp/src/dogma/features/repo/NewRepo.tsxwebapp/src/dogma/features/repo/RepoList.tsxwebapp/src/dogma/features/repo/RepoRoleList.tsxwebapp/src/dogma/features/repo/RepositoriesMetadataDto.tswebapp/src/dogma/features/repo/roles/ConfirmUpdateRepositoryProjectRoles.tsxwebapp/src/dogma/features/repo/roles/ProjectRolesForm.tsxwebapp/src/pages/app/projects/[projectName]/index.tsxwebapp/src/pages/app/projects/[projectName]/repos/[repoName]/settings/index.tsxwebapp/src/pages/app/projects/[projectName]/settings/index.tsxwebapp/src/pages/app/settings/app-identities/index.tsxwebapp/tests/dogma/feature/repo/RepoList.test.tsxwebapp/tests/dogma/feature/repo/roles/ProjectRolesForm.test.tsxxds/src/main/java/com/linecorp/centraldogma/xds/internal/ControlPlanePlugin.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/ControlPlanePluginTest.java
🚧 Files skipped from review as they are similar to previous changes (32)
- webapp/src/pages/app/settings/app-identities/index.tsx
- webapp/src/pages/app/projects/[projectName]/settings/index.tsx
- webapp/src/pages/app/projects/[projectName]/index.tsx
- server/src/main/java/com/linecorp/centraldogma/server/metadata/RepositoryMetadata.java
- webapp/src/dogma/features/repo/RepoList.tsx
- common/src/main/java/com/linecorp/centraldogma/internal/api/v1/CreateRepositoryRequest.java
- webapp/src/dogma/features/project/ProjectMetadataDto.ts
- webapp/src/dogma/features/project/settings/AllowPublicRepositoriesToggle.tsx
- webapp/tests/dogma/feature/repo/roles/ProjectRolesForm.test.tsx
- server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/AppIdentityRegistryService.java
- server/src/main/java/com/linecorp/centraldogma/server/metadata/RepositoryMetadataTransformer.java
- webapp/src/dogma/common/components/RepoIcon.tsx
- webapp/src/dogma/features/project/settings/repositories/RepoMetaList.tsx
- server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceUtil.java
- server/src/test/java/com/linecorp/centraldogma/server/metadata/ProjectMetadataTest.java
- webapp/src/dogma/features/repo/roles/ProjectRolesForm.tsx
- webapp/tests/dogma/feature/repo/RepoList.test.tsx
- webapp/src/dogma/features/repo/RepositoriesMetadataDto.ts
- server/src/main/java/com/linecorp/centraldogma/server/internal/storage/project/DefaultProject.java
- .gitignore
- server/src/test/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1Test.java
- server/src/main/java/com/linecorp/centraldogma/server/metadata/AppIdentityService.java
- webapp/src/dogma/features/repo/roles/ConfirmUpdateRepositoryProjectRoles.tsx
- server/src/test/java/com/linecorp/centraldogma/server/internal/admin/model/SerializationTest.java
- server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.java
- webapp/src/dogma/features/app-identity/NewAppIdentity.tsx
- webapp/src/dogma/features/repo/RepoRoleList.tsx
- server/src/main/java/com/linecorp/centraldogma/server/metadata/ProjectMetadata.java
- webapp/src/dogma/features/repo/NewRepo.tsx
- server/src/test/java/com/linecorp/centraldogma/server/internal/api/AppIdentityRegistryServiceTest.java
- server/src/test/java/com/linecorp/centraldogma/server/metadata/MetadataApiServiceTest.java
- server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java
There was a problem hiding this comment.
♻️ Duplicate comments (1)
xds/src/main/java/com/linecorp/centraldogma/xds/internal/ControlPlanePlugin.java (1)
59-71: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve an owner’s explicit policy choice atomically.
The null check is stale by Line 71: an owner can set the policy to
trueafter Line 63, andupdateAllowPublicRepositories(..., false)will overwrite it. Add an atomic metadata operation that only transitionsnull → falseand no-ops for either explicit value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xds/src/main/java/com/linecorp/centraldogma/xds/internal/ControlPlanePlugin.java` around lines 59 - 71, Update disallowPublicRepositories so the metadata update uses an atomic conditional transition from null to false, leaving both explicit true and false values unchanged even if set after the initial read. Add or reuse an atomic MetadataService operation for this behavior and invoke it instead of unconditional updateAllowPublicRepositories.
🧹 Nitpick comments (4)
server/src/main/java/com/linecorp/centraldogma/server/metadata/ProjectMetadata.java (2)
162-177: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a less collision-prone name for the derived accessor.
allowPublicRepositories()(raw, nullable tri-state) andallowsPublicRepositories()(effective boolean) differ by a single character but have materially different semantics — and the raw one NPEs if unboxed in a boolean context. Something likepublicRepositoriesAllowed()orisPublicRepositoriesAllowed()for the derived form would make accidental misuse much harder to write.♻️ Suggested rename
- public boolean allowsPublicRepositories() { + public boolean publicRepositoriesAllowed() { return allowPublicRepositories == null || allowPublicRepositories; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/main/java/com/linecorp/centraldogma/server/metadata/ProjectMetadata.java` around lines 162 - 177, Rename the derived boolean accessor allowsPublicRepositories() to a more distinct name such as publicRepositoriesAllowed() or isPublicRepositoriesAllowed(), while preserving its existing default-allowed behavior. Update all call sites and references to use the new name; leave the nullable raw accessor allowPublicRepositories() unchanged.
77-82: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRolling-upgrade downgrade path silently re-allows public repositories.
With
@JsonIgnoreProperties(ignoreUnknown = true)(line 43), an older replica that readsmetadata.jsondropsallowPublicRepositories, and any metadata push it performs rewrites the file without the field — whichallowsPublicRepositories()then interprets as allowed. The PR notes this caveat; consider calling it out in the upgrade/release notes so operators sequence the upgrade before relying on the setting as a security control.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/main/java/com/linecorp/centraldogma/server/metadata/ProjectMetadata.java` around lines 77 - 82, Document the rolling-upgrade downgrade caveat in the relevant upgrade or release notes: older replicas ignore ProjectMetadata.allowPublicRepositories because of JsonIgnoreProperties and can rewrite metadata.json so allowsPublicRepositories() defaults to allowing public repositories. Instruct operators to complete the upgrade before relying on this setting as a security control.server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java (1)
187-195: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider a copy/builder API on
ProjectMetadatato stop this class of bug recurring.Ten call sites in this file (plus
RepositoryMetadataTransformer) hand-copy everyProjectMetadatafield, so each new field requires touching all of them and any miss silently drops persisted state — exactly the failure mode this PR is fixing forallowPublicRepositories. AtoBuilder()orwithRemoval(...)/withMembers(...)style API would make future fields preserved by default.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java` around lines 187 - 195, Introduce a copy/builder API on ProjectMetadata, such as toBuilder() or focused with... methods, that preserves all existing fields by default; then update the ProjectMetadata construction sites in MetadataService and RepositoryMetadataTransformer to use it for modifications like removal and member changes instead of manually copying every field.server/src/main/java/com/linecorp/centraldogma/server/internal/api/MetadataApiService.java (1)
155-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winJavadoc contradicts actual behavior for an unspecified
allowPublicRepositories.The javadoc says an unspecified field is "left unchanged," but the handler throws a 400 (
"No settings are specified.") whenallowPublicRepositoriesisnull/omitted — confirmed by theupdateAllowPublicRepositoriestest expectingBAD_REQUESTfor an empty body{}. Update the javadoc to reflect that the sole current field is required, or adjust wording to only describe the intended future multi-field behavior once it exists.♻️ Proposed doc fix
/** * PUT /metadata/{projectName}/settings * - * <p>Updates the settings of the specified {`@code` projectName}. A field which is not specified - * is left unchanged. The body of the request will be: + * <p>Updates the settings of the specified {`@code` projectName}. At least one setting must be + * specified. The body of the request will be: * <pre>{`@code` * { * "allowPublicRepositories": false * } * }</pre> */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/main/java/com/linecorp/centraldogma/server/internal/api/MetadataApiService.java` around lines 155 - 178, Update the Javadoc for updateProjectSettings to state that allowPublicRepositories is currently required and omitted settings produce a bad request; remove the claim that unspecified fields are left unchanged while preserving the existing handler behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/internal/ControlPlanePlugin.java`:
- Around line 59-71: Update disallowPublicRepositories so the metadata update
uses an atomic conditional transition from null to false, leaving both explicit
true and false values unchanged even if set after the initial read. Add or reuse
an atomic MetadataService operation for this behavior and invoke it instead of
unconditional updateAllowPublicRepositories.
---
Nitpick comments:
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/api/MetadataApiService.java`:
- Around line 155-178: Update the Javadoc for updateProjectSettings to state
that allowPublicRepositories is currently required and omitted settings produce
a bad request; remove the claim that unspecified fields are left unchanged while
preserving the existing handler behavior.
In
`@server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java`:
- Around line 187-195: Introduce a copy/builder API on ProjectMetadata, such as
toBuilder() or focused with... methods, that preserves all existing fields by
default; then update the ProjectMetadata construction sites in MetadataService
and RepositoryMetadataTransformer to use it for modifications like removal and
member changes instead of manually copying every field.
In
`@server/src/main/java/com/linecorp/centraldogma/server/metadata/ProjectMetadata.java`:
- Around line 162-177: Rename the derived boolean accessor
allowsPublicRepositories() to a more distinct name such as
publicRepositoriesAllowed() or isPublicRepositoriesAllowed(), while preserving
its existing default-allowed behavior. Update all call sites and references to
use the new name; leave the nullable raw accessor allowPublicRepositories()
unchanged.
- Around line 77-82: Document the rolling-upgrade downgrade caveat in the
relevant upgrade or release notes: older replicas ignore
ProjectMetadata.allowPublicRepositories because of JsonIgnoreProperties and can
rewrite metadata.json so allowsPublicRepositories() defaults to allowing public
repositories. Instruct operators to complete the upgrade before relying on this
setting as a security control.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 18fbafc6-dc3f-47d1-b811-31deeaea7350
📒 Files selected for processing (41)
.gitignorecommon/src/main/java/com/linecorp/centraldogma/internal/api/v1/CreateRepositoryRequest.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/MetadataApiService.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceUtil.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/UpdateProjectSettingsRequest.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/AppIdentityRegistryService.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/storage/project/DefaultProject.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/AppIdentityService.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/ProjectMetadata.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/RepositoryMetadata.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/RepositoryMetadataTransformer.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/admin/model/SerializationTest.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/api/AppIdentityRegistryServiceTest.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1Test.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/MetadataApiServiceTest.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/MetadataServiceTest.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/ProjectMetadataTest.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/TokenGuestPermissionTest.javasite/src/sphinx/auth.rstwebapp/src/dogma/common/components/RepoIcon.tsxwebapp/src/dogma/features/api/apiSlice.tswebapp/src/dogma/features/app-identity/NewAppIdentity.tsxwebapp/src/dogma/features/project/ProjectMetadataDto.tswebapp/src/dogma/features/project/settings/AllowPublicRepositoriesToggle.tsxwebapp/src/dogma/features/project/settings/repositories/RepoMetaList.tsxwebapp/src/dogma/features/repo/NewRepo.tsxwebapp/src/dogma/features/repo/RepoList.tsxwebapp/src/dogma/features/repo/RepoRoleList.tsxwebapp/src/dogma/features/repo/RepositoriesMetadataDto.tswebapp/src/dogma/features/repo/roles/ConfirmUpdateRepositoryProjectRoles.tsxwebapp/src/dogma/features/repo/roles/ProjectRolesForm.tsxwebapp/src/pages/app/projects/[projectName]/index.tsxwebapp/src/pages/app/projects/[projectName]/repos/[repoName]/settings/index.tsxwebapp/src/pages/app/projects/[projectName]/settings/index.tsxwebapp/src/pages/app/settings/app-identities/index.tsxwebapp/tests/dogma/feature/repo/RepoList.test.tsxwebapp/tests/dogma/feature/repo/roles/ProjectRolesForm.test.tsxxds/src/main/java/com/linecorp/centraldogma/xds/internal/ControlPlanePlugin.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/ControlPlanePluginTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- site/src/sphinx/auth.rst
- .gitignore
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@server/src/main/java/com/linecorp/centraldogma/server/metadata/AppIdentityService.java`:
- Around line 83-85: The token and certificate identity flows in
AppIdentityService must preserve only explicitly requested guest access after
system-admin demotion. Update the token creation logic around Token and the
certificate identity logic around the sibling site so implied system-admin
access is cleared when isSystemAdmin becomes false, while retaining explicitly
requested guest access; ensure both identity types are handled consistently
through withSystemAdmin and appIdentityRegistryService.updateAppIdentityLevel.
In `@webapp/src/dogma/features/repo/roles/ProjectRolesForm.tsx`:
- Around line 49-56: Update the useEffect that synchronizes projectRoles so
refetched metadata is merged into fields that are not dirty, rather than
skipping reset whenever isDirty is true. Preserve the user’s edited visibility
value while applying refreshed member-role values, ensuring the save payload
does not overwrite concurrent role updates; use the form’s existing dirty-field
state and reset behavior.
In `@webapp/src/pages/app/projects/`[projectName]/index.tsx:
- Around line 21-23: Update the useGetMetadataByProjectNameQuery call to skip
execution until the router is ready and projectName is available by adding skip:
!router.isReady || !projectName, while preserving the existing refetchOnFocus
option.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d3dab9fa-9890-4957-a8b2-77df9cfb6b9e
📒 Files selected for processing (43)
.gitignorecommon/src/main/java/com/linecorp/centraldogma/internal/api/v1/CreateRepositoryRequest.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/MetadataApiService.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceUtil.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/UpdateProjectSettingsRequest.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/AppIdentityRegistryService.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/storage/project/DefaultProject.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/AppIdentityService.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/ProjectMetadata.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/RepositoryMetadata.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/RepositoryMetadataTransformer.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/admin/model/SerializationTest.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/api/AppIdentityRegistryServiceTest.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1Test.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/MetadataApiServiceTest.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/MetadataServiceTest.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/ProjectMetadataTest.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/TokenGuestPermissionTest.javasite/src/sphinx/auth.rstwebapp/src/dogma/common/components/RepoIcon.tsxwebapp/src/dogma/features/api/apiSlice.tswebapp/src/dogma/features/app-identity/NewAppIdentity.tsxwebapp/src/dogma/features/project/ProjectMetadataDto.tswebapp/src/dogma/features/project/settings/AllowPublicRepositoriesToggle.tsxwebapp/src/dogma/features/project/settings/repositories/RepoMetaList.tsxwebapp/src/dogma/features/repo/NewRepo.tsxwebapp/src/dogma/features/repo/RepoList.tsxwebapp/src/dogma/features/repo/RepoRoleList.tsxwebapp/src/dogma/features/repo/RepositoriesMetadataDto.tswebapp/src/dogma/features/repo/roles/ConfirmUpdateRepositoryProjectRoles.tsxwebapp/src/dogma/features/repo/roles/ProjectRolesForm.tsxwebapp/src/pages/app/projects/[projectName]/index.tsxwebapp/src/pages/app/projects/[projectName]/repos/[repoName]/settings/index.tsxwebapp/src/pages/app/projects/[projectName]/settings/index.tsxwebapp/src/pages/app/settings/app-identities/index.tsxwebapp/tests/dogma/feature/repo/RepoList.test.tsxwebapp/tests/dogma/feature/repo/roles/ProjectRolesForm.test.tsxxds/src/main/java/com/linecorp/centraldogma/xds/internal/ControlPlanePlugin.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsProjectUtil.javaxds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesEndpointFetchingPlugin.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/ControlPlanePluginTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- webapp/src/dogma/features/repo/RepositoriesMetadataDto.ts
- site/src/sphinx/auth.rst
Comment on lines
+21
to
+23
| const { data: metadata } = useGetMetadataByProjectNameQuery(projectName, { | ||
| refetchOnFocus: true, | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg 'webapp/src/pages/app/projects/\[projectName\]/index\.tsx|package\.json|next\.config'
echo
echo "Target file outline/size:"
wc -l webapp/src/pages/app/projects/\[projectName\]/index.tsx
ast-grep outline webapp/src/pages/app/projects/\[projectName\]/index.tsx --view expanded || true
echo
echo "Relevant target file lines:"
cat -n webapp/src/pages/app/projects/\[projectName\]/index.tsx | sed -n '1,220p'
echo
echo "Search for useRouter and router.query/readiness usage:"
rg -n "useRouter|router\.isReady|router\.query|skip:" webapp/src -S || true
echo
echo "Search query hook definitions:"
rg -n "useGetMetadataByProjectNameQuery|getMetadataByProjectName|baseQuery" webapp/src -S || trueRepository: line/centraldogma
Length of output: 39407
🌐 Web query:
Next.js useRouter router.query undefined before isReady Pages Router documentation
💡 Result:
In the Next.js Pages Router, the router.query object is intentionally empty ({}) during the initial render [1][2][3]. This behavior occurs because Next.js must ensure that the initial client-side render matches the server-side render (hydration) [4][5]. For pages that are statically optimized (prerendered without getServerSideProps), the server has no knowledge of the URL query parameters at build time, so they are not available until the component is hydrated on the client [2][4][3]. To safely access router.query or dynamic route parameters, you must use the isReady property provided by the useRouter hook [1][2][6]. Key guidelines for using isReady: 1. Availability: isReady is a boolean that indicates whether the router fields have been updated and are ready for use on the client [1][7]. It is false on the server and during the first client-side render [2][4]. 2. Usage in useEffect: You should perform operations that depend on router.query (such as fetching data or setting state) inside a useEffect hook, checking isReady to ensure the data is available [1][6][4]. javascript import { useRouter } from 'next/router'; import { useEffect } from 'react'; export default function MyPage() { const router = useRouter(); useEffect(() => { if (!router.isReady) return; // Now you can safely access router.query console.log(router.query); }, [router.isReady, router.query]); return <div>{router.isReady ? 'Ready' : 'Loading...'}</div>; } 3. Avoid Conditional Rendering on Server: The isReady property should not be used to conditionally render content on the server, as this can lead to hydration mismatches [1][7]. It is specifically designed for client-side logic after hydration [1][4].
Citations:
- 1: https://nextjs.org/docs/pages/api-reference/functions/use-router
- 2: Query parameter in dynamic route is undefined on first render vercel/next.js#21533
- 3: Initial router.query is empty when URL query is set vercel/next.js#13220
- 4: https://stackoverflow.com/questions/67332108/next-js-router-is-returning-query-parameters-as-undefined-on-first-render
- 5: Add a
ready: booleantoRouterreturned byuseRoutervercel/next.js#8259 - 6: Dynamic routes react router returns undefined vercel/next.js#44425
- 7: Add isReady field on router vercel/next.js#20628
Skip the metadata query before route readiness.
During the initial client render in the Pages Router, router.query.projectName is unavailable and can be empty before hydration. Add skip: !router.isReady || !projectName to the metadata query to avoid an early request for an invalid project.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webapp/src/pages/app/projects/`[projectName]/index.tsx around lines 21 - 23,
Update the useGetMetadataByProjectNameQuery call to skip execution until the
router is ready and projectName is available by adding skip: !router.isReady ||
!projectName, while preserving the existing refetchOnFocus option.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java (1)
422-451: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPrevent policy races from leaving an orphaned repository.
RepositoryServiceUtil.createRepositorycreates repository storage before this policy check. If the setting becomes disabled between the API precheck and Lines 422-450, metadata registration fails after storage creation, leaving the name occupied and potentially later registered as a private repository by missing-metadata repair.Add transactional coordination or safe compensation that removes storage when metadata registration fails, and cover this race with a test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java` around lines 422 - 451, Update createRepository and its metadata registration flow around the transformer and metadataRepo.push calls to coordinate repository storage creation with the allowPublicRepositories policy, or reliably delete the newly created storage whenever registration fails due to the policy race. Ensure failed registration cannot leave an orphaned repository that later appears private, and add a test covering the setting being disabled between the API precheck and metadata registration.
🧹 Nitpick comments (2)
webapp/src/dogma/common/components/RepoIcon.tsx (1)
67-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared
VisibilityBadgecomponent. The exact same Public/PrivateBadgestyling (fontSize="x-small",variant="outline",borderRadius="full",px={2}, teal/gray color scheme) is copy-pasted across three components.
webapp/src/dogma/common/components/RepoIcon.tsx#L67-L71: replace the inline PublicBadgewith a shared<VisibilityBadge isPublic />(or equivalent) component.webapp/src/dogma/features/repo/RepoRoleList.tsx#L46-L55: replace the Public/Private ternaryBadgeblock with the same shared component.webapp/src/dogma/features/repo/roles/ProjectRolesForm.tsx#L88-L98: replace the inlineBadgewith the same shared component.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/dogma/common/components/RepoIcon.tsx` around lines 67 - 71, Extract a shared VisibilityBadge component that accepts the public/private state and preserves the existing Badge styling and teal/gray color schemes. In webapp/src/dogma/common/components/RepoIcon.tsx lines 67-71, replace the inline Public Badge; in webapp/src/dogma/features/repo/RepoRoleList.tsx lines 46-55, replace the Public/Private ternary; and in webapp/src/dogma/features/repo/roles/ProjectRolesForm.tsx lines 88-98, replace the inline Badge, importing and reusing VisibilityBadge at each site.server/src/test/java/com/linecorp/centraldogma/server/metadata/ProjectMetadataTest.java (1)
32-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the default
allowPublicRepositoriesvalue in the JSON round-trip tests.
buildJsonWithTokensOnly()/buildJsonWithAppIds()omitallowPublicRepositories, but neitherdeserializeWithTokensOnlynordeserializeWithAppIdsassert the resulting default (true). Since this is the new backward-compat default being introduced, add an assertion so a future regression (e.g. defaulting tofalse) is caught.Suggested addition
final AppIdentityRegistration token = metadata.appIds().get("app-token-1"); assertThat(token.id()).isEqualTo("app-token-1"); assertThat(token.role()).isEqualTo(ProjectRole.MEMBER); + assertThat(metadata.allowPublicRepositories()).isTrue(); }Also applies to: 48-61
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/test/java/com/linecorp/centraldogma/server/metadata/ProjectMetadataTest.java` around lines 32 - 45, Update the ProjectMetadata JSON deserialization tests in deserializeWithTokensOnly and deserializeWithAppIds to assert that metadata.allowPublicRepositories() defaults to true when the field is omitted. Keep the existing token and app ID assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.java`:
- Around line 204-222: Prevent RepositoryServiceUtil.createRepository from
leaving a physical repository without metadata when the atomic public-repository
validation fails. Ensure the allowPublicRepositories check is reserved or
completed before physical creation, or compensate by removing the created
repository whenever mds.addRepo fails, while preserving existing duplicate-name
handling.
In `@site/src/sphinx/auth.rst`:
- Around line 346-349: Update the effective-role description in the
authorization documentation to include the ADMIN override granted by
MetadataService.findRepositoryRole() to system administrators and project
owners, alongside direct grants and member/guest project roles.
- Around line 354-357: Update the public repository description in the
guest-role documentation to state that implicit guest access is read-only, while
explicitly noting that direct WRITE or ADMIN repository grants can allow a guest
user or application identity to write. Remove the absolute claim that guests can
never write and preserve the existing explanation of public repository access.
In
`@xds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsProjectUtil.java`:
- Around line 36-42: Update the xDS initialization logic in XdsProjectUtil so
the public-repository policy is migrated on every initialization, not only when
INTERNAL_PROJECT_XDS is newly created. Atomically change only an unset
allowPublicRepositories value from null to false, while preserving explicit true
and false owner settings; remove the created-only guard around this migration
and use the existing project policy update mechanism.
---
Outside diff comments:
In
`@server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java`:
- Around line 422-451: Update createRepository and its metadata registration
flow around the transformer and metadataRepo.push calls to coordinate repository
storage creation with the allowPublicRepositories policy, or reliably delete the
newly created storage whenever registration fails due to the policy race. Ensure
failed registration cannot leave an orphaned repository that later appears
private, and add a test covering the setting being disabled between the API
precheck and metadata registration.
---
Nitpick comments:
In
`@server/src/test/java/com/linecorp/centraldogma/server/metadata/ProjectMetadataTest.java`:
- Around line 32-45: Update the ProjectMetadata JSON deserialization tests in
deserializeWithTokensOnly and deserializeWithAppIds to assert that
metadata.allowPublicRepositories() defaults to true when the field is omitted.
Keep the existing token and app ID assertions unchanged.
In `@webapp/src/dogma/common/components/RepoIcon.tsx`:
- Around line 67-71: Extract a shared VisibilityBadge component that accepts the
public/private state and preserves the existing Badge styling and teal/gray
color schemes. In webapp/src/dogma/common/components/RepoIcon.tsx lines 67-71,
replace the inline Public Badge; in
webapp/src/dogma/features/repo/RepoRoleList.tsx lines 46-55, replace the
Public/Private ternary; and in
webapp/src/dogma/features/repo/roles/ProjectRolesForm.tsx lines 88-98, replace
the inline Badge, importing and reusing VisibilityBadge at each site.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 23078018-0d3e-4745-a62e-b1b488eeb0da
⛔ Files ignored due to path filters (4)
site/src/sphinx/_images/auth_1.pngis excluded by!**/*.pngsite/src/sphinx/_images/auth_2.pngis excluded by!**/*.pngsite/src/sphinx/_images/auth_3.pngis excluded by!**/*.pngsite/src/sphinx/_images/auth_4.pngis excluded by!**/*.png
📒 Files selected for processing (43)
.gitignorecommon/src/main/java/com/linecorp/centraldogma/internal/api/v1/CreateRepositoryRequest.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/MetadataApiService.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceUtil.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/UpdateProjectSettingsRequest.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/AppIdentityRegistryService.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/storage/project/DefaultProject.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/AppIdentityService.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/ProjectMetadata.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/RepositoryMetadata.javaserver/src/main/java/com/linecorp/centraldogma/server/metadata/RepositoryMetadataTransformer.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/admin/model/SerializationTest.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/api/AppIdentityRegistryServiceTest.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1Test.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/MetadataApiServiceTest.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/MetadataServiceTest.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/ProjectMetadataTest.javaserver/src/test/java/com/linecorp/centraldogma/server/metadata/TokenGuestPermissionTest.javasite/src/sphinx/auth.rstwebapp/src/dogma/common/components/RepoIcon.tsxwebapp/src/dogma/features/api/apiSlice.tswebapp/src/dogma/features/app-identity/NewAppIdentity.tsxwebapp/src/dogma/features/project/ProjectMetadataDto.tswebapp/src/dogma/features/project/settings/AllowPublicRepositoriesToggle.tsxwebapp/src/dogma/features/project/settings/repositories/RepoMetaList.tsxwebapp/src/dogma/features/repo/NewRepo.tsxwebapp/src/dogma/features/repo/RepoList.tsxwebapp/src/dogma/features/repo/RepoRoleList.tsxwebapp/src/dogma/features/repo/RepositoriesMetadataDto.tswebapp/src/dogma/features/repo/roles/ConfirmUpdateRepositoryProjectRoles.tsxwebapp/src/dogma/features/repo/roles/ProjectRolesForm.tsxwebapp/src/pages/app/projects/[projectName]/index.tsxwebapp/src/pages/app/projects/[projectName]/repos/[repoName]/settings/index.tsxwebapp/src/pages/app/projects/[projectName]/settings/index.tsxwebapp/src/pages/app/settings/app-identities/index.tsxwebapp/tests/dogma/feature/repo/RepoList.test.tsxwebapp/tests/dogma/feature/repo/roles/ProjectRolesForm.test.tsxxds/src/main/java/com/linecorp/centraldogma/xds/internal/ControlPlanePlugin.javaxds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsProjectUtil.javaxds/src/main/java/com/linecorp/centraldogma/xds/k8s/v1/XdsKubernetesEndpointFetchingPlugin.javaxds/src/test/java/com/linecorp/centraldogma/xds/internal/ControlPlanePluginTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- server/src/main/java/com/linecorp/centraldogma/server/metadata/RepositoryMetadataTransformer.java
- .gitignore
Comment on lines
+36
to
+42
| final boolean created = !context.projectManager().exists(INTERNAL_PROJECT_XDS); | ||
| context.internalProjectInitializer().initialize(INTERNAL_PROJECT_XDS); | ||
| if (created) { | ||
| // Disallow public repositories by default. An owner can still allow them explicitly and | ||
| // the decision is respected because this runs only when the project is created. | ||
| disallowPublicRepositories(context); | ||
| } |
There was a problem hiding this comment.
Migrate an unset xDS policy atomically, not only newly created projects.
Existing xDS projects skip this branch, so an absent allowPublicRepositories value retains the compatible “allowed” behavior after upgrade. Apply an atomic null → false update on every initialization while preserving explicit true and false owner choices.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@xds/src/main/java/com/linecorp/centraldogma/xds/internal/XdsProjectUtil.java`
around lines 36 - 42, Update the xDS initialization logic in XdsProjectUtil so
the public-repository policy is migrated on every initialization, not only when
INTERNAL_PROJECT_XDS is newly created. Atomically change only an unset
allowPublicRepositories value from null to false, while preserving explicit true
and false owner settings; remove the created-only guard around this migration
and use the existing project policy update mechanism.
ikhoon
force-pushed
the
public-repo
branch
3 times, most recently
from
July 30, 2026 12:49
0326a9f to
bb441af
Compare
…entities Motivation: A repository whose guest role is READ is readable by any signed-in user, but application tokens created after line#1093 are always blocked from guest access and there has been no way to allow it. Users keep asking to let other teams' tokens read guest-open repositories without registering them to the project, and line#622 asks for the private/public repository distinction. Modifications: - Reframe a repository whose guest role is `READ` as a *public* repository. The authorization logic is unchanged: a public repository is readable by every signed-in user and by the app identities whose guest access is allowed. Guests can never write. - Allow choosing guest access when creating a token or a certificate via a new `allowGuestAccess` parameter of `POST /api/v1/appIdentities`. It is disabled by default; a system administrator-level app identity always allows guest access. - Add `allowPublicRepositories` to the project metadata with an owner-only `PUT /api/v1/metadata/{project}/settings`. Disallowing is rejected while the project still has a public repository, and a repository cannot be made public in a project which disallows it. - Accept `"isPublic": true` in `POST /api/v1/projects/{project}/repos` to create a public repository. - Web UI: replace the guest role radio with a Visibility section with confirmation dialogs, show public badges in the repository lists, dim inaccessible repositories with a tooltip, add an 'Allow public repositories' project setting, and add a scope choice to the app identity creation form and a scope column to the app identity list. - Fix a `NullPointerException` when the `guest` field is missing in the `roles/projects` payload and remove the unused `MetadataService.findRepositoryRole(AppIdentity)` overload. - Document public repositories in `auth.rst`. Result: - Closes line#622. - A repository can be switched between public and private. A public repository is readable — never writable — by every signed-in user and by the app identities created with guest access allowed, without granting them any role. - During a rolling upgrade, a project-metadata update served by a not-yet-upgraded replica may drop the new `allowPublicRepositories` field. Re-apply the setting after the whole cluster is upgraded.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation:
A repository whose guest role is READ is readable by any signed-in user, but
application tokens created after #1093 are always blocked from guest access
and there has been no way to allow it. Users keep asking to let other teams'
tokens read guest-open repositories without registering them to the project,
and #622 asks for the private/public repository distinction.
Modifications:
READas a public repository.The authorization logic is unchanged: a public repository is readable by
every signed-in user and by the app identities whose guest access is
allowed. Guests can never write.
allowGuestAccessparameter ofPOST /api/v1/appIdentities. It is disabledby default; a system administrator-level app identity always allows guest
access.
allowPublicRepositoriesto the project metadata with an owner-onlyPUT /api/v1/metadata/{project}/settings. Disallowing isrejected while the project still has a public repository, and a repository
cannot be made public in a project which disallows it.
"isPublic": trueinPOST /api/v1/projects/{project}/reposto createa public repository.
confirmation dialogs, show public badges in the repository lists, dim
inaccessible repositories with a tooltip, add an 'Allow public repositories'
project setting, and add a scope choice to the app identity creation form
and a scope column to the app identity list.
NullPointerExceptionwhen theguestfield is missing in theroles/projectspayload and remove the unusedMetadataService.findRepositoryRole(AppIdentity)overload.auth.rst.Result:
is readable — never writable — by every signed-in user and by the app
identities created with guest access allowed, without granting them any
role.
not-yet-upgraded replica may drop the new
allowPublicRepositoriesfield.Re-apply the setting after the whole cluster is upgraded.