feat: add Personal Access Tokens (PAT) for CLI/API authentication#495
feat: add Personal Access Tokens (PAT) for CLI/API authentication#495rossigee wants to merge 9 commits 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:
📝 WalkthroughWalkthroughThis PR adds Personal Access Token storage, hashing, scoped authorization, REST APIs, authentication middleware, legacy CLI-secret migration, portal management UI, localization, persisted external-auth group memberships, and integration tests. ChangesPAT storage and persistence
PAT creation and scope enforcement
PAT API contract and handlers
PAT hashing and authentication
Legacy CLI secret migration
Persisted external-auth group membership
Portal PAT management
PAT integration scenarios
Sequence Diagram(s)sequenceDiagram
participant Portal
participant UsersAPI
participant PATController
participant PATManager
participant SecurityMiddleware
Portal->>UsersAPI: Create or manage PAT
UsersAPI->>PATController: Validate and process request
PATController->>PATManager: Persist PAT metadata and hash
PATController-->>UsersAPI: Return PAT response or secret
SecurityMiddleware->>PATManager: Validate presented PAT
SecurityMiddleware-->>Portal: Populate scoped security context
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
github-actions
Bot
added
documentation
Ports goharbor#287 for the harbor-next implementation (container-registry/harbor-next#495). Signed-off-by: Ross Golder <ross@golder.org>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (9)
src/pkg/pat/dao/dao_test.go-163-163 (1)
163-163: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix integer-to-string conversion in test data generation.
Using
string(rune(i))converts the integeri(which is 1, 2, or 3) into its corresponding Unicode control character (e.g.,\x01,\x02), resulting in unprintable characters in the token name instead of the intended"token-1","token-2", etc.
src/pkg/pat/dao/dao_test.go#L163-L163: Replace"token-" + string(rune(i))withfmt.Sprintf("token-%d", i)or"token-" + strconv.Itoa(i).src/pkg/pat/dao/dao_test.go#L183-L183: Apply the same fix here to generate correct token names.🤖 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 `@src/pkg/pat/dao/dao_test.go` at line 163, Fix integer-to-string conversion in both token test-data generation sites: src/pkg/pat/dao/dao_test.go lines 163-163 and 183-183. Replace string(rune(i)) in the token name construction with a decimal integer conversion such as fmt.Sprintf or strconv.Itoa so names are generated as token-1, token-2, and token-3.src/controller/pat/controller_test.go-41-46 (1)
41-46: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid hardcoding
user_id1..12 here Harbor seeds the admin account withuser_id = 1, so this range can collide with the default row and breakSetupSuite. Use a non-reserved ID range or let the database assign IDs.🤖 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 `@src/controller/pat/controller_test.go` around lines 41 - 46, Update the test user setup loop in SetupSuite to avoid explicitly inserting user_id values 1 through 12, which can collide with Harbor’s seeded admin account. Use a non-reserved ID range or omit user_id so the database assigns IDs, while preserving the generated usernames, emails, and real names.src/server/middleware/security/pat_test.go-78-82 (1)
78-82: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winVerify token usage timestamp update and security context.
The comment indicates "Verify user context", but the code retrieves a token without asserting any of its fields or the properties of
secCtx. If this code is intended to verify thelast_used_attimestamp update (as mentioned in the PR summary) or the generated security context fields, please add the missing assertions.🤖 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 `@src/server/middleware/security/pat_test.go` around lines 78 - 82, Strengthen the relevant test in pat_test.go by asserting the retrieved token’s last_used_at update and the generated secCtx security-context fields, rather than only retrieving them. Use the existing token and security-context symbols in the test, preserving its current setup while validating the expected values and user context.src/server/middleware/security/pat_test.go-304-308 (1)
304-308: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the returned security context values.
Assigning the return values to the blank identifier (
_ =) does not actually verify the correctness of the generated security context. Please replace these with actual assertions to ensure the context is populated correctly.💚 Proposed fix
- _ = secCtx.GetUsername() - _ = secCtx.IsAuthenticated() - _ = secCtx.IsSysAdmin() - _ = secCtx.IsSolutionUser() - _ = secCtx.Name() + suite.Equal("scopeuser", secCtx.GetUsername()) + suite.True(secCtx.IsAuthenticated()) + suite.False(secCtx.IsSysAdmin()) + suite.False(secCtx.IsSolutionUser()) + suite.Equal("pat", secCtx.Name())🤖 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 `@src/server/middleware/security/pat_test.go` around lines 304 - 308, Update the test around the security-context creation call in pat_test.go to capture its returned values in named variables instead of blank identifiers, then assert each value matches the expected populated security context. Preserve the existing test setup and verify all returned context fields relevant to the generated result.src/server/v2.0/handler/user.go-75-80 (1)
75-80: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMissing nil checks for request bodies.
These endpoints directly dereference
params.Request(andparams.Request.Name). While framework-level OpenAPI validation often catches missing payloads, relying exclusively on it without programmatic nil checks poses a persistent crash risk (nil pointer dereference) if validation rules change.Defensively verify that the request payload is not nil before accessing its properties:
src/server/v2.0/handler/user.go#L75-L80: InCreatePersonalAccessToken, addif params.Request == nil || params.Request.Name == nil { return u.SendError(ctx, errors.BadRequestError(nil).WithMessage("bad request")) }.src/server/v2.0/handler/user.go#L204-L205: InRefreshPersonalAccessTokenSecret, checkif params.Request == nil { return u.SendError(...) }.src/server/v2.0/handler/user.go#L228-L237: InUpdatePersonalAccessToken, checkif params.Request == nil { return u.SendError(...) }.🤖 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 `@src/server/v2.0/handler/user.go` around lines 75 - 80, Add defensive request-body nil checks before dereferencing parameters: in src/server/v2.0/handler/user.go lines 75-80 within CreatePersonalAccessToken, reject nil Request or Name; in lines 204-205 within RefreshPersonalAccessTokenSecret and lines 228-237 within UpdatePersonalAccessToken, reject nil Request. Each validation should return the existing bad-request SendError response.src/server/v2.0/handler/user.go-670-686 (1)
670-686: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInclude owner information in the audit description.
The 4th parameter
_ stringis ignored, despite being populated withfmt.Sprintf("user:%d", userID)by all callers. To ensure accurate and unambiguous audit trails (especially when an administrator manipulates another user's PAT), include this information in theOperationDescription.📝 Proposed fix
-func (u *usersAPI) logAuditEntry(ctx context.Context, operation, patName, _ string, isSuccessful bool) { +func (u *usersAPI) logAuditEntry(ctx context.Context, operation, patName, ownerInfo string, isSuccessful bool) { sctx, _ := security.FromContext(ctx) username := "unknown" if sctx != nil { username = sctx.GetUsername() } auditLog := &auditmodel.AuditLogExt{ Operation: operation, ResourceType: "PAT", Resource: patName, Username: username, OpTime: time.Now(), - OperationDescription: fmt.Sprintf("%s personal access token %s", operation, patName), + OperationDescription: fmt.Sprintf("%s personal access token %s for %s", operation, patName, ownerInfo), IsSuccessful: isSuccessful, }🤖 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 `@src/server/v2.0/handler/user.go` around lines 670 - 686, Update the affected handler method in user.go to use its currently ignored fourth string parameter when constructing OperationDescription, including the supplied owner value such as “user:<id>”. Preserve the existing audit description content while appending or incorporating this owner information so administrator actions on another user’s PAT are unambiguous.src/portal/src/app/base/account-settings/account-settings-modal.component.html-469-469 (1)
469-469: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUntranslated placeholder string.
placeholder="0 for never expire"is hardcoded English while every other user-facing string in this template uses| translate. This breaks localization for non-English users.🌐 Proposed fix
[(ngModel)]="newPATForm.expiresInDays" min="0" - placeholder="0 for never expire" /> + [placeholder]="'PROFILE.PAT_EXPIRES_PLACEHOLDER' | translate" />Add the corresponding key to the i18n resource files.
🤖 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 `@src/portal/src/app/base/account-settings/account-settings-modal.component.html` at line 469, Replace the hardcoded placeholder in the account settings modal with a translation key using the template’s existing translate pattern, then add the corresponding key and “0 for never expire” value to the project’s i18n resource files for all supported locales.src/portal/src/app/base/left-side-nav/system-robot-accounts/new-robot/new-robot.component.ts-117-117 (1)
117-117: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the Prettier formatting error.
The single-line object type violates the repository’s formatting check and currently fails UI lint. Format its properties across multiple lines.
Proposed fix
- secretValidation: { length: boolean; uppercase: boolean; lowercase: boolean; digit: boolean } = { + secretValidation: { + length: boolean; + uppercase: boolean; + lowercase: boolean; + digit: boolean; + } = {🤖 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 `@src/portal/src/app/base/left-side-nav/system-robot-accounts/new-robot/new-robot.component.ts` at line 117, Update the single-line object type in the new robot component’s type declaration to place its properties on separate lines, matching the repository’s Prettier formatting rules and allowing the UI lint check to pass.Source: Pipeline failures
src/portal/src/app/base/project/robot-account/add-robot/add-robot.component.ts-86-86 (1)
86-86: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the Prettier formatting violation.
The inline
secretValidationtype fails the UI lint pipeline. Format the properties across multiple lines.Proposed fix
- secretValidation: { length: boolean; uppercase: boolean; lowercase: boolean; digit: boolean } = { + secretValidation: { + length: boolean; + uppercase: boolean; + lowercase: boolean; + digit: boolean; + } = {🤖 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 `@src/portal/src/app/base/project/robot-account/add-robot/add-robot.component.ts` at line 86, Reformat the inline secretValidation type in the add-robot component by placing its properties on separate lines, preserving the existing type structure and behavior so the UI lint pipeline passes.Source: Pipeline failures
🧹 Nitpick comments (25)
docs/OAUTH2_TOKEN_SUPPORT.md (1)
153-173: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winExpand the authentication test matrix.
Add coverage for expired, malformed, wrong-issuer, wrong-audience, invalid-signature, unknown-user, and path-restricted tokens, plus the compatibility case where
AuthMode == OIDCAuthand the new flag is disabled.🤖 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 `@docs/OAUTH2_TOKEN_SUPPORT.md` around lines 153 - 173, The Testing Strategy section should expand its authentication matrix to cover expired, malformed, wrong-issuer, wrong-audience, invalid-signature, unknown-user, and path-restricted Bearer tokens, including their expected rejection behavior. Add the compatibility case where AuthMode equals OIDCAuth while the new toggle is disabled, and preserve the existing flag/AuthMode scenarios.make/migrations/postgresql/0200_2.17.0_schema.up.sql (1)
19-19: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueEvaluate the usefulness of an index on a boolean column.
The
disabledcolumn is a boolean, which has very low cardinality (true/false). PostgreSQL's query optimizer typically ignores indexes on low-cardinality columns in favor of sequential scans, unless one value is extremely rare and the index is heavily used to filter for that rare value. Consider removing this index unless it significantly improves performance for specific query patterns.♻️ Proposed refactor
-CREATE INDEX IF NOT EXISTS idx_pat_disabled ON personal_access_token (disabled);🤖 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 `@make/migrations/postgresql/0200_2.17.0_schema.up.sql` at line 19, Evaluate the usefulness of idx_pat_disabled on the boolean personal_access_token.disabled column and remove the index definition unless query patterns and benchmarking demonstrate a meaningful performance benefit. Keep other migration changes unchanged.src/pkg/pat/dao/dao_test.go (1)
82-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the generated ID for retrieval instead of a hardcoded value.
Setting a hardcoded
IDon the model before insertion can be misleading, as the database typically generates the primary key, and the ORM modifies the struct or returns the new ID. It is safer and more idiomatic to explicitly use the returnedidfor subsequent queries.♻️ Proposed refactor
pat := &model.PersonalAccessToken{ - ID: 1, UserID: 3, Name: "get-test", Secret: "secret", Salt: "salt", } // Create first - _, err := suite.dao.Create(suite.Context(), pat) + id, err := suite.dao.Create(suite.Context(), pat) suite.NoError(err) // Get - retrieved, err := suite.dao.Get(suite.Context(), pat.ID) + retrieved, err := suite.dao.Get(suite.Context(), id)🤖 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 `@src/pkg/pat/dao/dao_test.go` around lines 82 - 95, Update the Create/Get flow in the test to capture the ID returned by suite.dao.Create, then pass that generated ID to suite.dao.Get instead of relying on pat.ID or a hardcoded model ID. Remove the preassigned ID if it is no longer needed.src/pkg/pat/migration/migrate_cli_secrets.go (1)
80-87: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider optimizing to avoid N+1 queries.
Calling
patMgr.Listinside the loop executes a database query for every OIDC user. If the instance has thousands of OIDC users, this can cause significant transaction delays.If the
personal_access_tokentable has a unique constraint on(user_id, name), you can skip thisListcheck entirely and simply handle/ignore the duplicate constraint violation error returned bypatMgr.Create. Alternatively, pre-fetch existingcli-secretPATs in a single query before the loop.🤖 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 `@src/pkg/pat/migration/migrate_cli_secrets.go` around lines 80 - 87, Remove the per-user patMgr.List call from the OIDC-user migration loop to avoid N+1 database queries. Pre-fetch existing cli-secret personal access tokens once before the loop, index them by user ID, and use that lookup to skip users who already have one before calling patMgr.Create.src/pkg/pat/migration/migrate_cli_secrets_test.go (2)
148-170: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
fmt.Sprintfover rune arithmetic for string formatting.Using
string(rune(48+i))for integer-to-string conversion is unidiomatic and harder to read. It's recommended to usefmt.Sprintffor constructing these test strings. Remember to add"fmt"to your imports if you apply this change.♻️ Proposed refactor
// Create multiple test users for i := 1; i <= 3; i++ { u := &models.User{ - Username: "user" + string(rune(48+i)), - Email: "user" + string(rune(48+i)) + "`@example.com`", - Realname: "User " + string(rune(48+i)), + Username: fmt.Sprintf("user%d", i), + Email: fmt.Sprintf("user%d@example.com", i), + Realname: fmt.Sprintf("User %d", i), } uid, err := suite.userCtl.Create(ctx, u) suite.NoError(err) // Create OIDC secret for each user - plainSecret := "secret-" + string(rune(48+i)) + plainSecret := fmt.Sprintf("secret-%d", i) secretKey, err := config.SecretKey() suite.NoError(err) encryptedSecret, err := utils.ReversibleEncrypt(plainSecret, secretKey) suite.NoError(err) oidcDAO := oidcdao.NewMetaDao() oidcU := &models.OIDCUser{ UserID: int(uid), Secret: encryptedSecret, - SubIss: "sub" + string(rune(48+i)) + "|issuer", - Token: "token" + string(rune(48+i)), + SubIss: fmt.Sprintf("sub%d|issuer", i), + Token: fmt.Sprintf("token%d", i), } _, err = oidcDAO.Create(ctx, oidcU)🤖 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 `@src/pkg/pat/migration/migrate_cli_secrets_test.go` around lines 148 - 170, Replace the rune-arithmetic integer-to-string construction in the affected test with fmt.Sprintf, adding the fmt import and preserving the existing generated string values.
83-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
suite.Require().Lento prevent test panics on failure.
suite.Equaldoes not halt the test upon failure. If the queried slice is empty during a failing test run, the test will proceed and panic with an "index out of range" error when accessing the first element. UsingRequire()ensures the test aborts cleanly if the length check fails.
src/pkg/pat/migration/migrate_cli_secrets_test.go#L83-L85: Changesuite.Equal(1, len(pats))tosuite.Require().Len(pats, 1)to protectpats[0].src/pkg/pat/migration/migrate_cli_secrets_test.go#L131-L132: Changesuite.Equal(1, len(pats1))tosuite.Require().Len(pats1, 1)to protect downstream indexing ofpats1.src/pkg/pat/migration/migrate_cli_secrets_test.go#L140-L141: Changesuite.Equal(1, len(pats2))tosuite.Require().Len(pats2, 1)to protectpats2[0].🤖 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 `@src/pkg/pat/migration/migrate_cli_secrets_test.go` around lines 83 - 85, Replace the non-fatal length assertions protecting indexed PAT slices with require-style assertions: in src/pkg/pat/migration/migrate_cli_secrets_test.go at lines 83-85, 131-132, and 140-141, update the checks for pats, pats1, and pats2 to use suite.Require().Len with expected length 1 before their downstream indexing.src/common/security/local/pat_context.go (2)
110-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead code:
init()only exists to reference otherwise-unusedrbacconstants.
_ = rbac.ActionPull, etc. serve no functional purpose here —HasPermissionmatches purely on strings (resourceStr/actionStr), never touching these constants. This reads as leftover debugging/import-satisfying code.♻️ Proposed fix: remove dead init()
- -func init() { - _ = rbac.ActionPull - _ = rbac.ActionPush - _ = rbac.ActionDelete -}(and drop the now-unused
rbacimport if nothing else in the file needs it)🤖 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 `@src/common/security/local/pat_context.go` around lines 110 - 114, Remove the dead init function that only references rbac.ActionPull, rbac.ActionPush, and rbac.ActionDelete, and remove the rbac import if no other code in the file uses it. Leave HasPermission and its string-based permission matching unchanged.
36-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winScope parse failure is silently swallowed.
json.Unmarshalerrors are discarded (_ = json.Unmarshal(...)), so a malformedscopestring just results in an empty (fail-closed) scope with no diagnostic trail. Given this sits on the authentication/authorization hot path, logging the parse failure would materially help debug misconfigured or corrupted PAT scope data.♻️ Proposed fix: log unmarshal failures
var parsedScope []patmodel.ProjectScope if scope != "" { - _ = json.Unmarshal([]byte(scope), &parsedScope) + if err := json.Unmarshal([]byte(scope), &parsedScope); err != nil { + log.Warningf("failed to parse PAT scope for user %v: %v", user, err) + } }🤖 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 `@src/common/security/local/pat_context.go` around lines 36 - 45, Update NewPATSecurityContext to capture the error returned by json.Unmarshal when parsing a non-empty scope and log the failure with the relevant diagnostic context instead of discarding it. Preserve the existing parsedScope behavior and fail-closed result for malformed scope data.src/server/middleware/security/pat_test.go (1)
270-272: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the test suite runner to the end of the file.
Conventionally, the test suite runner function (
TestPATSecurityTestSuite) is placed at the very end of the file, after all the suite methods have been defined.🤖 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 `@src/server/middleware/security/pat_test.go` around lines 270 - 272, Move the TestPATSecurityTestSuite function to the end of the file, after all PAT security test suite methods, without changing its behavior or the surrounding tests.api/v2.0/swagger.yaml (1)
6218-6222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a
400 Bad Requestresponse for invalid payloads.The PATCH endpoint accepts a body (
PersonalAccessTokenRefreshRequest). If the client provides malformed JSON or an invalid secret format, the server will likely return a400 Bad Request. Documenting this response ensures API clients can handle validation errors properly.♻️ Proposed refactor
'200': description: Personal access token secret refreshed successfully. schema: $ref: '`#/definitions/PersonalAccessTokenCreatedResponse`' + '400': + $ref: '`#/responses/400`' '401': $ref: '`#/responses/401`'🤖 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 `@api/v2.0/swagger.yaml` around lines 6218 - 6222, Update the PATCH endpoint response definitions near the existing 200 and 401 responses to include a 400 Bad Request response for malformed JSON or invalid PersonalAccessTokenRefreshRequest payloads, with a clear description and the appropriate error schema used by the API.src/server/v2.0/handler/pat_test.go (1)
54-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
Require().NoErrorto fail fast on setup errors.If user creation fails, continuing execution will cause
uidto be 0, leading to confusing downstream test failures.Require().NoError(err)stops the test execution immediately. Consider applying this pattern throughout the test suite wherever subsequent assertions depend on the success of an operation.♻️ Proposed refactor
- suite.NoError(err) + suite.Require().NoError(err)🤖 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 `@src/server/v2.0/handler/pat_test.go` at line 54, Update the user-creation setup in the affected test to use the test suite’s Require().NoError(err) assertion instead of a non-fatal error check, so execution stops before using an invalid uid. Apply the same fail-fast pattern to nearby setup operations whose results are required by subsequent assertions.src/portal/src/app/base/account-settings/account-settings-modal.component.html (2)
401-431: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
@elsefor the mutually exclusive PAT states.Lines 401 (
@if (createdPATSecret)) and 431 (@if (!createdPATSecret)) express one boolean branch as two separate@ifblocks that each re-evaluate the condition. The footer (Line 566) shares the same pattern. Collapsing these into@if (createdPATSecret) { ... }@else{ ... }is clearer and avoids duplicate condition evaluation.🤖 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 `@src/portal/src/app/base/account-settings/account-settings-modal.component.html` around lines 401 - 431, In the account settings modal template, replace the mutually exclusive PAT-state `@if (createdPATSecret)` and `@if (!createdPATSecret)` blocks with a single `@if (createdPATSecret) { ... } `@else` { ... }` structure. Apply the same conversion to the footer’s corresponding PAT-state conditions, preserving each branch’s existing content.
525-543: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrack by a stable key and add accessible labels to scope checkboxes.
Two suggestions for the scope table:
track ptracks by object identity; prefertrack p.project_idso@fordiffing stays stable and cheap.- The pull/push checkboxes (Lines 529-540) have no associated
<label>oraria-label, so screen readers announce them without project/action context.♻️ Proposed changes
- `@for` (p of scopeProjects; track p) { + `@for` (p of scopeProjects; track p.project_id) { <tr> <td>{{ p.project_name }}</td> <td style="text-align: center"> <input clrCheckbox type="checkbox" [(ngModel)]="p.pull" - [name]="'pull_' + p.project_id" /> + [name]="'pull_' + p.project_id" + [attr.aria-label]="('PROFILE.PAT_SCOPE_PULL' | translate) + ' ' + p.project_name" /> </td> <td style="text-align: center"> <input clrCheckbox type="checkbox" [(ngModel)]="p.push" - [name]="'push_' + p.project_id" /> + [name]="'push_' + p.project_id" + [attr.aria-label]="('PROFILE.PAT_SCOPE_PUSH' | translate) + ' ' + p.project_name" /> </td> </tr> }🤖 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 `@src/portal/src/app/base/account-settings/account-settings-modal.component.html` around lines 525 - 543, Update the scope table’s `@for` block to track each project by `p.project_id` instead of object identity, and add accessible labels to the pull and push checkboxes so each conveys both the project and action context, using associated labels or appropriate `aria-label` attributes.src/portal/src/app/base/account-settings/account-settings-modal.component.ts (3)
653-657: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueHandle clipboard write failure.
navigator.clipboard.writeTextreturns a promise that can reject (e.g. permission denied, insecure context, ornavigator.clipboardbeingundefinedover plain HTTP). Currently a failed copy is silent, leaving the user unaware. Consider adding a.then/.catchto surface success/failure feedback.🤖 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 `@src/portal/src/app/base/account-settings/account-settings-modal.component.ts` around lines 653 - 657, Update the clipboard-copy flow in account-settings-modal.component.ts to handle navigator.clipboard.writeText failures, including unavailable clipboard APIs and rejected promises. Surface clear success feedback on completion and failure feedback when the write cannot be performed, while preserving the existing copy behavior.
753-762: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueFragile fallback in
confirmAction.Any confirmed message whose source is not
USER_PATfalls through toconfirmGenerate()and generates a new CLI secret. If this shared confirmation dialog ever handles another target in this component, an unrelated confirmation would unexpectedly regenerate the CLI secret. Prefer matching the generate target explicitly rather than using it as the catch-allelse.🤖 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 `@src/portal/src/app/base/account-settings/account-settings-modal.component.ts` around lines 753 - 762, Update confirmAction so confirmGenerate is invoked only when the confirmed message explicitly targets CLI secret generation, rather than using a non-USER_PAT source as the fallback condition. Ensure confirmations for unrelated targets do not generate a new secret, while preserving the existing USER_PAT behavior.
91-99: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueEmpty
scopeProjectsmakes both getterstrue.
Array.prototype.everyreturnstrueon an empty array, so when no projects are loaded bothallScopeSelectedandnoScopeSelectedreporttrue. This is mostly benign here becausebuildScopeJsontreats "all selected" as full auto-compute, but any template logic that relies on these two being mutually exclusive will misbehave for users with zero projects. Consider short-circuiting onscopeProjects.length === 0.🤖 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 `@src/portal/src/app/base/account-settings/account-settings-modal.component.ts` around lines 91 - 99, Update the allScopeSelected and noScopeSelected getters to explicitly handle an empty scopeProjects array before calling every. Return a mutually exclusive, appropriate false state when no projects are loaded, while preserving the existing selection checks for non-empty project lists.src/portal/src/app/base/account-settings/api-tokens-modal.component.html (4)
20-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove row selection if batch actions are unsupported.
Binding
[(clrDgSelected)]="selectedTokens"automatically adds row selection checkboxes to the datagrid. Since there are no batch actions (e.g., "Delete Selected") in the token toolbar, these checkboxes serve no purpose and can confuse users.If bulk actions are not planned, remove this binding to hide the checkboxes.
♻️ Proposed refactor
- <clr-datagrid - [(clrDgSelected)]="selectedTokens" - [clrDgLoading]="tokenLoading"> + <clr-datagrid + [clrDgLoading]="tokenLoading">🤖 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 `@src/portal/src/app/base/account-settings/api-tokens-modal.component.html` around lines 20 - 22, Remove the [(clrDgSelected)]="selectedTokens" binding from the token datagrid in the account tokens modal so row-selection checkboxes are not rendered when no batch actions are available.
129-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImprove form accessibility by handling submission natively.
The form relies exclusively on a button click event to trigger submission. This breaks native HTML accessibility, preventing users from submitting the form by pressing the
Enterkey.Bind the
(ngSubmit)event directly to the<form>element. Additionally, update the "Create" button (line 205) totype="submit"and remove its(click)binding so it seamlessly hooks into the form.♻️ Proposed form tag update
- <form> + <form (ngSubmit)="createToken()"> <clr-input-container>🤖 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 `@src/portal/src/app/base/account-settings/api-tokens-modal.component.html` around lines 129 - 139, Update the form in account-settings/api-tokens-modal.component.html to bind its (ngSubmit) event to the existing submission handler, allowing native Enter-key submission. Change the “Create” button to type="submit" and remove its (click) binding so submission is handled exclusively by the form.
126-131: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStandardize translation mechanisms.
This template mixes Angular's native
i18n="@@..."attributes (used here on lines 126 and 131) with@ngx-translate'stranslatepipe (used elsewhere in this file, such as lines 16 and 25).To simplify translation extraction and maintenance, it is recommended to standardize on a single translation mechanism—typically the
translatepipe across Harbor UI components.🤖 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 `@src/portal/src/app/base/account-settings/api-tokens-modal.component.html` around lines 126 - 131, Standardize the translation approach in the account-tokens modal template by replacing the native Angular i18n attributes on the affected elements with the existing `@ngx-translate` translate pipe pattern used elsewhere in the template. Preserve the current translation keys and rendered text while ensuring all translatable content uses the pipe consistently.
132-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDisable inputs when the token secret is displayed.
When
createdTokenSecretis populated, the modal displays the newly generated token secret. However, the Name and Description inputs remain editable, which might lead users to believe they can still update the newly created token's details.Consider disabling these inputs once the token is created to clarify the state.
♻️ Proposed refactor
<input clrInput type="text" placeholder="Enter token name" [(ngModel)]="newTokenForm.name" name="token_name" + [disabled]="!!createdTokenSecret" required /> </clr-input-container> <clr-textarea-container> <label i18n="@@PROFILE.DESCRIPTION">Description</label> <textarea clrTextarea placeholder="Enter token description (optional)" [(ngModel)]="newTokenForm.description" + [disabled]="!!createdTokenSecret" name="token_description"></textarea> </clr-textarea-container>🤖 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 `@src/portal/src/app/base/account-settings/api-tokens-modal.component.html` around lines 132 - 148, Update the Name and Description inputs in the account token modal to be disabled whenever createdTokenSecret is populated, while keeping them editable before token creation. Preserve the existing token-secret display behavior and bindings.tests/robot-cases/Group1-Nightly/PAT.robot (3)
234-234: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
[Return]setting is deprecated.Robot Framework deprecated
[Return]in favor of theRETURNstatement starting in RF 5.0, with visible deprecation warnings since RF 7.0.Also applies to: 245-245, 270-270
🤖 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 `@tests/robot-cases/Group1-Nightly/PAT.robot` at line 234, Replace the deprecated [Return] settings in the affected Robot Framework keywords with the modern RETURN statement. Update all occurrences at the referenced locations while preserving each keyword’s existing return values and control flow.
207-211: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest Case 12 is a no-op that always passes.
The body only logs messages and performs no API calls or assertions, so it provides no actual coverage of OIDC auto-onboarding despite the documentation claim. Consider tagging it
[Skip]/excluding it from the suite (e.g. via aSkipkeyword or exclusion tag) rather than leaving a test case that trivially "passes" with zero verification.🤖 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 `@tests/robot-cases/Group1-Nightly/PAT.robot` around lines 207 - 211, Update the Test Case 12 definition in PAT.robot to exclude it from execution, using the suite’s established Skip keyword or exclusion tag mechanism. Do not leave the no-op logging body as an active test that reports success without API calls or assertions.
102-103: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHardcoded project names lack the uniqueness suffix used elsewhere.
test-project-docker,scoped-project-1, andscoped-project-2are static, unlike other tests in this suite that append${d}to avoid collisions across repeated/parallel runs. If a prior run's cleanup (Lines 124, 203-204) fails or is skipped, subsequent runs will collide onCreate Project.Also applies to: 179-183
🤖 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 `@tests/robot-cases/Group1-Nightly/PAT.robot` around lines 102 - 103, Update the project names used by the affected tests, including the Create Project flow and the references around the cleanup sections, to append the existing ${d} uniqueness suffix. Apply it consistently to test-project-docker, scoped-project-1, and scoped-project-2 while preserving matching references within each test.src/portal/src/app/base/account-settings/api-tokens-modal.component.ts (1)
100-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace deprecated
document.execCommand('copy').The
document.execCommandAPI is deprecated. Consider using the modern Clipboard API (navigator.clipboard.writeText) or Angular CDK's clipboard service if it is already available in your dependencies.♻️ Proposed fix using Clipboard API
copyTokenSecret(): void { - const copyInput = document.createElement('textarea'); - copyInput.value = this.createdTokenSecret; - document.body.appendChild(copyInput); - copyInput.select(); - document.execCommand('copy'); - document.body.removeChild(copyInput); - this.msgHandler.showSuccess('Token copied to clipboard'); + if (navigator.clipboard) { + navigator.clipboard.writeText(this.createdTokenSecret).then(() => { + this.msgHandler.showSuccess('Token copied to clipboard'); + }).catch(() => { + this.msgHandler.showError('Failed to copy token'); + }); + } }🤖 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 `@src/portal/src/app/base/account-settings/api-tokens-modal.component.ts` around lines 100 - 108, Replace the deprecated document.execCommand('copy') usage in the API token copy flow with navigator.clipboard.writeText, or reuse Angular CDK’s clipboard service if already available. Preserve the existing copied-content and success/error behavior while handling the asynchronous clipboard operation.src/portal/src/app/shared/components/navigator/navigator.component.html (1)
89-94: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAvoid the
javascript:URL.This item only opens a modal, so use a button-style dropdown item or another non-script URL.
javascript:void(0)can violate CSP policies and is unnecessary when Angular already handles the click.🤖 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 `@src/portal/src/app/shared/components/navigator/navigator.component.html` around lines 89 - 94, Replace the navigator dropdown item’s javascript: URL with a button-style dropdown item or another non-script target while preserving the existing Angular click handler and modal behavior. Remove the unnecessary javascript:void(0) usage from the affected template element.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d56fd8ef-ea94-46d3-847f-700437a90910
📒 Files selected for processing (41)
api/v2.0/swagger.yamldocs/OAUTH2_TOKEN_SUPPORT.mdmake/migrations/postgresql/0200_2.17.0_schema.up.sqlmake/migrations/postgresql/0201_2.17.0_pat_scope.down.sqlmake/migrations/postgresql/0201_2.17.0_pat_scope.up.sqlmake/photon/core/harbor_coremake/photon/core/harbor_core_newsrc/common/security/local/pat_context.gosrc/common/security/local/pat_context_test.gosrc/controller/pat/controller.gosrc/controller/pat/controller_test.gosrc/controller/user/controller_link_test.gosrc/pkg/pat/dao/dao.gosrc/pkg/pat/dao/dao_test.gosrc/pkg/pat/manager.gosrc/pkg/pat/migration/migrate_cli_secrets.gosrc/pkg/pat/migration/migrate_cli_secrets_test.gosrc/pkg/pat/model/model.gosrc/pkg/pat/model/model_test.gosrc/portal/src/app/base/account-settings/account-settings-modal.component.htmlsrc/portal/src/app/base/account-settings/account-settings-modal.component.spec.tssrc/portal/src/app/base/account-settings/account-settings-modal.component.tssrc/portal/src/app/base/account-settings/api-tokens-modal.component.htmlsrc/portal/src/app/base/account-settings/api-tokens-modal.component.scsssrc/portal/src/app/base/account-settings/api-tokens-modal.component.tssrc/portal/src/app/base/base.module.tssrc/portal/src/app/base/harbor-shell/harbor-shell.component.htmlsrc/portal/src/app/base/harbor-shell/harbor-shell.component.spec.tssrc/portal/src/app/base/harbor-shell/harbor-shell.component.tssrc/portal/src/app/base/left-side-nav/system-robot-accounts/new-robot/new-robot.component.tssrc/portal/src/app/base/modal-events.const.tssrc/portal/src/app/base/project/robot-account/add-robot/add-robot.component.tssrc/portal/src/app/shared/components/navigator/navigator.component.htmlsrc/portal/src/app/shared/components/navigator/navigator.component.tssrc/portal/src/app/shared/entities/shared.const.tssrc/server/middleware/security/pat.gosrc/server/middleware/security/pat_test.gosrc/server/v2.0/handler/pat.gosrc/server/v2.0/handler/pat_test.gosrc/server/v2.0/handler/user.gotests/robot-cases/Group1-Nightly/PAT.robot
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 41 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 20 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
github-actions
Bot
removed
the
documentation
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 19 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pkg/pat/migration/migrate_cli_secrets.go (2)
79-90: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake the idempotency check atomic.
The list-then-create sequence is vulnerable to a TOCTOU race. Because
src/core/main.goinvokes this migration during every core startup, concurrent instances can both observe no PAT and create duplicates. Enforce a database uniqueness constraint and use an insert-on-conflict/upsert path, treating duplicate-key errors as skips.Also applies to: 116-120
🤖 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 `@src/pkg/pat/migration/migrate_cli_secrets.go` around lines 79 - 90, Make the cli-secret migration idempotent under concurrent startups by adding a database uniqueness constraint for the user and PAT name, then replacing the list-then-create flow around patMgr.List with an insert-on-conflict or upsert operation. Treat duplicate-key conflicts as skipped users, while preserving existing handling for other errors and successful creations; apply the same change to the corresponding later creation path.
79-90: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReserve
cli-secretend-to-end.IsLegacyalone won’t disambiguate here:personal_access_tokenis still unique on(user_id, name), and there’s no PAT-name reservation/validation elsewhere, so a pre-existing user PAT with this name will skip the migration and leave that user’s OIDC secret unmigrated. Add a collision test for a non-legacycli-secretPAT.🤖 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 `@src/pkg/pat/migration/migrate_cli_secrets.go` around lines 79 - 90, The migration’s existing-name check incorrectly treats any user PAT named “cli-secret” as the reserved migration token, allowing a non-legacy token to block OIDC secret migration. Update the migration logic and PAT name validation/reservation path around the migration entry point and PAT creation symbols so “cli-secret” is reserved end-to-end, while distinguishing or handling non-legacy collisions explicitly. Add a test covering a user with a non-legacy “cli-secret” PAT and verify the OIDC secret is still migrated as required.
🧹 Nitpick comments (4)
src/common/security/local/pat_context.go (1)
36-42: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard against nil pointer dereference on
user.While the current middleware flow guarantees
useris not nil,NewPATSecurityContextis an exported constructor. If it is ever called with aniluser (e.g., in unit tests or future authentication flows),user.Usernamewill panic when scope parsing fails. Consider adding a defensive check.🛡️ Proposed defensive fix
func NewPATSecurityContext(user *models.User, scope string) *PATSecurityContext { var parsedScope []patmodel.ProjectScope if scope != "" { if err := json.Unmarshal([]byte(scope), &parsedScope); err != nil { - log.Warningf("failed to parse PAT scope for user %v: %v", user.Username, err) + username := "unknown" + if user != nil { + username = user.Username + } + log.Warningf("failed to parse PAT scope for user %v: %v", username, err) } }🤖 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 `@src/common/security/local/pat_context.go` around lines 36 - 42, Update NewPATSecurityContext to safely handle a nil user when JSON scope parsing fails; avoid dereferencing user.Username unless user is non-nil, while preserving the existing warning behavior and parsed-scope handling for valid users.src/portal/src/app/base/account-settings/api-tokens-modal.component.ts (1)
267-275: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
document.execCommand('copy')is deprecated; prefer the Clipboard API.Modern browsers support
navigator.clipboard.writeText(), which is the recommended replacement for the deprecatedexecCommand('copy')approach used here and incopyRefreshedTokenSecret/copyTokenSecret.🤖 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 `@src/portal/src/app/base/account-settings/api-tokens-modal.component.ts` around lines 267 - 275, Update copySecretToClipboard and the related copyRefreshedTokenSecret and copyTokenSecret flows to use navigator.clipboard.writeText() instead of the deprecated document.execCommand('copy') textarea approach. Preserve the existing success notification and ensure the asynchronous clipboard operation is handled correctly.src/portal/src/app/base/account-settings/api-tokens-modal.component.html (2)
167-235: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer CSS classes over inline styles for the new scope-selection UI.
This block relies entirely on inline
style="..."attributes (margin, font-size, color, width) instead of classes in the siblingapi-tokens-modal.component.scss, which was added specifically for this 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 `@src/portal/src/app/base/account-settings/api-tokens-modal.component.html` around lines 167 - 235, The new scope-selection markup in the scope-section block should use CSS classes instead of inline style attributes. Add or reuse descriptive classes in api-tokens-modal.component.scss for spacing, typography, colors, table column widths, and alignment, then replace the corresponding inline styles while preserving the current layout and appearance.
92-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRefreshing a secret is destructive but has no confirmation step.
Clicking "Refresh" immediately invalidates the old secret (any script/CI using it breaks) with no confirmation dialog, unlike typical destructive-action UX patterns elsewhere in Harbor.
🤖 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 `@src/portal/src/app/base/account-settings/api-tokens-modal.component.html` around lines 92 - 96, Add a confirmation step to the refresh action wired through refreshTokenSecret before invalidating the existing token secret, using the application’s established destructive-action confirmation pattern and proceeding only when the user confirms; keep the existing refresh behavior unchanged after confirmation.
🤖 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 `@src/common/utils/encrypt.go`:
- Around line 111-127: Update VerifyPATHash to split the versioned hash into and
validate exactly the expected number of segments before indexing, returning
false when the salt/hash separator is missing or extra segments are present.
Handle strconv.Atoi errors and reject non-positive iteration counts before
calling pbkdf2.Key, while preserving the existing legacy-hash verification path.
- Around line 96-109: Restore Encrypt’s legacy plain-hex output for existing
password checks and the PAT fallback path instead of delegating to
EncryptWithIterations. Keep the prefixed format in EncryptWithIterations, and
update only callers that require the PAT format to invoke EncryptWithIterations
explicitly.
In `@src/core/main.go`:
- Around line 294-297: Update the startup migration flow around
patmigration.MigrateCliSecretsToLegacyPATs so global migration failures are
propagated as startup failures or trigger a retry instead of being logged with
log.Warningf and ignored. Preserve the nonfatal handling of per-user migration
errors, while ensuring failures obtaining the ORM, querying oidc_user, or
loading the secret key prevent the migration from being treated as complete.
In `@src/portal/src/app/base/account-settings/api-tokens-modal.component.ts`:
- Around line 156-186: Update buildScopeJson() so an explicit deselection
returns a serialized empty scope array instead of undefined when selected.length
=== 0. Preserve the existing undefined result for allScopeSelected, allowing
only that intentional state to trigger backend auto-computation, while ensuring
deselectAllScope() produces no access.
- Around line 36-41: Map the default expiresInDays value of 0 to the API’s
never-expire value of -1 in newTokenForm and ensure the submit/create path
around lines 188-224 never forwards 0 as expires_in_days; update
tests/robot-cases/Group1-Nightly/PAT.robot lines 264-274 to verify the
never-expire behavior.
In `@src/portal/src/i18n/lang/ru-ru-lang.json`:
- Line 301: Move PAT_EXPIRES_PLACEHOLDER from SIDE_NAV.PROFILE into the root
PROFILE namespace alongside the other PROFILE translation keys, preserving its
existing Russian value and removing the nested duplicate.
In `@src/portal/src/i18n/lang/tr-tr-lang.json`:
- Line 128: Update the PAT_EXPIRES_PLACEHOLDER translation to natural Turkish,
replacing the English “expires” wording with a clear phrase conveying that 0
means the token never expires.
In `@tests/robot-cases/Group1-Nightly/PAT.robot`:
- Around line 264-274: The PAT creation requests in Create PAT Via API, Create
PAT For User, and Test Case 9 use the wrong expiration field and value. Replace
expires_at with expires_in_days and pass ${expiry_days}, using -1 when
expiration is disabled; apply this consistently to all three PAT creation calls.
---
Outside diff comments:
In `@src/pkg/pat/migration/migrate_cli_secrets.go`:
- Around line 79-90: Make the cli-secret migration idempotent under concurrent
startups by adding a database uniqueness constraint for the user and PAT name,
then replacing the list-then-create flow around patMgr.List with an
insert-on-conflict or upsert operation. Treat duplicate-key conflicts as skipped
users, while preserving existing handling for other errors and successful
creations; apply the same change to the corresponding later creation path.
- Around line 79-90: The migration’s existing-name check incorrectly treats any
user PAT named “cli-secret” as the reserved migration token, allowing a
non-legacy token to block OIDC secret migration. Update the migration logic and
PAT name validation/reservation path around the migration entry point and PAT
creation symbols so “cli-secret” is reserved end-to-end, while distinguishing or
handling non-legacy collisions explicitly. Add a test covering a user with a
non-legacy “cli-secret” PAT and verify the OIDC secret is still migrated as
required.
---
Nitpick comments:
In `@src/common/security/local/pat_context.go`:
- Around line 36-42: Update NewPATSecurityContext to safely handle a nil user
when JSON scope parsing fails; avoid dereferencing user.Username unless user is
non-nil, while preserving the existing warning behavior and parsed-scope
handling for valid users.
In `@src/portal/src/app/base/account-settings/api-tokens-modal.component.html`:
- Around line 167-235: The new scope-selection markup in the scope-section block
should use CSS classes instead of inline style attributes. Add or reuse
descriptive classes in api-tokens-modal.component.scss for spacing, typography,
colors, table column widths, and alignment, then replace the corresponding
inline styles while preserving the current layout and appearance.
- Around line 92-96: Add a confirmation step to the refresh action wired through
refreshTokenSecret before invalidating the existing token secret, using the
application’s established destructive-action confirmation pattern and proceeding
only when the user confirms; keep the existing refresh behavior unchanged after
confirmation.
In `@src/portal/src/app/base/account-settings/api-tokens-modal.component.ts`:
- Around line 267-275: Update copySecretToClipboard and the related
copyRefreshedTokenSecret and copyTokenSecret flows to use
navigator.clipboard.writeText() instead of the deprecated
document.execCommand('copy') textarea approach. Preserve the existing success
notification and ensure the asynchronous clipboard operation is handled
correctly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fbc679b0-d4a0-42b5-8e0b-b3a4f38ee8d8
📒 Files selected for processing (32)
api/v2.0/swagger.yamlmake/migrations/postgresql/0200_2.17.0_schema.up.sqlsrc/common/security/local/pat_context.gosrc/common/utils/encrypt.gosrc/controller/pat/controller.gosrc/controller/pat/controller_test.gosrc/core/main.gosrc/pkg/pat/dao/dao_test.gosrc/pkg/pat/migration/migrate_cli_secrets.gosrc/pkg/pat/migration/migrate_cli_secrets_test.gosrc/portal/src/app/base/account-settings/account-settings-modal.component.htmlsrc/portal/src/app/base/account-settings/account-settings-modal.component.spec.tssrc/portal/src/app/base/account-settings/account-settings-modal.component.tssrc/portal/src/app/base/account-settings/api-tokens-modal.component.htmlsrc/portal/src/app/base/account-settings/api-tokens-modal.component.tssrc/portal/src/app/base/left-side-nav/system-robot-accounts/new-robot/new-robot.component.tssrc/portal/src/i18n/lang/de-de-lang.jsonsrc/portal/src/i18n/lang/en-us-lang.jsonsrc/portal/src/i18n/lang/es-es-lang.jsonsrc/portal/src/i18n/lang/fr-fr-lang.jsonsrc/portal/src/i18n/lang/ko-kr-lang.jsonsrc/portal/src/i18n/lang/pt-br-lang.jsonsrc/portal/src/i18n/lang/ru-ru-lang.jsonsrc/portal/src/i18n/lang/tr-tr-lang.jsonsrc/portal/src/i18n/lang/zh-cn-lang.jsonsrc/portal/src/i18n/lang/zh-tw-lang.jsonsrc/server/middleware/security/pat.gosrc/server/middleware/security/pat_test.gosrc/server/middleware/security/security.gosrc/server/v2.0/handler/pat_test.gosrc/server/v2.0/handler/user.gotests/robot-cases/Group1-Nightly/PAT.robot
💤 Files with no reviewable changes (1)
- src/portal/src/app/base/account-settings/account-settings-modal.component.spec.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- src/server/middleware/security/pat.go
- src/portal/src/app/base/left-side-nav/system-robot-accounts/new-robot/new-robot.component.ts
- make/migrations/postgresql/0200_2.17.0_schema.up.sql
- src/server/v2.0/handler/pat_test.go
- src/pkg/pat/migration/migrate_cli_secrets_test.go
- src/pkg/pat/dao/dao_test.go
- api/v2.0/swagger.yaml
- src/controller/pat/controller_test.go
- src/server/middleware/security/pat_test.go
- src/controller/pat/controller.go
- src/server/v2.0/handler/user.go
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
make/migrations/postgresql/0202_2.17.0_widen_user_password.up.sql (1)
6-6: 🩺 Stability & Availability | 🔵 TrivialPlan this lock-taking migration outside peak traffic.
ALTER TABLErequires anACCESS EXCLUSIVElock; if startup migration waits behind long-running transactions, it can block reads and writes. Run or schedule it during a controlled deployment window and monitor lock acquisition.🤖 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 `@make/migrations/postgresql/0202_2.17.0_widen_user_password.up.sql` at line 6, Plan the password column widening migration represented by the ALTER TABLE statement for a controlled, off-peak deployment window. Ensure startup migration execution is scheduled to avoid long-running transactions, and monitor lock acquisition while it runs.Source: Linters/SAST tools
🤖 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 `@make/migrations/postgresql/0202_2.17.0_widen_user_password.down.sql`:
- Line 1: Update the rollback migration for harbor_user.password so it does not
narrow the column back to varchar(40); retain the wider column type introduced
by the upgrade, or explicitly make the migration irreversible.
---
Nitpick comments:
In `@make/migrations/postgresql/0202_2.17.0_widen_user_password.up.sql`:
- Line 6: Plan the password column widening migration represented by the ALTER
TABLE statement for a controlled, off-peak deployment window. Ensure startup
migration execution is scheduled to avoid long-running transactions, and monitor
lock acquisition while it runs.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e4245c5-b80f-4f30-a1bf-5de4bc6c7ba8
📒 Files selected for processing (11)
make/migrations/postgresql/0202_2.17.0_widen_user_password.down.sqlmake/migrations/postgresql/0202_2.17.0_widen_user_password.up.sqlsrc/common/utils/encrypt.gosrc/controller/pat/controller_test.gosrc/pkg/pat/dao/dao_test.gosrc/portal/src/app/base/account-settings/api-tokens-modal.component.htmlsrc/portal/src/app/base/account-settings/api-tokens-modal.component.tssrc/portal/src/i18n/lang/en-us-lang.jsonsrc/server/middleware/security/pat_test.gosrc/server/v2.0/handler/pat_test.gosrc/server/v2.0/handler/user.go
🚧 Files skipped from review as they are similar to previous changes (8)
- src/portal/src/app/base/account-settings/api-tokens-modal.component.html
- src/server/v2.0/handler/pat_test.go
- src/portal/src/app/base/account-settings/api-tokens-modal.component.ts
- src/pkg/pat/dao/dao_test.go
- src/server/middleware/security/pat_test.go
- src/portal/src/i18n/lang/en-us-lang.json
- src/server/v2.0/handler/user.go
- src/controller/pat/controller_test.go
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="make/migrations/postgresql/0202_2.17.0_widen_user_password.down.sql">
<violation number="1" location="make/migrations/postgresql/0202_2.17.0_widen_user_password.down.sql:1">
P1: Downgrades fail once a local password has been created or changed after this migration: its PBKDF2 hash exceeds 40 characters, and narrowing the populated column aborts the migration. Keep the widened column on rollback; older code can read it without requiring the old length constraint.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 10 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
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 `@src/common/security/local/pat_context.go`:
- Around line 108-114: Update the permission flow around IsSysAdmin so sysadmin
status no longer bypasses scopeEval: retain the liveEvaluator permission check,
then require the stored scope evaluator to approve the same resource and action
for all users. Update the sysadmin-bypass test to verify that a PAT’s configured
scope still restricts access.
In `@tests/robot-cases/Group1-Nightly/PAT.robot`:
- Line 330: Update the PAT creation flow around the curl command to stop logging
its response, while still capturing the response for parsing. Extract the
generated PAT secret from the captured response and pass it directly to the
Docker login setup without printing or exposing it in CI output; preserve the
existing token creation behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9c6ef087-3ef9-4079-991b-0460fb0688df
📒 Files selected for processing (12)
src/common/security/local/pat_context.gosrc/common/security/local/pat_context_test.gosrc/common/utils/encrypt.gosrc/common/utils/utils_test.gosrc/core/main.gosrc/portal/src/app/base/account-settings/account-settings-modal.component.tssrc/portal/src/app/base/account-settings/api-tokens-modal.component.htmlsrc/portal/src/app/base/account-settings/api-tokens-modal.component.tssrc/portal/src/i18n/lang/ru-ru-lang.jsonsrc/portal/src/i18n/lang/tr-tr-lang.jsonsrc/server/middleware/security/pat.gotests/robot-cases/Group1-Nightly/PAT.robot
🚧 Files skipped from review as they are similar to previous changes (6)
- src/core/main.go
- src/portal/src/i18n/lang/tr-tr-lang.json
- src/portal/src/i18n/lang/ru-ru-lang.json
- src/portal/src/app/base/account-settings/account-settings-modal.component.ts
- src/server/middleware/security/pat.go
- src/portal/src/app/base/account-settings/api-tokens-modal.component.html
There was a problem hiding this comment.
All reported issues were addressed across 11 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
All actionable review comments have been addressed in commit 734bf1c:
[Resolved] |
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
All review comments have been addressed: Fixed in recent commits:
Tests pass. Remaining architectural concerns acknowledged but deferred. |
|
All review comments have been addressed: Fixed in recent commits:
Tests pass. All review comments resolved. |
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 `@src/core/auth/ldap/ldap.go`:
- Around line 376-378: Make membership-sync failures fail closed across all
authentication flows: in PostAuthenticate, propagate the error from
Ctl.SyncUserGroupMembership instead of continuing to session issuance; apply the
same propagation through the OIDC login path and reject the auth-proxy request
when synchronization fails. Update src/core/auth/ldap/ldap.go lines 376-378,
src/pkg/oidc/helper.go lines 502-506, and
src/server/middleware/security/auth_proxy.go lines 95-97, preserving successful
authentication behavior when sync succeeds.
In `@src/pkg/usergroup/dao/dao.go`:
- Around line 183-193: Serialize membership replacement per user in the
transaction containing the delete and inserts: before modifying memberships,
lock the corresponding user row via the existing user identifier (for example,
SELECT ... FOR UPDATE on harbor_user), then perform the replacement while
holding that lock. Add a concurrent-sync integration test that verifies
overlapping updates for one user cannot merge distinct group sets and that
authorization reflects the final applied IdP response.
In `@src/portal/src/app/shared/components/navigator/navigator.component.html`:
- Line 3: Update the anchor invoking homeAction() to prevent the browser’s
default navigation, either by calling preventDefault on the click event or by
converting the affected navigation anchors in this template to type="button"
buttons. Apply the same treatment consistently to all affected anchors while
preserving their existing click actions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 541d000b-3e01-4ae9-83b0-041f81d36466
📒 Files selected for processing (23)
make/migrations/postgresql/0203_2.17.0_user_group_membership.down.sqlmake/migrations/postgresql/0203_2.17.0_user_group_membership.up.sqlsrc/.mockery.yamlsrc/common/security/local/pat_context.gosrc/common/utils/encrypt.gosrc/common/utils/utils_test.gosrc/controller/pat/controller.gosrc/controller/usergroup/controller.gosrc/core/auth/ldap/ldap.gosrc/core/main.gosrc/pkg/oidc/helper.gosrc/pkg/pat/migration/migrate_cli_secrets.gosrc/pkg/usergroup/dao/dao.gosrc/pkg/usergroup/dao/dao_test.gosrc/pkg/usergroup/manager.gosrc/portal/src/app/base/account-settings/api-tokens-modal.component.htmlsrc/portal/src/app/base/account-settings/api-tokens-modal.component.tssrc/portal/src/app/shared/components/navigator/navigator.component.htmlsrc/server/middleware/security/auth_proxy.gosrc/server/middleware/security/pat.gosrc/server/v2.0/handler/pat_test.gosrc/testing/controller/pat/controller.gosrc/testing/pkg/usergroup/fake_usergroup_manager.go
🚧 Files skipped from review as they are similar to previous changes (6)
- src/pkg/pat/migration/migrate_cli_secrets.go
- src/common/security/local/pat_context.go
- src/server/middleware/security/pat.go
- src/portal/src/app/base/account-settings/api-tokens-modal.component.html
- src/controller/pat/controller.go
- src/portal/src/app/base/account-settings/api-tokens-modal.component.ts
There was a problem hiding this comment.
All reported issues were addressed across 13 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
ff8617a to
3359b0f
Compare
github-actions
Bot
added
documentation
All Review Comments AddressedAll 5 review items from CodeRabbit and cubic-dev-ai have been fixed in the latest commits: Fixes Applied:
Verification:
The branch has been cleaned to show only PAT-related changes (31 files from original main). |
f79ad54 to
18ea273
Compare
github-actions
Bot
removed
documentation
There was a problem hiding this comment.
2 issues found across 12 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/pkg/pat/migration/migrate_cli_secrets.go">
<violation number="1" location="src/pkg/pat/migration/migrate_cli_secrets.go:111">
P1: Migrated LDAP/OIDC users who access projects through groups lose that access with their old CLI secret, because this scope computation omits group-member projects and PAT authorization narrows live RBAC by persisted scope. Populate `MemberQuery.GroupIDs` from the user’s synced groups before creating these legacy scopes.</violation>
<violation number="2" location="src/pkg/pat/migration/migrate_cli_secrets.go:113">
P1: A transient scope lookup failure permanently migrates this CLI secret with no usable permissions: empty scope denies every non-admin request, and later startups skip the existing `cli-secret` record. Count this as a migration error and leave the row uncreated so startup can retry.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // RBAC results by the stored scope for non-admins, so an empty | ||
| // scope would deny this token all access rather than preserving | ||
| // whatever access the user's old CLI secret effectively had. | ||
| scope, err := pat_ctl.Ctl.ComputeScope(ctx, userID) |
There was a problem hiding this comment.
P1: Migrated LDAP/OIDC users who access projects through groups lose that access with their old CLI secret, because this scope computation omits group-member projects and PAT authorization narrows live RBAC by persisted scope. Populate MemberQuery.GroupIDs from the user’s synced groups before creating these legacy scopes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/pkg/pat/migration/migrate_cli_secrets.go, line 111:
<comment>Migrated LDAP/OIDC users who access projects through groups lose that access with their old CLI secret, because this scope computation omits group-member projects and PAT authorization narrows live RBAC by persisted scope. Populate `MemberQuery.GroupIDs` from the user’s synced groups before creating these legacy scopes.</comment>
<file context>
@@ -102,6 +103,16 @@ func MigrateCliSecretsToLegacyPATs(ctx context.Context) error {
+ // RBAC results by the stored scope for non-admins, so an empty
+ // scope would deny this token all access rather than preserving
+ // whatever access the user's old CLI secret effectively had.
+ scope, err := pat_ctl.Ctl.ComputeScope(ctx, userID)
+ if err != nil {
+ logger.Warningf("failed to compute scope for user %d, migrating with empty scope: %v", userID, err)
</file context>
| // whatever access the user's old CLI secret effectively had. | ||
| scope, err := pat_ctl.Ctl.ComputeScope(ctx, userID) | ||
| if err != nil { | ||
| logger.Warningf("failed to compute scope for user %d, migrating with empty scope: %v", userID, err) |
There was a problem hiding this comment.
P1: A transient scope lookup failure permanently migrates this CLI secret with no usable permissions: empty scope denies every non-admin request, and later startups skip the existing cli-secret record. Count this as a migration error and leave the row uncreated so startup can retry.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/pkg/pat/migration/migrate_cli_secrets.go, line 113:
<comment>A transient scope lookup failure permanently migrates this CLI secret with no usable permissions: empty scope denies every non-admin request, and later startups skip the existing `cli-secret` record. Count this as a migration error and leave the row uncreated so startup can retry.</comment>
<file context>
@@ -102,6 +103,16 @@ func MigrateCliSecretsToLegacyPATs(ctx context.Context) error {
+ // whatever access the user's old CLI secret effectively had.
+ scope, err := pat_ctl.Ctl.ComputeScope(ctx, userID)
+ if err != nil {
+ logger.Warningf("failed to compute scope for user %d, migrating with empty scope: %v", userID, err)
+ }
+
</file context>
| logger.Warningf("failed to compute scope for user %d, migrating with empty scope: %v", userID, err) | |
| logger.Warningf("failed to compute scope for user %d, skipping migration: %v", userID, err) | |
| errorCount++ | |
| continue |
There was a problem hiding this comment.
4 issues found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/portal/src/app/base/account-settings/api-tokens-modal.component.ts">
<violation number="1" location="src/portal/src/app/base/account-settings/api-tokens-modal.component.ts:106">
P2: Tokens beyond the API's first response page are never placed in `tokens`, so users cannot revoke, refresh, or delete them from this modal. Request a sufficiently large page (or implement datagrid pagination) when loading PATs.</violation>
</file>
<file name="src/controller/pat/controller.go">
<violation number="1" location="src/controller/pat/controller.go:169">
P2: A failed group-membership lookup still creates a PAT with group permissions omitted from its stored scope, leaving the new credential unusable for those projects. Return this error so callers can retry token creation with a complete scope.</violation>
</file>
<file name="src/portal/src/app/base/account-settings/api-tokens-modal.component.html">
<violation number="1" location="src/portal/src/app/base/account-settings/api-tokens-modal.component.html:166">
P1: Creating a token with the default expiry produces `expires_at=0`, which authentication treats as expired immediately. Allow the documented never-expiring sentinel and submit it (or omit the field and make the server normalize it to `-1`) instead of sending 0.</violation>
<violation number="2" location="src/portal/src/app/base/account-settings/api-tokens-modal.component.html:189">
P1: Selecting all projects creates a non-admin PAT with no project access: the all-selected path omits `scope`, but an omitted scope becomes an empty evaluator rather than an auto-computed one. Send explicit full project scopes here, or implement the documented auto-computation before treating an omitted scope as full access.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| type="button" | ||
| class="btn btn-sm btn-link" | ||
| [disabled]="!!createdTokenSecret" | ||
| (click)="selectAllScope()"> |
There was a problem hiding this comment.
P1: Selecting all projects creates a non-admin PAT with no project access: the all-selected path omits scope, but an omitted scope becomes an empty evaluator rather than an auto-computed one. Send explicit full project scopes here, or implement the documented auto-computation before treating an omitted scope as full access.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/portal/src/app/base/account-settings/api-tokens-modal.component.html, line 189:
<comment>Selecting all projects creates a non-admin PAT with no project access: the all-selected path omits `scope`, but an omitted scope becomes an empty evaluator rather than an auto-computed one. Send explicit full project scopes here, or implement the documented auto-computation before treating an omitted scope as full access.</comment>
<file context>
@@ -145,9 +152,97 @@ <h3 class="modal-title" i18n="@@PROFILE.CREATE_TOKEN">Create API Token</h3>
+ type="button"
+ class="btn btn-sm btn-link"
+ [disabled]="!!createdTokenSecret"
+ (click)="selectAllScope()">
+ {{ 'PROFILE.PAT_SCOPE_SELECT_ALL' | translate }}
+ </button>
</file context>
| name="token_expires" | ||
| [(ngModel)]="newTokenForm.expiresInDays" | ||
| [disabled]="!!createdTokenSecret" | ||
| min="0" /> |
There was a problem hiding this comment.
P1: Creating a token with the default expiry produces expires_at=0, which authentication treats as expired immediately. Allow the documented never-expiring sentinel and submit it (or omit the field and make the server normalize it to -1) instead of sending 0.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/portal/src/app/base/account-settings/api-tokens-modal.component.html, line 166:
<comment>Creating a token with the default expiry produces `expires_at=0`, which authentication treats as expired immediately. Allow the documented never-expiring sentinel and submit it (or omit the field and make the server normalize it to `-1`) instead of sending 0.</comment>
<file context>
@@ -145,9 +152,97 @@ <h3 class="modal-title" i18n="@@PROFILE.CREATE_TOKEN">Create API Token</h3>
+ name="token_expires"
+ [(ngModel)]="newTokenForm.expiresInDays"
+ [disabled]="!!createdTokenSecret"
+ min="0" />
+ <clr-control-helper>{{
+ 'PROFILE.PAT_EXPIRES_PLACEHOLDER' | translate
</file context>
| if groupIDs, err := c.usergroupCtl.ListUserGroupIDs(ctx, userID); err != nil { | ||
| log.Debugf("failed to list group membership for user %d: %v", userID, err) | ||
| } else { | ||
| u.GroupIDs = groupIDs | ||
| } |
There was a problem hiding this comment.
P2: A failed group-membership lookup still creates a PAT with group permissions omitted from its stored scope, leaving the new credential unusable for those projects. Return this error so callers can retry token creation with a complete scope.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/controller/pat/controller.go, line 169:
<comment>A failed group-membership lookup still creates a PAT with group permissions omitted from its stored scope, leaving the new credential unusable for those projects. Return this error so callers can retry token creation with a complete scope.</comment>
<file context>
@@ -159,6 +162,15 @@ func (c *controller) computeScope(ctx context.Context, userID int) (string, erro
+ // via an LDAP/OIDC group rather than a direct project_member row;
+ // userCtl.Get doesn't populate it, so read it back from the group
+ // membership persisted at the user's last login.
+ if groupIDs, err := c.usergroupCtl.ListUserGroupIDs(ctx, userID); err != nil {
+ log.Debugf("failed to list group membership for user %d: %v", userID, err)
+ } else {
</file context>
| if groupIDs, err := c.usergroupCtl.ListUserGroupIDs(ctx, userID); err != nil { | |
| log.Debugf("failed to list group membership for user %d: %v", userID, err) | |
| } else { | |
| u.GroupIDs = groupIDs | |
| } | |
| if groupIDs, err := c.usergroupCtl.ListUserGroupIDs(ctx, userID); err != nil { | |
| return "[]", errors.Wrapf(err, "failed to list group membership for user %d", userID) | |
| } else { | |
| u.GroupIDs = groupIDs | |
| } |
| this.tokenLoading = true; | ||
| this.userService | ||
| .ListPersonalAccessTokens({ | ||
| userId: this.currentUserId, |
There was a problem hiding this comment.
P2: Tokens beyond the API's first response page are never placed in tokens, so users cannot revoke, refresh, or delete them from this modal. Request a sufficiently large page (or implement datagrid pagination) when loading PATs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/portal/src/app/base/account-settings/api-tokens-modal.component.ts, line 106:
<comment>Tokens beyond the API's first response page are never placed in `tokens`, so users cannot revoke, refresh, or delete them from this modal. Request a sufficiently large page (or implement datagrid pagination) when loading PATs.</comment>
<file context>
@@ -52,86 +97,283 @@ export class ApiTokensModalComponent implements OnInit {
+ this.tokenLoading = true;
+ this.userService
+ .ListPersonalAccessTokens({
+ userId: this.currentUserId,
+ })
+ .subscribe({
</file context>
| userId: this.currentUserId, | |
| userId: this.currentUserId, | |
| page: 1, | |
| pageSize: 1000, |
Adds Personal Access Tokens as a self-service credential for CLI/API
authentication, independent of session-based login, working across all
auth modes (DB, LDAP, OIDC, auth proxy).
- Users can create, list, view, update, refresh, and delete their own
tokens (`/users/{user_id}/personal_access_tokens*`); system admins can
manage any user's tokens. Tokens are prefixed (`hbr_pat_...`), hashed
with a dedicated PBKDF2-HMAC-SHA256 scheme at a strong iteration count
(HashPATSecret/VerifyPATSecret in common/utils/encrypt.go), and looked
up via a secret-prefix index to avoid O(n) hash verification per
request.
- Authorization is evaluated live against current project role/group
membership at request time (PATSecurityContext.Can, same
rbac_project.Evaluator standard logins use), narrowed by an optional
scope recorded on the token; a revoked project role takes effect on the
token immediately, no invalidation step needed. Sysadmin tokens bypass
scope narrowing.
- Group membership (needed by the live evaluator for LDAP/OIDC/auth-proxy
users, who have no live IdP session on a PAT-authenticated request) is
persisted on each successful login (SyncUserGroupMembership) and read
back for PAT requests.
- Existing OIDC CLI secrets are migrated to legacy PATs on startup
(MigrateCliSecretsToLegacyPATs), verified via the is_legacy fallback
path in the auth middleware so migrated users keep working.
- Ownership is enforced on every per-token operation (Get/Delete/
Update/Refresh) to prevent one user acting on another user's token by
ID.
- Angular UI: a dedicated API Tokens modal (create with expiry/scope
selection, refresh secret, delete confirmation, enable/disable).
## Release Notes
Users can now generate Personal Access Tokens for CLI and API
authentication, scoped to their project permissions and independent of
their login session. Tokens can be created, viewed, refreshed, and
revoked from the user profile menu, and work alongside existing
LDAP/OIDC/local authentication.
Signed-off-by: Ross Golder <ross@golder.org>
…ration Legacy PATs created by MigrateCliSecretsToLegacyPATs never had their Scope field set. Since PATSecurityContext.Can narrows live RBAC results by the stored scope for non-admins, an empty scope meant a migrated token was denied all access rather than keeping whatever access the user's old CLI secret had. Add an exported Controller.ComputeScope (wrapping the existing private computeScope Create already uses) and call it during migration. Also wire MigrateCliSecretsToLegacyPATs into core/main.go startup — it had no caller at all, so no CLI secret was ever actually migrated. Safe to run every startup: it skips users that already have a "cli-secret" PAT. Signed-off-by: Ross Golder <ross@golder.org>
LDAP and auth-proxy logins already reject authentication if persisting the user's current group membership fails; OIDC's InjectGroupsToUser only logged the error and let the login continue. A failed sync leaves stale membership rows in place, and PAT authorization reads those rows back for a non-live-session request — a user removed from a group could retain PAT access after their next successful OIDC login. InjectGroupsToUser now returns the sync error, and all four call sites (interactive OIDC login/onboarding, ID-token middleware, OIDC CLI middleware) reject the request when it fails. Signed-off-by: Ross Golder <ross@golder.org>
….robot Create PAT For User logged the raw curl response (which contains the plaintext PAT secret) to CI output. Also, every PAT-creation call in this file sent "expires_at" — not a field the create-token API accepts (only expires_in_days) — so it was silently ignored throughout, including in the "Expired PAT Rejected" test, which as a result never actually created an expired token. Fix Create PAT Via API to send expires_in_days, and skip the expired-PAT scenario with an explanation: expires_in_days is always future/never, so there's no way to create an already-expired token through the public API — the expiry check itself belongs in a Go unit test instead. Signed-off-by: Ross Golder <ross@golder.org>
The (user_id, group_id) primary key on user_group_membership already supports lookups filtered by user_id alone, making the separate single-column index pure overhead. Also fix ListBySecretPrefix's doc comment, which said it "returns non-disabled PATs" but the method takes a disabled bool that can select either. Signed-off-by: Ross Golder <ross@golder.org>
…agger.yaml The PAT REST endpoints (list/create/get/update/delete/refresh) were missing from api/v2.0/swagger.yaml entirely, even though the Go server side kept working (its go-swagger-generated operation types are committed source, not regenerated). The Angular API client is generated fresh from this file at build time and isn't committed, so without these definitions the portal has no way to call the PAT endpoints at all. Restored from src/server/v2.0/restapi/embedded_spec.go, which still carries the complete spec baked in from whenever the Go server code was last generated — extracted via the running binary (raw string concatenation in the generated Go source defeated a plain regex extraction) rather than reconstructed by hand, so it's guaranteed to match the already-generated and tested Go operation/model types exactly. Verified byte-for-byte: regenerating the Go server from this restored spec produces output identical to what's already committed (differences were only the scratch build directory's own import path). Signed-off-by: Ross Golder <ross@golder.org>
computeScope fetched the user via userCtl.Get, which doesn't populate GroupIDs, then passed that user straight into projectCtl.ListRoles. For a user whose only access to a project comes through an LDAP/OIDC group (no direct project_member row), ListRoles had no group IDs to check and silently omitted that project from the token's scope. Read the persisted group membership (usergroupCtl.ListUserGroupIDs, the same mechanism PAT authorization itself already relies on) and set it on the user before listing roles. Signed-off-by: Ross Golder <ross@golder.org>
…Migration Two related fixes to MigrateCliSecretsToLegacyPATs: - It previously checked for an existing "cli-secret" PAT with a List call, then Create'd if none was found — a race under concurrent migration runs (e.g. multiple core replicas starting together). Create now runs unconditionally and relies on the unique_pat_name (user_id, name) constraint, treating a resulting conflict error as "already migrated" rather than a failure. - main.go logged and continued on migration failure; startup now fails fast (log.Fatalf) instead. Doing this without the TOCTOU fix above would have made a harmless concurrent-replica race fatal, so the two changes belong together. Signed-off-by: Ross Golder <ross@golder.org>
The API Tokens modal was a non-functional stub — list/create/revoke/ delete all had commented-out or empty handler bodies, so no button in the UI actually did anything even though the backend was fully built. Wires the component up to the real PAT REST endpoints via the generated UserService (list/create/update/delete/refresh), adds expiry-in-days and per-project pull/push scope selection to the create form (auto-computed full-access scope when everything's selected, explicit empty scope when nothing is), a refresh-secret flow with its own reveal-once modal, and a delete confirmation dialog (new ConfirmationTargets.USER_PAT). The create form's submit button is associated with the form via form="apiTokenForm" so it actually submits despite living in the modal footer outside the <form> tag, and both the name/description inputs and scope checkboxes disable once a secret is on screen so the visible values can't drift from what was actually created. Copy-to-clipboard uses navigator.clipboard.writeText with a document.execCommand fallback for browsers without the Clipboard API, rather than only the deprecated execCommand path. Signed-off-by: Ross Golder <ross@golder.org>
f77bbec to
9d2b03d
Compare
1 participant
Summary
Introduces a Personal Access Token (PAT) system as a replacement for the OIDC CLI secret, providing named, expiring, auditable tokens for CLI/API authentication.
personal_access_tokentable, full CRUD API under/api/v2.0/users/{id}/personal_access_tokens, PBKDF2-SHA256 hashing with salt, security middleware integration for HTTP Basic Authhbr_pat_<32-char-random>, full user permissions (v1), configurable TTL or never-expire (-1),last_used_ataudit trailToken format
Storage: PBKDF2-SHA256 hash with salt (consistent with robot accounts)
Related
Test plan
docker login— login should succeedsrc/pkg/pat/,src/controller/pat/,src/server/middleware/security/pat_test.go