Add secrets CRUD API and template rendering with masking support#1275
Draft
Add secrets CRUD API and template rendering with masking support#1275
Conversation
Motivation: Add support for managing secrets that can be injected into templates. Secrets differ from variables in that their values are masked for security: - list APIs always mask - GET/create/update APIs mask for non-system-admin users. When rendered in templates: - secrets are shown only to app-identity or system-admin users - session-based (web UI) users see "****". Modifications: - Add `SecretServiceV1` with project-level and repo-level CRUD endpoints - Add `Secret` model class with withoutValue() masking support - Update `Templater` to merge secrets into the FreeMarker data model via syntax. - Update `ContentServiceV1` to pass User through template rendering paths - Rename package from 'variable' to 'template' for better organization - Add `SecretServiceV1Test` covering CRUD, masking, and role permissions - Add `SecretTemplateCrudTest` covering template rendering with secrets including session-cookie masking verification Result: - Users can now store secrets at project and repo levels and reference them in templates. TODO: - Add Web UI for managing secrets at the project and repository levels.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a Secret model and SecretServiceV1 (project/repo CRUD, RBAC, masking), moves variable-related classes into an api.template package, updates templating and content services to fetch/mask secrets alongside variables, and adds comprehensive tests and related wiring changes. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant SecretServiceV1
participant Authorizer
participant CrudOperation
Client->>SecretServiceV1: createSecret(project, secret, author, user)
SecretServiceV1->>Authorizer: check permissions
alt allowed
Authorizer-->>SecretServiceV1: allowed
SecretServiceV1->>CrudOperation: persist secret (with name/creation)
CrudOperation-->>SecretServiceV1: HasRevision<Secret>
alt user is admin
SecretServiceV1-->>Client: return secret (real value)
else
SecretServiceV1->>SecretServiceV1: mask value ("****")
SecretServiceV1-->>Client: return secret (masked)
end
else denied
Authorizer-->>SecretServiceV1: denied
SecretServiceV1-->>Client: HTTP 403
end
sequenceDiagram
actor Client
participant ContentServiceV1
participant Templater
participant VariableServiceV1
participant SecretServiceV1
Client->>ContentServiceV1: getFiles(repo, path, user)
ContentServiceV1->>Templater: render(repo, entry, variableFile, revision, user)
par fetch variables and secrets
Templater->>VariableServiceV1: list project/repo variables
VariableServiceV1-->>Templater: List<HasRevision<Variable>>
and
Templater->>SecretServiceV1: list project/repo secrets
SecretServiceV1-->>Templater: List<HasRevision<Secret>>
end
Templater->>Templater: maybeMaskSecret(secret, user)
Templater->>Templater: merge variables + secrets into data model
Templater->>Templater: process template with combined data
Templater-->>ContentServiceV1: rendered Entry
ContentServiceV1-->>Client: HTTP 200 with rendered content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
🧹 Nitpick comments (4)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/template/Secret.java (1)
105-117: Rename local variable fromvariabletosecretfor consistency.The local variable is named
variablebut should besecretto match the class name and improve readability.📝 Proposed fix
`@Override` public boolean equals(Object o) { if (!(o instanceof Secret)) { return false; } - final Secret variable = (Secret) o; - return id.equals(variable.id) && - Objects.equals(name, variable.name) && - value.equals(variable.value) && - Objects.equals(description, variable.description) && - Objects.equals(creation, variable.creation); + final Secret secret = (Secret) o; + return id.equals(secret.id) && + Objects.equals(name, secret.name) && + value.equals(secret.value) && + Objects.equals(description, secret.description) && + Objects.equals(creation, secret.creation); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/java/com/linecorp/centraldogma/server/internal/api/template/Secret.java` around lines 105 - 117, In the Secret.equals method, rename the local variable currently declared as "variable" to "secret" for clarity and consistency with the Secret class; update the cast line and all subsequent uses (the local variable in the equals method) so the code reads: final Secret secret = (Secret) o; and compare against secret instead of variable.server/src/main/java/com/linecorp/centraldogma/server/internal/api/template/package-info.java (1)
17-21: Update package Javadoc to reflect the broader scope.The Javadoc says "An API for managing variables" but this package now also manages secrets and template rendering. Consider updating the description to reflect the expanded scope.
📝 Suggested Javadoc update
/** - * An API for managing variables. + * An API for managing variables, secrets, and template rendering. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/java/com/linecorp/centraldogma/server/internal/api/template/package-info.java` around lines 17 - 21, Update the package Javadoc in package-info.java for package com.linecorp.centraldogma.server.internal.api.template to describe its broader responsibilities (managing variables, secrets, and template rendering) instead of the current "An API for managing variables"; replace the single-line description with a concise summary that mentions variables, secrets, and template rendering and keep the existing annotations (e.g., `@NullMarked`) intact.server/src/test/java/com/linecorp/centraldogma/server/SecretTemplateCrudTest.java (1)
324-358: Cover the app-identity rendering branch too.This test only contrasts a system-admin client with a session cookie. The new masking behavior also has a distinct app-identity path, so a regression that masks app-token renders would still pass here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/test/java/com/linecorp/centraldogma/server/SecretTemplateCrudTest.java` around lines 324 - 358, The test secretsMaskedWhenAccessedViaSessionCookie only verifies token-based and session-cookie rendering paths; add a similar assertion for the app-identity rendering branch to prevent regressions that affect app-token rendering. After pushing the template and before finishing the test, create or obtain an app-identity client (e.g., reuse or implement a helper like createAppIdentityClient or obtain an app token) and fetch the same contents path with renderTemplate=true using that client; then assert the HTTP response is OK and that both "proj" and "repo" are masked as "****" just like the session-based assertions, mirroring the token/session checks around testRepo1.file(...).renderTemplate(true) and sessionClient.get(...).server/src/test/java/com/linecorp/centraldogma/server/internal/api/template/SecretServiceV1Test.java (1)
59-62: Add a non-admin write-capable client to this fixture.Only MEMBER and outsider app tokens are created here, so the contract that create/update responses are masked for non-system-admin users never gets exercised. A non-admin project OWNER / repo WRITER client would close that gap.
Also applies to: 86-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/test/java/com/linecorp/centraldogma/server/internal/api/template/SecretServiceV1Test.java` around lines 59 - 62, Add a non-admin write-capable client to the SecretServiceV1Test fixture by declaring a new BlockingWebClient (e.g., writerClient) alongside memberClient and outsiderClient, then initialize it in the same setup block where the MEMBER and outsider tokens are created by generating a project OWNER or repo WRITER token (use the same token-creation helper used for the MEMBER token) and building a BlockingWebClient with that token; ensure writerClient is used in the tests that exercise create/update responses for non-system-admin users so the masked responses for non-admin writers are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/api/template/Templater.java`:
- Around line 19-21: In Templater.java update the bad import of
VariableServiceV1.varaibleCrudContext to the correctly spelled
VariableServiceV1.variableCrudContext; remove the misspelled import line and
ensure any usages reference variableCrudContext (and not the old typo), keeping
the existing import of SecretServiceV1.secretCrudContext unchanged so all
references compile against VariableServiceV1.variableCrudContext.
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/api/template/VariableServiceV1.java`:
- Around line 258-260: Rename the static method currently named
varaibleCrudContext to variableCrudContext in VariableServiceV1 (so the
signature becomes static CrudContext variableCrudContext(String projectName,
Revision revision)), and update any call sites and imports (notably the import
in Templater.java that references the old name) to use the corrected name;
ensure there are no other references to the misspelled method remaining.
In
`@server/src/test/java/com/linecorp/centraldogma/server/SecretTemplateCrudTest.java`:
- Around line 426-443: The helper createVariable currently constructs JSON by
string interpolation which breaks for ids/values containing quotes or
backslashes; instead instantiate a Variable (or a POJO with id/type/value) and
pass it to the HTTP request serializer (use .contentJson(variable) on the
request) so the body is properly JSON-escaped; keep the same path selection
logic and assert CREATED as before, but replace the manual json string and
.content(MediaType.JSON, json) call with building the Variable object and
calling .contentJson(variable) on the request prepared by httpClient in
createVariable.
---
Nitpick comments:
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/api/template/package-info.java`:
- Around line 17-21: Update the package Javadoc in package-info.java for package
com.linecorp.centraldogma.server.internal.api.template to describe its broader
responsibilities (managing variables, secrets, and template rendering) instead
of the current "An API for managing variables"; replace the single-line
description with a concise summary that mentions variables, secrets, and
template rendering and keep the existing annotations (e.g., `@NullMarked`) intact.
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/api/template/Secret.java`:
- Around line 105-117: In the Secret.equals method, rename the local variable
currently declared as "variable" to "secret" for clarity and consistency with
the Secret class; update the cast line and all subsequent uses (the local
variable in the equals method) so the code reads: final Secret secret = (Secret)
o; and compare against secret instead of variable.
In
`@server/src/test/java/com/linecorp/centraldogma/server/internal/api/template/SecretServiceV1Test.java`:
- Around line 59-62: Add a non-admin write-capable client to the
SecretServiceV1Test fixture by declaring a new BlockingWebClient (e.g.,
writerClient) alongside memberClient and outsiderClient, then initialize it in
the same setup block where the MEMBER and outsider tokens are created by
generating a project OWNER or repo WRITER token (use the same token-creation
helper used for the MEMBER token) and building a BlockingWebClient with that
token; ensure writerClient is used in the tests that exercise create/update
responses for non-system-admin users so the masked responses for non-admin
writers are covered.
In
`@server/src/test/java/com/linecorp/centraldogma/server/SecretTemplateCrudTest.java`:
- Around line 324-358: The test secretsMaskedWhenAccessedViaSessionCookie only
verifies token-based and session-cookie rendering paths; add a similar assertion
for the app-identity rendering branch to prevent regressions that affect
app-token rendering. After pushing the template and before finishing the test,
create or obtain an app-identity client (e.g., reuse or implement a helper like
createAppIdentityClient or obtain an app token) and fetch the same contents path
with renderTemplate=true using that client; then assert the HTTP response is OK
and that both "proj" and "repo" are masked as "****" just like the session-based
assertions, mirroring the token/session checks around
testRepo1.file(...).renderTemplate(true) and sessionClient.get(...).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 322cd3ca-892e-4d4a-986d-f848bf7e3f83
📒 Files selected for processing (13)
server/src/main/java/com/linecorp/centraldogma/server/CentralDogma.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/template/Secret.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/template/SecretServiceV1.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/template/Templater.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/template/Variable.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/template/VariableServiceV1.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/template/VariableType.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/template/package-info.javaserver/src/test/java/com/linecorp/centraldogma/server/SecretTemplateCrudTest.javaserver/src/test/java/com/linecorp/centraldogma/server/VariableTemplateCrudTest.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/api/template/SecretServiceV1Test.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/api/template/VariableServiceV1Test.java
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:
Add support for managing secrets that can be injected into templates. Secrets differ from variables in that their values are masked for security:
When rendered in templates:
****Modifications:
SecretServiceV1with project-level and repo-level CRUD endpointsSecretmodel class withwithoutValue()masking supportTemplaterto merge secrets into the FreeMarker data model via syntaxContentServiceV1to pass User through template rendering pathsSecretServiceV1Testcovering CRUD, masking, and role permissionsSecretTemplateCrudTestcovering template rendering with secrets including session-cookie masking verificationResult:
Users can now store secrets at project and repo levels and reference them in templates.
TODO: