Make REDASH_DISABLE_PUBLIC_URLS the source of truth over stale org settings - #7792
Make REDASH_DISABLE_PUBLIC_URLS the source of truth over stale org settings#7792ekanshul wants to merge 1 commit into
Conversation
Greptile SummaryThe PR makes an explicitly configured
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| redash/models/organizations.py | Updates setting lookup precedence so explicitly environment-controlled keys bypass stale organization values. |
| redash/settings/organization.py | Defines the environment-controlled organization-setting keys based on explicitly present environment variables. |
| tests/handlers/test_settings.py | Covers environment precedence and preservation of API-persisted behavior when no override is configured. |
Reviews (2): Last reviewed commit: "Make explicitly-set REDASH_DISABLE_PUBLI..." | Re-trigger Greptile
| if k in environment_only_org_settings: | ||
| continue |
There was a problem hiding this comment.
Runtime test toggles are discarded
When the sharing Cypress scenarios call updateOrgSettings to enable or disable public URLs, this branch discards the requested value and get_setting ignores any stored value, causing scenarios to run with the unchanged environment setting and fail their state-dependent assertions.
There was a problem hiding this comment.
Good catch — reworked. The env var now takes precedence only when it is explicitly set in the environment; when unset (as in CI), stored values and the settings endpoint behave exactly as before, so the Cypress sharing/embed specs that call updateOrgSettings({ disable_public_urls: ... }) are unaffected.
| if k in environment_only_org_settings: | ||
| continue |
There was a problem hiding this comment.
Audit records ignored setting changes
When an administrator saves General Settings, this branch excludes disable_public_urls from persistence and previous_values, but the later audit event records the unfiltered request as new_values, causing the audit trail to report a settings change that was never applied.
There was a problem hiding this comment.
Resolved by the rework: the settings endpoint's persistence and audit behavior is back to unchanged — the override now only affects read precedence in Organization.get_setting(), and only when the environment variable is explicitly set.
…ttings disable_public_urls is not editable in the admin UI, but the General Settings form posts back every value it received from GET /api/settings/organization, so the env-derived value gets persisted into organizations.settings. From then on the stored copy shadowed the environment variable, and flipping REDASH_DISABLE_PUBLIC_URLS had no effect. Give the environment variable precedence in Organization.get_setting(), but only when it is explicitly present in the environment. When the variable is unset, stored values keep working exactly as before, so API consumers that toggle the setting at runtime (e.g. the Cypress sharing specs) are unaffected, and the settings endpoint's persistence and audit behavior is unchanged. Fixes getredash#7630 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
484227c to
a39e42e
Compare
What type of PR is this?
Description
Fixes #7630.
REDASH_DISABLE_PUBLIC_URLS=truehas no effect when theorganizations.settingsJSON already contains adisable_public_urlskey, becauseOrganization.get_setting()prefers the stored value.How the stale value gets into the DB in the first place:
disable_public_urlsis not editable anywhere in the admin UI, but the General Settings form posts back every value it received fromGET /api/settings/organization(seeuseOrganizationSettings.jsx—OrgSettings.save(currentValues)). So the first time an admin hits Save, the then-current env default is silently persisted, and from that point on the environment variable is dead for this key.The fix: when
REDASH_DISABLE_PUBLIC_URLSis explicitly set in the environment, it takes precedence over any value stored in the organizations table (Organization.get_setting()skips the stored value). This neutralizes stale rows written by older versions without any DB surgery, and matches the issue's expectation that a setting not editable via the UI follows the environment variable.When the variable is not set, nothing changes: stored values keep working,
POST /api/settings/organizationpersists them as before, and the audit trail is untouched. In particular the Cypress sharing/embed specs, which toggledisable_public_urlsthrough the API at runtime (cy.updateOrgSettings({ disable_public_urls: ... })), are unaffected since CI doesn't set the variable.The override set is computed in
redash/settings/organization.py(env_overrides) and is easy to extend to other env-only settings if the need arises.How is this tested?
New tests in
tests/handlers/test_settings.py:test_env_override_wins_over_stored_value— with the override active, a storeddisable_public_urls: trueno longer shadows the env value.test_stored_value_applies_without_env_override— without the env var, posting the setting through the API still works (guards the Cypress flows).pytest tests/handlers/test_settings.py tests/handlers/test_dashboards.pypasses locally;ruff checkandblack --checkare clean.Related Tickets & Documents
#7630
Mobile & Desktop Screenshots/Recordings (if there are UI changes)
N/A (backend only; the Share button simply follows the env var again)