Repository navigation
fix(auth): grant view:config to non-admin groups; disable settings fields for read-only users - #1428
Merged
Merged
Conversation
cristim
added
triaged
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
📝 WalkthroughWalkthroughNon-admin roles now receive ChangesConfiguration permissions
Frontend settings behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant loadAutomationSettings
participant getRIExchangeConfig
participant isPermissionDeniedError
participant AutomationSettingsContainer
loadAutomationSettings->>getRIExchangeConfig: request Exchange Automation config
getRIExchangeConfig-->>loadAutomationSettings: permission-denied error
loadAutomationSettings->>isPermissionDeniedError: classify error
isPermissionDeniedError-->>loadAutomationSettings: true
loadAutomationSettings->>AutomationSettingsContainer: clear content without error or retry
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 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 `@internal/api/handler_config_test.go`:
- Around line 1247-1271: Add mockAuth.AssertExpectations(t) after the
updateConfig assertions so the test verifies the expected HasPermissionAPI call
for "update", "config" was made, while preserving the existing store-not-called
assertions.
In
`@internal/database/postgres/migrations/000088_grant_view_config_to_nonadmin_groups.down.sql`:
- Around line 8-25: Update migration 000088 so the up migration records which
Standard Users and Read-Only Users groups actually received the view:config
grant, and the down migration removes only those recorded grants rather than
every matching permission. Add regression coverage for groups that already had
view:config before the up migration, verifying rollback preserves their
pre-existing permission.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d72e3141-15fd-49c4-b561-6b1ef50c9da5
⛔ Files ignored due to path filters (1)
frontend/src/permissions.generated.tsis excluded by!**/*.generated.*
📒 Files selected for processing (12)
frontend/src/__tests__/permissions.test.tsfrontend/src/__tests__/riexchange-automation-settings.test.tsfrontend/src/__tests__/settings-permissions.test.tsfrontend/src/riexchange.tsfrontend/src/settings.tsinternal/api/handler_config_test.gointernal/auth/service_group_test.gointernal/auth/types.gointernal/auth/types_test.gointernal/database/postgres/migrations/000088_grant_view_config_to_nonadmin_groups.down.sqlinternal/database/postgres/migrations/000088_grant_view_config_to_nonadmin_groups.up.sqlinternal/database/postgres/migrations/000088_grant_view_config_to_nonadmin_groups_test.go
Comment on lines
+1247
to
+1271
| mockAuth.On("ValidateSession", ctx, "user-token").Return(userSession, nil) | ||
| // Grant view:config so the session itself is valid (GET would succeed). | ||
| mockAuth.On("HasPermissionAPI", ctx, userSession.UserID, "view", "config").Return(true, nil).Maybe() | ||
| // Deny update:config — this is the permission PUT /api/config checks. | ||
| mockAuth.On("HasPermissionAPI", ctx, userSession.UserID, "update", "config").Return(false, nil) | ||
| // Deny admin:* so requireAdmin also fails (belt-and-suspenders: the route | ||
| // is AuthAdmin-gated, but the handler adds its own requirePermission check). | ||
| mockAuth.On("HasPermissionAPI", ctx, userSession.UserID, mock.AnythingOfType("string"), mock.AnythingOfType("string")). | ||
| Return(false, nil).Maybe() | ||
|
|
||
| handler := &Handler{config: mockStore, auth: mockAuth} | ||
|
|
||
| req := &events.LambdaFunctionURLRequest{ | ||
| Headers: map[string]string{"Authorization": "Bearer user-token"}, | ||
| Body: `{"default_term": 1}`, | ||
| } | ||
| result, err := handler.updateConfig(ctx, req) | ||
| require.Error(t, err, "updateConfig must be rejected for a user who lacks update:config") | ||
| assert.Nil(t, result) | ||
| ce, ok := IsClientError(err) | ||
| require.True(t, ok, "expected ClientError, got %T: %v", err, err) | ||
| assert.Equal(t, 403, ce.code, "expected 403, got %d", ce.code) | ||
| // The store must never be reached — the permission check fires first. | ||
| mockStore.AssertNotCalled(t, "GetGlobalConfig", mock.Anything) | ||
| mockStore.AssertNotCalled(t, "SaveGlobalConfig", mock.Anything, mock.Anything) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -A55 -B5 'func TestHandler_updateConfig_RequiresUpdateConfigPermission' \
internal/api/handler_config_test.go
rg -n -P -A70 -B10 'func \(.*\) updateConfig\s*\(' internal/apiRepository: LeanerCloud/CUDly
Length of output: 9069
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the authorization helpers and the mock implementation used by this test.
rg -n -A80 -B20 'func \(h \*Handler\) requirePermission|func \(h \*Handler\) requireAdmin|type MockAuthService|HasPermissionAPI' internal/api
# If the mock is in a generated test file, show the relevant section with line numbers.
files=$(git ls-files 'internal/api/*test.go' 'internal/api/*mock*.go' 'internal/api/*_mock.go')
printf '%s\n' "$files" | sed -n '1,120p'Repository: LeanerCloud/CUDly
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect testify/mock expectation matching in the repository's version if vendored code exists.
rg -n -A40 -B10 'func \(.*\) AssertExpectations|func \(.*\) MethodCalled|ExpectedCalls' . \
-g '*/mock*.go' -g '*testify*' -g 'go.mod' -g 'go.sum'Repository: LeanerCloud/CUDly
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- requirePermission / requireAdmin / updateConfig ---'
rg -n -A80 -B20 'func \(h \*Handler\) requirePermission|func \(h \*Handler\) requireAdmin|func \(h \*Handler\) updateConfig' internal/api/handler_config.go internal/api/*.go
echo '--- MockAuthService ---'
rg -n -A120 -B20 'type MockAuthService|HasPermissionAPI|ValidateSession' internal/api/*test.go internal/api/*mock*.go internal/api/*_mock.go
echo '--- testify/mock matching references if present in repo ---'
rg -n -A40 -B10 'AssertExpectations|MethodCalled|ExpectedCalls' . -g '*mock*.go' -g '*test.go' -g 'go.mod' -g 'go.sum'Repository: LeanerCloud/CUDly
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the auth helper and the test mock used in handler_config_test.go.
sed -n '1,220p' internal/api/handler_config.go
echo '---'
sed -n '1,260p' internal/api/handler_config_test.goRepository: LeanerCloud/CUDly
Length of output: 19536
Assert the update:config call
mockAuth.AssertExpectations(t) is missing, so the exact HasPermissionAPI(..., "update", "config") expectation is never enforced and this test can still pass on a different 403 path before the write-permission check.
🤖 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 `@internal/api/handler_config_test.go` around lines 1247 - 1271, Add
mockAuth.AssertExpectations(t) after the updateConfig assertions so the test
verifies the expected HasPermissionAPI call for "update", "config" was made,
while preserving the existing store-not-called assertions.
Comment on lines
+8
to
+25
| UPDATE groups | ||
| SET | ||
| permissions = ( | ||
| SELECT COALESCE( | ||
| jsonb_agg(elem ORDER BY elem->>'action', elem->>'resource'), | ||
| '[]'::jsonb | ||
| ) | ||
| FROM jsonb_array_elements(permissions) AS elem | ||
| WHERE NOT ( | ||
| elem->>'action' = 'view' | ||
| AND elem->>'resource' = 'config' | ||
| ) | ||
| ), | ||
| updated_at = NOW() | ||
| WHERE id IN ( | ||
| '00000000-0000-5000-8000-000000000005', -- Standard Users | ||
| '00000000-0000-5000-8000-000000000006' -- Read-Only Users | ||
| ); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve pre-existing view:config grants on rollback.
Line 16 removes the permission even when the up migration did not add it: Line 26 of the up migration skips groups that already have the grant. Rolling back can therefore revoke a manually pre-existing permission. Record which grants 000088 introduced and remove only those; add an up/down regression test for this case.
🤖 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
`@internal/database/postgres/migrations/000088_grant_view_config_to_nonadmin_groups.down.sql`
around lines 8 - 25, Update migration 000088 so the up migration records which
Standard Users and Read-Only Users groups actually received the view:config
grant, and the down migration removes only those recorded grants rather than
every matching permission. Add regression coverage for groups that already had
view:config before the up migration, verifying rollback preserves their
pre-existing permission.
cristim
added a commit
that referenced
this pull request
Jul 17, 2026
Expand-contract rename of all British-spelled 'cancelled'/'cancellable' variants to US-spelled 'canceled'/'cancelable' across the codebase. - Add migration 000089: adds canceled_by column alongside cancelled_by, widens CHECK constraints to accept both spellings, COALESCE reads both during the expand window (prev version 088) - Rename field CancelledBy->CanceledBy in PurchaseExecution, update json tag to canceled_by; add IsImmediatelyCancelable() predicate - Rename SetCancelledBy->SetCanceledBy in StoreInterface + all implementations and mocks - Update handler_purchases, handler_purchases_revoke and all tests to use US spellings; replace em-dashes with double hyphens in comments - Patch frontend history.ts, riexchange.ts and OpenAPI spec to use 'canceled' status string Rebased onto main (473f69b); migration renumbered from 000082 to 000089 to land after in-flight #808 (000087) and #1428 (000088).
This was referenced Jul 17, 2026
cristim
force-pushed
the
fix/qa-nonadmin-settings-rbac
branch
from
July 17, 2026 08:51
b757853 to
fb736c4
Compare
cristim
added a commit
that referenced
this pull request
Jul 17, 2026
Expand-contract rename of all British-spelled 'cancelled'/'cancellable' variants to US-spelled 'canceled'/'cancelable' across the codebase. - Add migration 000089: adds canceled_by column alongside cancelled_by, widens CHECK constraints to accept both spellings, COALESCE reads both during the expand window (prev version 088) - Rename field CancelledBy->CanceledBy in PurchaseExecution, update json tag to canceled_by; add IsImmediatelyCancelable() predicate - Rename SetCancelledBy->SetCanceledBy in StoreInterface + all implementations and mocks - Update handler_purchases, handler_purchases_revoke and all tests to use US spellings; replace em-dashes with double hyphens in comments - Patch frontend history.ts, riexchange.ts and OpenAPI spec to use 'canceled' status string Rebased onto main (473f69b); migration renumbered from 000082 to 000089 to land after in-flight #808 (000087) and #1428 (000088).
Member
Author
|
Rebase complete onto current main (7f0573d, includes #1436 migration-test-pin fix) Branch fix/qa-nonadmin-settings-rbac was already based on top of origin/main HEAD -- no rebase commits needed. Push was a no-op (up-to-date). Migration 000088 confirmed present. Local gate results:
|
cristim
force-pushed
the
fix/qa-nonadmin-settings-rbac
branch
from
July 17, 2026 12:30
fb736c4 to
f0a9468
Compare
cristim
added a commit
that referenced
this pull request
Jul 17, 2026
Expand-contract rename of all British-spelled 'cancelled'/'cancellable' variants to US-spelled 'canceled'/'cancelable' across the codebase. - Add migration 000089: adds canceled_by column alongside cancelled_by, widens CHECK constraints to accept both spellings, COALESCE reads both during the expand window (prev version 088) - Rename field CancelledBy->CanceledBy in PurchaseExecution, update json tag to canceled_by; add IsImmediatelyCancelable() predicate - Rename SetCancelledBy->SetCanceledBy in StoreInterface + all implementations and mocks - Update handler_purchases, handler_purchases_revoke and all tests to use US spellings; replace em-dashes with double hyphens in comments - Patch frontend history.ts, riexchange.ts and OpenAPI spec to use 'canceled' status string Rebased onto main (473f69b); migration renumbered from 000082 to 000089 to land after in-flight #808 (000087) and #1428 (000088).
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/settings.ts (1)
1266-1302: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winActually retain and replace the previous delete-toast handle.
The comment says each delete replaces an earlier toast, but this code ignores the
ToastHandlereturned byshowToast. SinceshowToastappends every toast, rapid deletes can leave multiple Undo actions visible.Store the handle and dismiss the previous one before creating the next toast.
Suggested adjustment
- showToast({ + activeDeleteToast?.dismiss(); + activeDeleteToast = showToast({Declare
activeDeleteToastat module scope with the appropriateToastHandletype.🤖 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 `@frontend/src/settings.ts` around lines 1266 - 1302, Update the delete flow around showToast to retain the returned ToastHandle and prevent stacked undo toasts. Declare an activeDeleteToast variable at module scope with the existing ToastHandle type, dismiss and replace it before showing a new delete toast, and assign the new handle to it.
🤖 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 `@internal/auth/types.go`:
- Around line 543-551: Update the comment above the ActionView/ResourceConfig
permission to scope the default to Standard users instead of every authenticated
user. State that Read-Only users receive this permission through the separate
default below, while preserving the existing read-only and SourceIdentity
details.
---
Outside diff comments:
In `@frontend/src/settings.ts`:
- Around line 1266-1302: Update the delete flow around showToast to retain the
returned ToastHandle and prevent stacked undo toasts. Declare an
activeDeleteToast variable at module scope with the existing ToastHandle type,
dismiss and replace it before showing a new delete toast, and assign the new
handle to it.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d7f4c00e-ca70-4130-bc32-40c3432a12c9
⛔ Files ignored due to path filters (1)
frontend/src/permissions.generated.tsis excluded by!**/*.generated.*
📒 Files selected for processing (12)
frontend/src/__tests__/permissions.test.tsfrontend/src/__tests__/riexchange-automation-settings.test.tsfrontend/src/__tests__/settings-permissions.test.tsfrontend/src/riexchange.tsfrontend/src/settings.tsinternal/api/handler_config_test.gointernal/auth/service_group_test.gointernal/auth/types.gointernal/auth/types_test.gointernal/database/postgres/migrations/000088_grant_view_config_to_nonadmin_groups.down.sqlinternal/database/postgres/migrations/000088_grant_view_config_to_nonadmin_groups.up.sqlinternal/database/postgres/migrations/000088_grant_view_config_to_nonadmin_groups_test.go
🚧 Files skipped from review as they are similar to previous changes (10)
- internal/auth/service_group_test.go
- internal/database/postgres/migrations/000088_grant_view_config_to_nonadmin_groups.up.sql
- internal/database/postgres/migrations/000088_grant_view_config_to_nonadmin_groups.down.sql
- frontend/src/riexchange.ts
- frontend/src/tests/permissions.test.ts
- internal/api/handler_config_test.go
- internal/database/postgres/migrations/000088_grant_view_config_to_nonadmin_groups_test.go
- internal/auth/types_test.go
- frontend/src/tests/settings-permissions.test.ts
- frontend/src/tests/riexchange-automation-settings.test.ts
…elds for read-only users Root cause for issues #1401, #1410, and #1413: GET /api/config and GET /api/ri-exchange/config both require view:config, but Standard Users and Read-Only Users lacked this permission. The cascade: - #1401: only the section header rendered; the config form was never shown because loadGlobalSettings returned early on 403. - #1410: applyReadOnlySettings was never called, so Purchasing Policies inputs stayed enabled for non-admin sessions. - #1413: a "permission denied" error paragraph appeared inside the Exchange Automation container instead of degrading gracefully. Fix: - internal/auth/types.go: add view:config to DefaultUserPermissions() and DefaultReadOnlyPermissions() (write gate update:config is still admin-only). - migration 000088: SQL UPDATE grants view:config to Standard Users (00000000-...-0005) and Read-Only Users (00000000-...-0006) with an idempotent NOT EXISTS guard; down migration removes it via jsonb_agg. - frontend/src/permissions.generated.ts: regenerated to include view:config in USER_PERMS and READONLY_PERMS. - frontend/src/settings.ts: export isPermissionDeniedError so sibling modules can reuse it without duplicating the detection logic. - frontend/src/riexchange.ts: graceful-degrade on permission denied in loadAutomationSettings (clears container, no error paragraph). Tests: - internal/auth/types_test.go + service_group_test.go: updated counts (11 -> 12 for users, 3 -> 4 for read-only) plus positive view:config and negative update:config assertions. - internal/api/handler_config_test.go: new test asserts updateConfig returns 403 for a session that holds view:config but not update:config. - internal/database/postgres/migrations/000088_..._test.go: integration tests cover grant, write-gate exclusion, sibling preservation, down migration, and idempotency. - frontend/__tests__/permissions.test.ts: updated expected permission sets to include view:config; renamed readonly describe to reflect the 4-permission set. - frontend/__tests__/settings-permissions.test.ts: 6 new tests verify the form is visible (not just the header) after getConfig succeeds for standard and read-only users, and that Purchasing Policies inputs are disabled. - frontend/__tests__/riexchange-automation-settings.test.ts: new file with 5 tests covering permission-denied graceful degradation. Closes #1401 Closes #1410 Closes #1413
cristim
force-pushed
the
fix/qa-nonadmin-settings-rbac
branch
from
July 17, 2026 13:09
f0a9468 to
18674ca
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
cristim
added a commit
that referenced
this pull request
Jul 17, 2026
Expand-contract rename of all British-spelled 'cancelled'/'cancellable' variants to US-spelled 'canceled'/'cancelable' across the codebase. - Add migration 000089: adds canceled_by column alongside cancelled_by, widens CHECK constraints to accept both spellings, COALESCE reads both during the expand window (prev version 088) - Rename field CancelledBy->CanceledBy in PurchaseExecution, update json tag to canceled_by; add IsImmediatelyCancelable() predicate - Rename SetCancelledBy->SetCanceledBy in StoreInterface + all implementations and mocks - Update handler_purchases, handler_purchases_revoke and all tests to use US spellings; replace em-dashes with double hyphens in comments - Patch frontend history.ts, riexchange.ts and OpenAPI spec to use 'canceled' status string Rebased onto main (473f69b); migration renumbered from 000082 to 000089 to land after in-flight #808 (000087) and #1428 (000088).
cristim
added a commit
that referenced
this pull request
Jul 17, 2026
… spelling) (#1277) * fix(db): rename cancelled->canceled (expand-contract, migration 000089) Expand-contract rename of all British-spelled 'cancelled'/'cancellable' variants to US-spelled 'canceled'/'cancelable' across the codebase. - Add migration 000089: adds canceled_by column alongside cancelled_by, widens CHECK constraints to accept both spellings, COALESCE reads both during the expand window (prev version 088) - Rename field CancelledBy->CanceledBy in PurchaseExecution, update json tag to canceled_by; add IsImmediatelyCancelable() predicate - Rename SetCancelledBy->SetCanceledBy in StoreInterface + all implementations and mocks - Update handler_purchases, handler_purchases_revoke and all tests to use US spellings; replace em-dashes with double hyphens in comments - Patch frontend history.ts, riexchange.ts and OpenAPI spec to use 'canceled' status string Rebased onto main (473f69b); migration renumbered from 000082 to 000089 to land after in-flight #808 (000087) and #1428 (000088). * fix(db): restore correct cancel error paths lost in rebase conflict The rebase of 000089 onto current main incorrectly resolved two conflicts in handler_purchases.go: 1. cancelOrRecoverExecution: reverted fmt.Errorf (router->500) back to NewClientError(409,...), misclassifying retriable backend faults as caller faults (feedback_http_status_classification). 2. cancelPurchaseViaSession: took main's broad !IsCancelable() guard instead of the PR's narrower guardImmediatelyCancelable, allowing "scheduled" executions through the pending/notified-only CAS path and producing a misleading "concurrent operation" 409 instead of the clear "use the revoke endpoint" message. Fix: restore fmt.Errorf for the backend-failure branch; add guardCancelableViaSession that explicitly routes "scheduled" to the revoke endpoint before falling through to IsCancelable; rename residual cancelledBy -> canceledBy; remove extra blank line in types.go. Regression tests: TestHandler_deletePlannedPurchase_BackendErrorReturns5xx and TestHandler_cancelPurchase_Session_ScheduledRoutedToRevoke now pass.
This was referenced Jul 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three cascading bugs all rooted in a single missing permission grant: Standard Users and Read-Only Users lacked
view:config, soGET /api/configandGET /api/ri-exchange/configreturned 403 for them.loadGlobalSettingsreturned early on 403 before callingshowForm()).applyReadOnlySettingswas never called when the initial fetch failed with 403. Fields are now disabled at render time and the backend rejectsPUT /api/configwithoutupdate:config(admin-only).loadAutomationSettingsnow degrades gracefully (clears container, no error) whenisPermissionDeniedErrorreturns true.Changes
Backend / data
internal/auth/types.go: addview:configtoDefaultUserPermissions()andDefaultReadOnlyPermissions().update:configremains admin-only viaadmin:*.internal/database/postgres/migrations/000088_grant_view_config_to_nonadmin_groups.{up,down}.sql: SQLUPDATEwith idempotentNOT EXISTSguard for groups00000000-...-0005and00000000-...-0006.Frontend
frontend/src/permissions.generated.ts: regenerated to includeview:configinUSER_PERMSandREADONLY_PERMS.frontend/src/settings.ts: exportisPermissionDeniedErrorfor reuse by sibling modules.frontend/src/riexchange.ts: graceful-degrade on permission-denied inloadAutomationSettings.Tests (fail-before / pass-after)
internal/auth/types_test.go: updated counts (11->12 user, 3->4 read-only) + explicitview:config/update:configassertions.internal/auth/service_group_test.go: updated three count assertions and the "missing groups" test.internal/api/handler_config_test.go: new test —view:configholder withoutupdate:configgets 403 on PUT.internal/database/postgres/migrations/000088_..._test.go: integration tests (grant, write-gate exclusion, sibling preservation, down migration, idempotency).frontend/__tests__/permissions.test.ts: updated expected sets to includeview:config; renamed readonly describe.frontend/__tests__/settings-permissions.test.ts: 6 new tests — form visible + inputs disabled after successfulgetConfigfor standard/read-only users; Purchasing Policies fields disabled; Save/Reset hidden.frontend/__tests__/riexchange-automation-settings.test.ts: 5 new tests for permission-denied graceful degradation vs real errors.Test plan
go test ./...passes (5728 tests)golangci-lint run ./...(v2.10.1) passesgocyclo -over 10 .produces no new functionsnpm testpasses (2579 tests, 79 suites)npm run buildsucceedsCloses #1401
Closes #1410
Closes #1413
Summary by CodeRabbit