Repository navigation
feat(service-accounts): put service accounts behind the ENABLE_SERVICE_ACCOUNTS feature flag - #1088
Conversation
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
209d7df to
90eb9e6
Compare
1b74ed8 to
a7e21b1
Compare
Review posted for
|
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Code: I found no correctness or security defects on a7e21b1.
- The router-level
Depends(validate_service_accounts_enabled)gates everyservice_accounts_routerroute.test_every_route_is_gatedkeeps that true for routes added later. ENABLE_SERVICE_ACCOUNTSresolves the same way asENABLE_BILLING: the DB row first, then the env var (true/1), thenfalse.- The admin
POST/PATCHendpoints exist with the payloads the new hook sends. They requireMANAGE_FEATURE_FLAGS(super admin only) and invalidate the process cache. - In the frontend, only the
/settings/service-accountspage calls the gated API. Hiding that page throughisSettingsPageHiddentherefore leaves no other UI path that would hit the 403.
All CI checks on this head are green. I re-ran the focused suites locally and they pass:
- Backend (real PostgreSQL):
test_service_accounts.py,test_default_web_client_config_injector.py,test_feature_flag_service.pyandtest_billing.py, 261 passed. - Frontend:
super-admin-instance,use-settings-nav-items,service-accountsandsettings-utils, 84 passed.
Blocking: live UI evidence is missing. This PR adds a new user-visible control (the "Enable Service Accounts (beta)" switch on the Super Admin Instance page). It also changes what the settings nav and the /settings/service-accounts route show. The PR template asks for a video or screenshots for GUI features, and the ticket's Verify step asks the same: as a Super Admin, flip the switch and show that the nav entry and the API follow it with no restart. The PR has "Video/Screenshots: None" and says the UI was not tested end to end in a browser. The curl log and the unit tests support the review, but they cannot replace that capture. Please attach screenshots or a screen recording from a running app (not a mock build) that show:
- The Instance page with the switch off, and the Service Accounts entry missing from the settings nav.
- The switch turned on, the Service Accounts entry appearing, and
/settings/service-accountsloading. - The switch turned off again, with the entry hidden and the page redirecting.
🔄 CHANGES REQUESTED
|
Added the live UI evidence to the PR description under "Video/Screenshots". I tested on a local docker compose stack built from this branch, with a fresh database and |
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
The head has not changed since my last review. The only open item was the blocking request for live UI evidence, and it is now resolved. The PR description has screenshots from a running stack built from this branch, logged in as a real Super Admin:
- The Instance page shows the "Enable Service Accounts (beta)" switch off, and then on.
- With the switch off, the settings nav has no Service Accounts entry.
- With the switch on, the entry appears and
/settings/service-accountsloads ("No service accounts yet").
That covers the production path for the new switch and for the nav and route gating.
Non-blocking observation: the two "switch off again" images (3a and 3b) are byte-identical to the "switch off" images (1a and 1b). The same render of the same state can produce identical files, so they do show the entry hidden again. They do not capture the direct-URL redirect to /settings/user. service-accounts.test.tsx ("redirects an owner away while enable_service_accounts is off") covers that redirect, and the backend 403 is covered by the gate tests, so this does not block.
No new findings. All checks on this head are green.
✅ APPROVED
Gate every service_accounts_router route on the ENABLE_SERVICE_ACCOUNTS database flag (env var fallback, default off), expose it in the web-client config, hide /settings/service-accounts while it is off, and add a Super Admin Instance switch that writes the database flag. Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
…nfig refetch returns The switch was enabled again with the old value while the config refetched. It now shows the saved value at once and stays disabled until the refetch finishes.
…hing the config The switch wrote the saved value, then refetched the web-client config. Each server process caches flags for 5 s, so with more than one replica the refetch could return the old value and show it for the config's 5 minute stale time. The switch now shows the PATCH or POST response and does not refetch. The test for the held refetch is replaced by one where a stale read would come back.
a7e21b1 to
32fab92
Compare
|
🚀 Released in 1.84.0. |
HUMAN:
AGENT:
Why
Service accounts must stay hidden until a Super Admin turns them on.
ENABLE_SERVICE_ACCOUNTSis a database feature flag that falls back to an env var of the same name (defaultfalse), the same wayENABLE_BILLINGworks. The switch on the Super Admin Instance page writes the flag, so no redeploy is needed.Summary
ENABLE_SERVICE_ACCOUNTSis now in_ENV_FLAG_DEFAULTS. The newvalidate_service_accounts_enabled()dependency onservice_accounts_routerreturns 403 on every/api/organizations/{org_id}/service-accounts*route while the flag is off. It is a copy ofvalidate_billing_enabled, and the 403 detail names the flag and the env var.WebClientFeatureFlags.enable_service_accountstakes the env value as a fallback in_get_feature_flags.get_web_client_configthen resolves it again with_resolve_flag('ENABLE_SERVICE_ACCOUNTS', ...), so a database row wins over the env var.isSettingsPageHiddenhides/settings/service-accounts(both the route loader and the nav entry) unlessenable_service_accountsis true.adminServicegetscreateFeatureFlagandupdateFeatureFlag.useSetServiceAccountsEnabled. It sendsPATCH, and if that returns 404 it sendsPOST. On success it writes the saved value into the web-client config query and does not refetch it, because each server process caches flags for 5 s and a refetch could return the old value.SettingsSwitchwithtestId="super-admin-instance-enable-service-accounts". The switch is disabled while a save is in progress.SUPER_ADMIN$INSTANCE_ENABLE_SERVICE_ACCOUNTS("Enable Service Accounts (beta)").How each acceptance criterion is met
service_accounts_routerroute returns 403Depends(validate_service_accounts_enabled)test_routes_return_403_when_flag_is_offchecks all 10 routes.test_every_route_is_gatedcompares the tested routes with the router's by method and path, so a new route fails until it is added./settings/service-accountsand its nav entry are hiddenisSettingsPageHiddensettings-utils.test.ts,use-settings-nav-items.test.tsx("hides service accounts while enable_service_accounts is off"),service-accounts.test.tsx("redirects an owner away while enable_service_accounts is off")feature_flag_service.resolvetest_routes_work_when_env_var_is_on[true/1],test_routes_work_when_database_row_is_on,test_database_row_wins_over_env_var. These run the real flag service against PostgreSQL. All existing route tests still pass with the gate open.GET /api/v1/web-client/configreturnsfeature_flags.enable_service_accounts, from the database row first, then the env vardefault_web_client_config_injector.pyTestEnableServiceAccounts(real database):test_env_var_when_no_row,test_database_row_wins,test_toggle_takes_effect_without_restartSuperAdminInstance+useSetServiceAccountsEnabledsuper-admin-instance.test.tsx: label and value shown,PATCHsent on toggle,POSTsent whenPATCHreturns 404, switch disabled while savingtest_toggle_takes_effect_without_restartIssue Number
OHE-3795
How to Test
These are the commands I ran on this branch, with their results:
uv run pre-commit run --config ./dev_config/python/.pre-commit-config.yaml(on the changed files): ruff, ruff format and mypy all passed.uv run pytest -n 4 --dist worksteal ./tests/unit: 9027 passed.uv run pytest tests/unit/server/routes/test_service_accounts.py tests/unit/app_server/test_default_web_client_config_injector.py tests/unit/server/services/test_feature_flag_service.py tests/unit/test_billing.py: all passed.cd frontend && npm run lint:fix && npm run lint(this includestsc): passed, with 0 errors.cd frontend && npm run build: passed.cd frontend && npx vitest run: 3260 passed across 360 test files (2 skipped), with 0 failures.ProgressEvent is not defined" errors from MSW's XHR interceptor during test teardown.feat/ohe-3713-service-accounts-uigave 3 such errors (fromapp-settings,agent-settingsandinteractive-chat-box).service-accounts.test.tsx. That file passed with no error when run on its own, twice.Local run (switch on → off → on, one server process, no restart)
Setup:
make local-db, migrated to head.feature_flag_router,service_accounts_router,FeatureFlagServiceandDefaultWebClientConfigInjector.In the frontend, the nav entry and the route follow
feature_flags.enable_service_accountsthroughisSettingsPageHidden. The mutation writes the saved value into the config query, so the page picks up the change without a reload.I also tested the UI end to end in a browser, on a local docker compose stack built from this branch. The stack had a fresh database and
ENABLE_SERVICE_ACCOUNTSwas not set. I logged in as a Super Admin and did not restart between steps. See the screenshots below.Video/Screenshots
1. Switch off (default). The settings nav has no Service Accounts entry, and the API returns 403.
2. Switch on.
PATCHreturns 404, thenPOSTreturns 201. The entry appears and/settings/service-accountsloads (API 200).3. Switch off again.
PATCHreturns 200. The entry is hidden, and/settings/service-accountsredirects to/settings/user(API 403).Type
Notes
ENABLE_SERVICE_ACCOUNTS=true._get_feature_flagsuses_env_flag_enabled(acceptstrueor1). The ticket text used== 'true', but the repo's AGENTS.md rule requires accepting both values. This also matches what the backend gate accepts.ENABLE_BILLINGworks the same way.createMockWebClientConfigdefault is unchanged (enable_service_accountsis absent, so the flag reads as off). The existing service-account route tests and nav tests now turn the flag on explicitly.This PR was created by an AI agent (OpenHands) on behalf of the ticket author.
@henriquehirako can click here to continue refining the PR
Enterprise server image for this PR: