Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a SetupService for initial instance provisioning with optional setup-time Personal Access Token (PAT) issuance, wires account manager into instance HTTP handlers, implements rollback that deletes the owner user and reloads setup state on post-creation failures, and updates API/types, handlers, and tests to support PAT options. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as "HTTP Handler"
participant SetupSvc as "SetupService"
participant InstanceMgr as "Instance Manager"
participant AccountMgr as "Account Manager"
participant IdP as "Embedded IdP"
Client->>Handler: POST /api/setup {email,password,name,create_pat?,pat_expire_in?}
Handler->>SetupSvc: SetupOwner(email,password,name, opts)
SetupSvc->>InstanceMgr: CreateOwnerUser(email,password,name)
InstanceMgr->>IdP: CreateUser(...)
IdP-->>InstanceMgr: userID
InstanceMgr-->>SetupSvc: owner user info
alt PAT enabled & requested
SetupSvc->>AccountMgr: Lookup account for userID
AccountMgr-->>SetupSvc: accountID
SetupSvc->>AccountMgr: CreatePAT(accountID,"setup-token",expires)
AccountMgr-->>SetupSvc: plainToken
SetupSvc-->>Handler: SetupResult{user, plainToken}
Handler-->>Client: 200 SetupResponse (personal_access_token) with Cache-Control: no-store
else PAT creation fails
SetupSvc->>InstanceMgr: RollbackSetup(userID)
InstanceMgr->>IdP: DeleteUser(userID)
IdP-->>InstanceMgr: success / not found
InstanceMgr->>InstanceMgr: Reload setupRequired state
InstanceMgr-->>SetupSvc: rollback result
SetupSvc-->>Handler: error
Handler-->>Client: error response
else PAT not requested or disabled
SetupSvc-->>Handler: SetupResult{user, no token}
Handler-->>Client: 200 SetupResponse
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
shared/management/http/api/openapi.yml (2)
3455-3458: Document non-cacheable semantics for setup responses containing PATs.This response now includes a plaintext credential. It’s safer to explicitly document
Cache-Control: no-storeonPOST /api/setupsuccess responses.🔐 Suggested response header documentation
responses: '200': description: Setup completed successfully + headers: + Cache-Control: + description: Prevents caching of responses containing sensitive setup credentials. + schema: + type: string + example: no-store content: application/json: schema: $ref: '#/components/schemas/SetupResponse'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/management/http/api/openapi.yml` around lines 3455 - 3458, The OpenAPI spec documents a plaintext personal_access_token in the POST /api/setup success response but does not declare non-cacheable semantics; update the POST /api/setup response definition in shared/management/http/api/openapi.yml (the response that includes the personal_access_token field) to add a response header entry declaring "Cache-Control: no-store" (optionally also "Pragma: no-cache" and "Vary" if desired) so clients and intermediaries know credentials must not be cached; ensure the header is added to the exact response object that contains personal_access_token.
3432-3438: Clarify whenpat_expire_inis effective.
pat_expire_inshould explicitly state it only applies whencreate_patistrue; otherwise clients may interpret it as always meaningful.✏️ Suggested doc tweak
pat_expire_in: - description: Expiration of the Personal Access Token in days. Defaults to 1 day when omitted. + description: Expiration of the Personal Access Token in days. Used only when `create_pat` is `true`. Defaults to 1 day when omitted. type: integer minimum: 1 maximum: 365 default: 1 example: 30🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/management/http/api/openapi.yml` around lines 3432 - 3438, Update the OpenAPI description for the pat_expire_in schema property to state that it only applies when create_pat is true: modify the description for pat_expire_in to read something like "Expiration of the Personal Access Token in days (applies only when create_pat is true). Defaults to 1 day when omitted; valid range 1–365." Keep existing type/minimum/maximum/default/example and ensure the text explicitly references create_pat so clients know it's conditional.management/server/instance/setup_service.go (2)
99-113: ValidateaccountManageravailability before creating the owner user.When
opts.CreatePATis true and the feature is enabled, butm.accountManager == nil(misconfiguration), the current order creates the IdP user, then fails, then rolls it back. Move this precondition aboveCreateOwnerUser(or assert it inNewSetupService) so a misconfigured deployment fails fast without churning IdP/embedded-IdP state.♻️ Proposed reorder
opts, err := normalizeSetupOptions(opts, m.setupPATEnabled) if err != nil { return nil, err } + if opts.CreatePAT && m.accountManager == nil { + return nil, fmt.Errorf("account manager is required to create setup PAT") + } + userData, err := m.instanceManager.CreateOwnerUser(ctx, email, password, name) if err != nil { return nil, err } result := &SetupResult{User: userData} if !opts.CreatePAT { return result, nil } - - if m.accountManager == nil { - err := fmt.Errorf("account manager is required to create setup PAT") - m.rollbackSetup(ctx, userData.ID, "setup PAT requested without account manager", err, "") - return nil, err - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/instance/setup_service.go` around lines 99 - 113, Move the check for m.accountManager being nil so we validate preconditions before creating an IdP user: ensure the accountManager nil check (and the error + rollbackSetup call currently below CreateOwnerUser) is performed before calling m.instanceManager.CreateOwnerUser(ctx, email, password, name) (or alternatively assert accountManager in NewSetupService); this prevents creating the owner user when opts.CreatePAT is true and accountManager is missing—update the logic around opts.CreatePAT, CreateOwnerUser, accountManager, and rollbackSetup to fail fast without mutating IdP state.
16-30: Consolidate PAT expiry bounds into a reusable constant fromuser.go.The bounds
patMinExpireDays = 1andpatMaxExpireDays = 365duplicate the validation inCreatePAT(user.go:398–400:expiresIn < 1 || expiresIn > 365). Currently they match, but this creates drift risk: a future change to the enforcement inuser.gowill not propagate to/api/setupwithout explicit awareness. Export these as public constants (e.g.,PAT_MIN_EXPIRE_DAYS,PAT_MAX_EXPIRE_DAYS) frommanagement/server/user.goand reuse them here to maintain a single source of truth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/instance/setup_service.go` around lines 16 - 30, Replace the duplicated local bounds patMinExpireDays and patMaxExpireDays with a single source of truth by exporting public constants from the user package and using them here: add exported constants (e.g., PAT_MIN_EXPIRE_DAYS and PAT_MAX_EXPIRE_DAYS) in the same file that defines DefaultAccountManager.CreatePAT (where the current 1 and 365 checks live), update CreatePAT to reference those exported names if needed, then remove the local patMinExpireDays/patMaxExpireDays definitions in setup_service.go and import/use the exported PAT_MIN_EXPIRE_DAYS and PAT_MAX_EXPIRE_DAYS constants instead (also update imports).management/server/http/handlers/instance/instance_handler_test.go (1)
314-528: SetContent-Type: application/jsonon the new/setupPAT tests for consistency.All pre-existing
TestSetup_*cases set the header beforeServeHTTP. The new PAT-related tests omit it. The Go decoder doesn't require it, so the tests pass today, but if the handler ever begins enforcing content negotiation (or middleware rejects on missing/invalid Content-Type), only these new tests will silently break. Easy alignment fix:req := httptest.NewRequest(http.MethodPost, "/setup", bytes.NewBufferString(body)) + req.Header.Set("Content-Type", "application/json") rec := httptest.NewRecorder()Apply to the 7 new
TestSetup_PAT_*cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/http/handlers/instance/instance_handler_test.go` around lines 314 - 528, The new PAT-related tests (TestSetup_PAT_FeatureDisabled_IgnoresCreatePAT, TestSetup_PAT_FlagOmitted_NoPAT, TestSetup_PAT_MissingExpireIn_DefaultsToOneDay, TestSetup_PAT_ExpireOutOfRange, TestSetup_PAT_Success, TestSetup_PAT_AccountCreationFails_Rollback, TestSetup_PAT_CreatePATFails_Rollback) omit the Content-Type header; update each test where the request is built to set req.Header.Set("Content-Type", "application/json") before calling router.ServeHTTP so they match the existing TestSetup_* cases and remain robust if the handler enforces content negotiation or middleware checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@management/server/instance/setup_service.go`:
- Around line 121-126: The rollback path can miss cleaning up an orphaned
account if GetAccountIDByUserID returns an error (it may have created the
account but failed to write IDP meta), so either capture and propagate the
created account ID right after SaveAccount (so the caller can call
rollbackSetup(ctx, userData.ID, ..., accountID)) or change rollbackSetup to
attempt an account lookup by user when accountID == "" (use
accountManager.FindAccountByUserID or an equivalent non-creating lookup instead
of a create-on-read call) and then invoke rollbackSetupAccount when found;
update references in setup_service.go around GetAccountIDByUserID and in
rollbackSetup/rollbackSetupAccount/instanceManager.RollbackSetup accordingly.
---
Nitpick comments:
In `@management/server/http/handlers/instance/instance_handler_test.go`:
- Around line 314-528: The new PAT-related tests
(TestSetup_PAT_FeatureDisabled_IgnoresCreatePAT,
TestSetup_PAT_FlagOmitted_NoPAT, TestSetup_PAT_MissingExpireIn_DefaultsToOneDay,
TestSetup_PAT_ExpireOutOfRange, TestSetup_PAT_Success,
TestSetup_PAT_AccountCreationFails_Rollback,
TestSetup_PAT_CreatePATFails_Rollback) omit the Content-Type header; update each
test where the request is built to set req.Header.Set("Content-Type",
"application/json") before calling router.ServeHTTP so they match the existing
TestSetup_* cases and remain robust if the handler enforces content negotiation
or middleware checks.
In `@management/server/instance/setup_service.go`:
- Around line 99-113: Move the check for m.accountManager being nil so we
validate preconditions before creating an IdP user: ensure the accountManager
nil check (and the error + rollbackSetup call currently below CreateOwnerUser)
is performed before calling m.instanceManager.CreateOwnerUser(ctx, email,
password, name) (or alternatively assert accountManager in NewSetupService);
this prevents creating the owner user when opts.CreatePAT is true and
accountManager is missing—update the logic around opts.CreatePAT,
CreateOwnerUser, accountManager, and rollbackSetup to fail fast without mutating
IdP state.
- Around line 16-30: Replace the duplicated local bounds patMinExpireDays and
patMaxExpireDays with a single source of truth by exporting public constants
from the user package and using them here: add exported constants (e.g.,
PAT_MIN_EXPIRE_DAYS and PAT_MAX_EXPIRE_DAYS) in the same file that defines
DefaultAccountManager.CreatePAT (where the current 1 and 365 checks live),
update CreatePAT to reference those exported names if needed, then remove the
local patMinExpireDays/patMaxExpireDays definitions in setup_service.go and
import/use the exported PAT_MIN_EXPIRE_DAYS and PAT_MAX_EXPIRE_DAYS constants
instead (also update imports).
In `@shared/management/http/api/openapi.yml`:
- Around line 3455-3458: The OpenAPI spec documents a plaintext
personal_access_token in the POST /api/setup success response but does not
declare non-cacheable semantics; update the POST /api/setup response definition
in shared/management/http/api/openapi.yml (the response that includes the
personal_access_token field) to add a response header entry declaring
"Cache-Control: no-store" (optionally also "Pragma: no-cache" and "Vary" if
desired) so clients and intermediaries know credentials must not be cached;
ensure the header is added to the exact response object that contains
personal_access_token.
- Around line 3432-3438: Update the OpenAPI description for the pat_expire_in
schema property to state that it only applies when create_pat is true: modify
the description for pat_expire_in to read something like "Expiration of the
Personal Access Token in days (applies only when create_pat is true). Defaults
to 1 day when omitted; valid range 1–365." Keep existing
type/minimum/maximum/default/example and ensure the text explicitly references
create_pat so clients know it's conditional.
🪄 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: fc6e4076-d0bc-4a73-912c-e1dec64cbd6e
📒 Files selected for processing (9)
management/server/http/handler.gomanagement/server/http/handlers/instance/instance_handler.gomanagement/server/http/handlers/instance/instance_handler_test.gomanagement/server/instance/manager.gomanagement/server/instance/manager_test.gomanagement/server/instance/setup_service.gomanagement/server/instance/setup_service_test.goshared/management/http/api/openapi.ymlshared/management/http/api/types.gen.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
management/server/instance/setup_service_test.go (2)
163-200: Strengthen rollback assertions with exact call counts.Both tests assert the rollback user ID, but not whether rollback happened exactly once. Add a counter assertion to avoid false positives if rollback is called multiple times.
Suggested test hardening
func TestSetupOwner_CreatePATFails_AccountAlreadyGoneStillRollsBackUser(t *testing.T) { @@ - rolledBackFor := "" + rolledBackFor := "" + rollbackCalls := 0 setupManager := NewSetupService( &setupInstanceManagerMock{ rollbackSetupFn: func(_ context.Context, userID string) error { + rollbackCalls++ rolledBackFor = userID return nil }, }, @@ assert.Contains(t, err.Error(), "create setup PAT") assert.Equal(t, "owner-id", rolledBackFor) + assert.Equal(t, 1, rollbackCalls) } func TestSetupOwner_CreatePATFails_AccountRollbackFailureStillRollsBackUser(t *testing.T) { @@ - rolledBackFor := "" + rolledBackFor := "" + rollbackCalls := 0 setupManager := NewSetupService( &setupInstanceManagerMock{ rollbackSetupFn: func(_ context.Context, userID string) error { + rollbackCalls++ rolledBackFor = userID return nil }, }, @@ assert.Contains(t, err.Error(), "create setup PAT") assert.Equal(t, "owner-id", rolledBackFor) + assert.Equal(t, 1, rollbackCalls) }Also applies to: 202-241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/instance/setup_service_test.go` around lines 163 - 200, The test currently verifies the rolledBackFor user ID but not how many times rollback was invoked; modify TestSetupOwner_CreatePATFails_AccountAlreadyGoneStillRollsBackUser (and the other test in lines 202-241) to track invocation count: replace the simple rolledBackFor string capture used in setupInstanceManagerMock.rollbackSetupFn with a small counter (e.g., rollbackCount int) and set rolledBackFor on first call, increment the counter on each call; after calling setupManager.SetupOwner assert rollbackCount == 1 and assert rolledBackFor == "owner-id" to ensure rollback occurred exactly once.
115-241: Consider adding tests for the two remaining PAT rollback branches.Given
SetupOwnerhas dedicated rollback paths for(1) nil account managerand(2) account ID resolution failure, adding explicit tests for those branches would complete branch-level coverage of the new orchestration logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/instance/setup_service_test.go` around lines 115 - 241, Add two tests that exercise the remaining PAT rollback branches in SetupOwner: (1) when the AccountManager passed to NewSetupService is nil, and (2) when AccountManager.GetAccountIDByUserId returns an error (account ID resolution failure). For each test set SetupPATEnabledEnvKey="true", call setupManager.SetupOwner(...) with CreatePAT true and PATExpireInDays, and verify the call returns an error containing "create setup PAT" and that the setupInstanceManagerMock.rollbackSetupFn was invoked for the owner user id (e.g., assert rollback called with "owner-id" or captured variable equals "owner-id"); for the GetAccountIDByUserId-failure test, also stub GetStore/GetAccount expectations as needed to avoid unrelated failures. Ensure tests are named to match existing pattern (e.g., TestSetupOwner_CreatePATFails_NilAccountManager_RollsBackUser and TestSetupOwner_CreatePATFails_AccountIDResolutionFails_RollsBackUser).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@management/server/instance/setup_service_test.go`:
- Around line 163-200: The test currently verifies the rolledBackFor user ID but
not how many times rollback was invoked; modify
TestSetupOwner_CreatePATFails_AccountAlreadyGoneStillRollsBackUser (and the
other test in lines 202-241) to track invocation count: replace the simple
rolledBackFor string capture used in setupInstanceManagerMock.rollbackSetupFn
with a small counter (e.g., rollbackCount int) and set rolledBackFor on first
call, increment the counter on each call; after calling setupManager.SetupOwner
assert rollbackCount == 1 and assert rolledBackFor == "owner-id" to ensure
rollback occurred exactly once.
- Around line 115-241: Add two tests that exercise the remaining PAT rollback
branches in SetupOwner: (1) when the AccountManager passed to NewSetupService is
nil, and (2) when AccountManager.GetAccountIDByUserId returns an error (account
ID resolution failure). For each test set SetupPATEnabledEnvKey="true", call
setupManager.SetupOwner(...) with CreatePAT true and PATExpireInDays, and verify
the call returns an error containing "create setup PAT" and that the
setupInstanceManagerMock.rollbackSetupFn was invoked for the owner user id
(e.g., assert rollback called with "owner-id" or captured variable equals
"owner-id"); for the GetAccountIDByUserId-failure test, also stub
GetStore/GetAccount expectations as needed to avoid unrelated failures. Ensure
tests are named to match existing pattern (e.g.,
TestSetupOwner_CreatePATFails_NilAccountManager_RollsBackUser and
TestSetupOwner_CreatePATFails_AccountIDResolutionFails_RollsBackUser).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8cde55d3-efa2-467e-8a9d-9f83af65cf1d
📒 Files selected for processing (1)
management/server/instance/setup_service_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
management/server/instance/setup_service.go (1)
131-174:⚠️ Potential issue | 🟠 MajorAvoid using the create-on-read account lookup during rollback.
lookupSetupAccountIDForRollbackstill goes throughGetAccountIDByUserID; if that path creates or mutates account state, this rollback can reintroduce the orphan-account problem instead of fixing it. Also, rollback failures are only logged, so the caller may still retry against partially provisioned state without knowing cleanup was incomplete.🔎 Verification script
#!/bin/bash # Confirm whether GetAccountIDByUserID is a pure lookup or a create-on-read path, # and whether a read-only alternative already exists for rollback use. rg -nP --type=go -C4 '\bGetAccountIDByUserID\b|\bGetOrCreateAccountByUser\b|\bFindAccountByUserID\b|\bLookupAccount.*UserID\b' management/server🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/instance/setup_service.go` around lines 131 - 174, lookupSetupAccountIDForRollback calls accountStore.GetAccountIDByUserID which may be a create-on-read path; change it to use a read-only lookup (or add one) and make rollback failures visible to the caller: replace the call to GetAccountIDByUserID in lookupSetupAccountIDForRollback with a pure-read method (e.g. FindAccountByUserID or GetAccountIDByUserIDReadOnly on accountStore) that does not mutate state, and if no such method exists add it to the account store interface and implement it; also change rollbackSetup to propagate rollback errors (return an error) instead of only logging so callers can detect incomplete cleanup (update rollbackSetupAccount and instanceManager.RollbackSetup error handling to return the combined/first failure to the caller).
🧹 Nitpick comments (1)
shared/management/http/api/openapi.yml (1)
3455-3458: Mark the returned PAT as sensitive.
personal_access_tokenis a secret-bearing field, but it is currently documented as a plain string. Marking it as a password-style field will help generated docs and clients handle it more safely.♻️ Proposed tweak
personal_access_token: description: Plain text Personal Access Token created during setup. Present only when create_pat was requested and the NB_SETUP_PAT_ENABLED feature was enabled on the server. type: string + format: password example: nbp_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/management/http/api/openapi.yml` around lines 3455 - 3458, The schema property personal_access_token is currently a plain string but should be treated as a secret; update its OpenAPI schema by adding format: password and mark it readOnly: true (since it is returned on creation) so generated docs/clients treat it as sensitive; locate the personal_access_token property in openapi.yml and add those fields to its definition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@management/server/http/handlers/instance/instance_handler_test.go`:
- Around line 501-544: The test TestSetup_PAT_CreatePATFails_Rollback contains
unnecessary/dead expectations on the mock store (GetAccount / DeleteAccount)
that are not reached because CreatePATFunc immediately returns an error; remove
the accountStore expectations and any references to accountStore from the test
setup so the test only asserts rollback behavior via
rollbackSetupFn/rolledBackFor and the CreatePATFunc error path, keeping the
focus on RollbackSetup being called when CreatePAT fails.
- Around line 461-499: The test mixes a mocked nbstore.Store expectation with a
stubbed AccountManager.GetAccountIDByUserIdFunc which already returns an error,
causing the accountStore.EXPECT() to be unused and potentially cause unmet-mock
failures; remove the accountStore.EXPECT() line (and the accountStore creation
if unused) or change accountMgr.GetStoreFunc to return nil so the test only
exercises the GetAccountIDByUserIdFunc error path; update
TestSetup_PAT_AccountCreationFails_Rollback to rely solely on the mocked
accountMgr.GetAccountIDByUserIdFunc and the mockInstanceManager
(createOwnerUserFn/rollbackSetupFn) to validate rollback.
---
Duplicate comments:
In `@management/server/instance/setup_service.go`:
- Around line 131-174: lookupSetupAccountIDForRollback calls
accountStore.GetAccountIDByUserID which may be a create-on-read path; change it
to use a read-only lookup (or add one) and make rollback failures visible to the
caller: replace the call to GetAccountIDByUserID in
lookupSetupAccountIDForRollback with a pure-read method (e.g.
FindAccountByUserID or GetAccountIDByUserIDReadOnly on accountStore) that does
not mutate state, and if no such method exists add it to the account store
interface and implement it; also change rollbackSetup to propagate rollback
errors (return an error) instead of only logging so callers can detect
incomplete cleanup (update rollbackSetupAccount and
instanceManager.RollbackSetup error handling to return the combined/first
failure to the caller).
---
Nitpick comments:
In `@shared/management/http/api/openapi.yml`:
- Around line 3455-3458: The schema property personal_access_token is currently
a plain string but should be treated as a secret; update its OpenAPI schema by
adding format: password and mark it readOnly: true (since it is returned on
creation) so generated docs/clients treat it as sensitive; locate the
personal_access_token property in openapi.yml and add those fields to its
definition.
🪄 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: 7f09c764-2f2b-493f-93c0-e64c6de67489
📒 Files selected for processing (8)
management/server/account/pat.gomanagement/server/http/handlers/instance/instance_handler.gomanagement/server/http/handlers/instance/instance_handler_test.gomanagement/server/instance/setup_service.gomanagement/server/instance/setup_service_test.gomanagement/server/user.goshared/management/http/api/openapi.ymlshared/management/http/api/types.gen.go
✅ Files skipped from review due to trivial changes (1)
- management/server/account/pat.go
🚧 Files skipped from review as they are similar to previous changes (1)
- management/server/http/handlers/instance/instance_handler.go
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
management/server/http/handlers/instance/instance_handler_test.go (2)
420-457: Assert captured email argument in PAT success testLine 427 captures
userAuth.Email, but the test never validates it. Add an assertion to ensure request email is propagated toGetAccountIDByUserId.Diff suggestion
@@ assert.Equal(t, "nbp_plain", *response.PersonalAccessToken) assert.Equal(t, "owner-id", gotAccountArgs.userID) + assert.Equal(t, "admin@example.com", gotAccountArgs.email) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/http/handlers/instance/instance_handler_test.go` around lines 420 - 457, The test captures userAuth.Email into gotAccountArgs.email in the GetAccountIDByUserIdFunc but never asserts it; add an assertion that gotAccountArgs.email equals the request email ("admin@example.com") after the request is served (e.g., using assert.Equal(t, "admin@example.com", gotAccountArgs.email)) so the test validates the email was propagated to GetAccountIDByUserId.
352-392: AssertCache-Control: no-storewhen PAT is returned in default-expiry path
TestSetup_PAT_MissingExpireIn_DefaultsToOneDayvalidates token issuance but misses the cache-control assertion. Since a PAT is present in the response, add the same header check used elsewhere.Diff suggestion
@@ router.ServeHTTP(rec, req) assert.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, "no-store", rec.Header().Get("Cache-Control")) assert.True(t, createCalled) var response api.SetupResponse🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/http/handlers/instance/instance_handler_test.go` around lines 352 - 392, Add an assertion to TestSetup_PAT_MissingExpireIn_DefaultsToOneDay to verify the response includes the Cache-Control: no-store header when a Personal Access Token is returned: after router.ServeHTTP(rec, req) (and/or after decoding response) call rec.Header().Get("Cache-Control") and assert it equals "no-store" (consistent with other tests that check cache control when a PAT is present).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@management/server/http/handlers/instance/instance_handler_test.go`:
- Around line 420-457: The test captures userAuth.Email into
gotAccountArgs.email in the GetAccountIDByUserIdFunc but never asserts it; add
an assertion that gotAccountArgs.email equals the request email
("admin@example.com") after the request is served (e.g., using assert.Equal(t,
"admin@example.com", gotAccountArgs.email)) so the test validates the email was
propagated to GetAccountIDByUserId.
- Around line 352-392: Add an assertion to
TestSetup_PAT_MissingExpireIn_DefaultsToOneDay to verify the response includes
the Cache-Control: no-store header when a Personal Access Token is returned:
after router.ServeHTTP(rec, req) (and/or after decoding response) call
rec.Header().Get("Cache-Control") and assert it equals "no-store" (consistent
with other tests that check cache control when a PAT is present).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45e32979-6f16-4e7b-af08-e4d0ebb36040
📒 Files selected for processing (1)
management/server/http/handlers/instance/instance_handler_test.go
bcmmbaga
requested changes
Apr 29, 2026
Comment on lines
+140
to
+151
| if accountID != "" { | ||
| if err := m.rollbackSetupAccount(ctx, accountID); err != nil { | ||
| log.WithContext(ctx).Errorf("failed to roll back setup account %s for user %s after %s: original error: %v, rollback error: %v", accountID, userID, reason, origErr, err) | ||
| } else { | ||
| log.WithContext(ctx).Warnf("rolled back setup account %s for user %s after %s: %v", accountID, userID, reason, origErr) | ||
| } | ||
| } | ||
|
|
||
| if err := m.instanceManager.RollbackSetup(ctx, userID); err != nil { | ||
| log.WithContext(ctx).Errorf("failed to roll back setup user %s after %s: original error: %v, rollback error: %v", userID, reason, origErr, err) | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
I think we should stop rollback if rollbackSetupAccount fails.
If account deletion fails but we continue to RollbackSetup, we can end up with an existing account whose owner has been deleted from the embedded IdP. That leaves /api/setup non-retryable because setup is blocked whenever any account exists, even if there is no local admin user left
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
Automation requires a way for the user to generate some kind of token they can use to control and populate the instance, this feature introduces the option to enable PAT creation through the
/api/setupendpoint.Now the user will be able to enable PAT creation on setup by:
NB_SETUP_PAT_ENABLED=true"create_pat": true"pat_expire_in": 30to specify the expiration time in days (defaults to 1)As answer they'll receive the same response with an extra field
personal_access_tokenwith the actual token they can use to control the instance.Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
I added/updated documentation for this change
Documentation is not needed for this change (explain why)
OpenAPI schema/response documentation is updated in this PR; no separate docs-site PR is needed.
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
netbirdio/docs#714
Summary by CodeRabbit
New Features
Validation
Reliability
Documentation
Tests