* EMB-2341 Don't honor features from an invalid token status
The store's 403 for an expired token still includes the full feature
list in its body, and since the token-grace-period refactor (#64938)
we never checked :valid — we just keyed off the features present. So
an expired token kept all premium features active (e.g. embed
branding stayed hidden).
The design principle: store faithfully what the store returned, and
enforce :valid at the point of access. Provenance is structural, not
data — the fetch layer either returns an authoritative answer (any
response with a parseable body, including a definitive 4xx rejection)
or throws for anything transient, so whatever reaches the caches is
holdable by construction.
- *token-features* returns #{} unless the stored status is :valid
- canonically-has-feature? answers a definitive false for a
canonical-but-invalid token
- a 4xx with no body now throws (like 5xx) instead of returning a
synthetic non-canonical status: previously one bodyless 4xx from an
intermediary would be cached fresh and drop features for up to the
12h soft TTL, defeating the grace period; real 4xx answers (expired,
does not exist) still replace the cache immediately, as they should
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A2yeC4Tzjnk2NWeKxDZYYS
* EMB-2341 Restore legible timeout errors; specify transient-failure behavior
A hung token-check request surfaced as "Cannot throw exception because
the return value of Throwable.getCause() is null": Diehard's
TimeoutExceededException has no cause, so the FailsafeException unwrap
threw nil (an NPE). It still failed safe, but the admin-visible error
was gibberish. Catch the timeout explicitly and restore the pre-64938
"Token validation timed out." message; guard the generic unwrap against
other causeless FailsafeExceptions while we're at it.
Test coverage for previously unspecified behavior, all transient
(thrown, never cached, grace period preserved) except the last:
- hung request: times out legibly, not cached (replaces the dead
:timeout stub in token-checker-test that was never exercised)
- unparseable body (a proxy's HTML error page): not cached
- 2xx with no body: not cached
- 2xx with a :valid false verdict: authoritative and cached, same as
a definitive 4xx
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A2yeC4Tzjnk2NWeKxDZYYS
* EMB-2341 Harden the authoritative/transient boundary on token checks
- A response body that is missing, unparseable (e.g. a proxy's HTML error
page), or not a JSON object is never a verdict: throw a legible error
instead of relying on schema validation (which is disabled in prod) or
leaking a raw Jackson parse error.
- A bare 403/404 with no body IS a verdict: the store may reject a token
without elaboration, so synthesize an authoritative invalid status and
cache it rather than refetching forever.
- do-refresh! now refuses to cache anything not marked :canonical?,
enforcing in code the invariant the comments described.
- Setting a token busts the cache first, so re-entering a just-renewed
token validates against the store instead of a stale expired verdict.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012tetFZMMxsgj7NBQgABraX
* EMB-2341 Negative-cache transient failures; widen breaker window
Move the error boundary inside the local memoize: failure maps now
memoize for local-ttl like any value, so a store outage costs one
network attempt per 5s per instance instead of one per feature check,
and concurrent misses share a single attempt (core.memoize
single-flights values, but not thrown exceptions). Synthetic maps still
never reach the durable db-hash cache, which sits inside the boundary.
Widen the circuit breaker window 60s -> 180s: 10 worst-case failures
(10s timeout + 5s negative cache apiece) span ~135s, so a 60s window
could never accumulate 10 slow failures and the breaker would never
open against a slow-failing store.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012tetFZMMxsgj7NBQgABraX
* Gate Data Analysts membership additions on advanced-permissions
Adding someone to the Data Analysts magic group now requires the
`:advanced-permissions` premium feature.
The check lives at the model layer, on `add-users-to-groups!` -- that
function inserts via raw HoneySQL and so never fires Toucan's
before-insert hook, and direct `t2/insert!` on the model is refused
outright, which makes it the one place every production caller passes
through. That covers the membership API, the `is_data_analyst`
user-update path, invites, LDAP/SSO sync (whose per-add catch already
logs and continues, so logins never break over it) and SCIM (which
surfaces the 402 to the identity provider).
Only additions are gated this way - after downgrade, the group can only shrink.
* Make the SCIM group PUT membership update diff-based
PUT /Groups/:id cleared the group and re-added the whole desired member
list. On a downgraded instance with grandfathered Data Analysts that
made every push fail: a removal-only push, and even an idempotent
resync, re-added members who were already there, tripped the
addition gate, 402'd and rolled back -- so a member could never be
removed via SCIM.
Apply the difference instead: remove (current - desired), add (desired -
current). A removal-only push and a resync now touch no additions and
succeed without the feature, while a push introducing a genuinely new
member still 402s so the drift is visible at the IdP. It also stops
churning membership row ids for members who never changed.
Group PATCH has no membership path -- only /Users/:id -- so PUT and the
POST create path were the only callers.
* Data Analysts group is visible iff advanced-permissions or it has active members
The group's visibility was a straight feature check. This had bad
implications: a user who was still in the group could get permissions
from an "invisible" group that couldn't be seen anywhere in the
permissions graph. We needed to do permissions graph surgery (basically
removing the group in OSS or when downgraded) to prevent this bad state.
Now we don't hide the group unless it's empty. This eliminates the
"hidden permissions" entirely - an empty group doesn't provide any
permissions.
* Render the Data Analysts group removal-only without advanced-permissions
The model-layer gate refuses additions to the Data Analysts group when
advanced-permissions is absent, but the group stays visible while it has
members. Without a matching UI state, every add affordance stayed live and
failed with a 402 on click. Mirror the gate in the frontend instead.
getAddMembersDisabledReason is the single seam: it pairs the group with the
advanced_permissions token feature (read through useHasTokenFeature) and
returns either null or the tooltip text. Removal paths never consult it, so
a grandfathered member can always be taken out -- in MembershipSelect and
GroupsMultiSelect the gate is skipped entirely once the person is already in
the group, which keeps deselect and the pill remove button working. The
gated membership row is also marked aria-disabled, since cursor styling
alone is invisible to assistive tech.
The API-key group picker is included because an API key becomes a member of
its group and so hits the same server-side refusal. Keeping the key's
current group is not an insert, so it is exempt the same way, and
EditApiKeyModal can still save an unchanged key.
GroupDetail wraps its button in a Tooltip only when there is a reason to
disable it, so the with-feature DOM is unchanged. Pinning, sort order, and
the bolt icon key off magic_group_type alone and are untouched; with the
feature present nothing changes.
Frontend code I'm least confident in so this could use a thorough FE
review.
* Gate Data Analyst app access on advanced-permissions
What we want:
- data studio is available to superusers in OSS
- you can also give access to Data Analysts, but only with the necessary feature
We formerly did this by removing all members from the Data Analysts
group on downgrade. This is fragile. Now, we just... gate access on
feature availability as well as `is_data_analyst` (in other words, you
can be a superuser OR you can be a data analyst with the feature
enabled).
The rule for what pauses: a Data Analyst capability survives a downgrade
iff the permissions graph renders it. Curate on the Library collection,
manage-table-metadata on every table, and any assigned data, collection,
or application permissions are durable state and stay put. App access
that appears in no graph exists only because the role does, so it
follows the token.
`is-data-analyst?`: are you a member of the Data Analysts group (or a superuser)?
`entitled-data-analyst?`: the above, plus a feature check in the Data Analysts group case.
Moved to `entitled-data-analyst?`:
- the Data Studio API surfaces (data-model table endpoints, glossary, library,
table publish, transforms listings),
- measure and segment write/create,
- Metabot's write-transforms capability,
- the transform, job, and tag models,
- `enabled-source-types-for-user`, `can_access_transforms` in the current-user
permissions map,
- the user-list authorization,
- and the people-list `can_access_data_studio` filter, where the SQL disjunct
on `is_data_analyst` is dropped when the feature is absent.
- `canAccessDataStudio` and the glossary page apply the same predicate on the
frontend.
Left on role membership: `data_permissions` and its SQL, `collection.clj`,
`permissions/user.clj`, the search context, the dependencies module, and the
request plumbing for the flag. `is_data_analyst` on the current user stays raw
too; the frontend ANDs it with the token.
api.common cannot require premium-features (settings sits between them
and requires api.common back), so the feature read is late-resolved, the
way `metabase.settings.models.setting` reads `has-feature?`.
Tests pin every moved reader in both feature states for a non-admin
analyst, with superusers shown unaffected. Analyst-driven tests that
previously needed only a transforms feature now grant
`:advanced-permissions` alongside it.
Under an OSS build every premium feature is false, so a non-admin
analyst is never entitled there. Tests that assert an analyst succeeds
are EE-only via `mt/when-ee-evailable`; the refusal cases still run in
OSS. The transforms API tests run once per requesting user through
`with-transforms-api-users!`: a superuser in every edition, and a data
analyst when EE is available.
* Delete the OSS startup demotion of the Data Analysts group
Every OSS startup ran a sync routine that mutated permission state: if the
Data Analysts magic group had members, it renamed the group in place with a
"(converted)" suffix, nulled its magic type, minted a fresh empty magic group
under the original name, and cleared `is_data_analyst` instance-wide. Because
a token check can be indeterminate as well as negative, the routine had to be
tri-state, retrying in a background future until it got a canonical answer.
This kind of state-change on startup due to a feature being missing
is... risky. If someone's token expires, it's a big annoyance if they
have to manually copy users and permissions from their old "Data
Analysts" group to the new one.
And the previous commits made it unnecessary:
- you can no longer add new members to the Data Analysts group unless
the feature is enabled.
- almost all permissions of Data Analysts are still active after a
downgrade. They still have curate access to the Library collection,
`manage-table-metadata` permissions on every table, and any data,
collection, or application permissions you assigned. Only Data Studio,
and the app access that exists purely because of the role (writing
measures, segments and transforms), pauses.
- when the last user is removed from the Data Analysts group, it
disappears.
So we can remove it: startup no longer touches permission state, a much
happier place to be.
Deleted: `sync-data-analyst-group-for-oss!`, `do-sync-conversion!`,
`unique-converted-group-name`, `grant-library-permissions!`,
`seconds-to-sleep-per-attempt`, the `*allow-modifying-magic-groups*` escape
hatch that only the conversion used, the `metabase.core.core` startup call, the
`permissions.core` re-export, six now-orphaned `permissions.db` helpers, and
the routine's five tests. `canonically-has-feature?` is untouched; it has other
callers.
* Update docs/people-and-groups/managing.md
Co-authored-by: Jeff Bruemmer <jeff.bruemmer@gmail.com>
* Read data_layer through the schema overlay in the Data Studio access tests
Table edits land in the user-settings overlay since #82246, so a bare
`t2/select` on `:model/Table` reports sync's value. Read it the way the
OSS Data Studio table tests do.
* Reword the Data Analysts add-members tooltip around plans, not feature names
Review feedback: "Advanced Permissions" is not a term the docs use. Match the
docs' framing of a plan that includes the Data Analysts group.
* Pause every role-granted Data Analyst capability on downgrade, not just Data Studio
Review feedback: Data Studio entry paused when the plan lost
advanced-permissions, but Monitor diagnostics, the metric dependencies tab and
relationships section, and analyst-wide visibility in the dependencies API
still keyed off bare group membership.
The rule the PR set out is that capabilities rendered by a permissions graph
survive a downgrade, while capabilities that come with the role alone pause.
Dependency viewing is the latter, so:
- getUserIsEntitledAnalyst (current-user) is the feature-aware selector, and
Data Studio, Monitor diagnostics, and both metric spots use it.
- The dependencies API builds its visibility opts from entitled-data-analyst?.
* Announce every non-clickable membership row as disabled
Review feedback: the click handler and cursor already key off isDisabled, so
aria-disabled should too, not only the feature-gated Data Analysts case.
* Make each membership row a real toggle button
Review feedback: the row was a clickable li, which keyboard and assistive
technology users couldn't operate or interpret. Each row is now an li holding
a full-width button (aria-pressed for membership) with the group-manager
toggle beside it, since a button cannot contain another button. aria-disabled
rather than disabled keeps a gated row focusable and hoverable so the tooltip
can still explain why it can't be toggled.
* Tighten the data-analyst visibility clause docstring
Review suggestion, taken verbatim.
* Name the token-status marker after the flag it sets
Review nit: the helper stamps :canonical?, so call it canonical.
* Leave SCIM group membership alone when no pushed member is a known user
Review feedback: the diff-based PUT emptied a group when every member value
was unknown, where the previous code was a no-op. Restore the no-op and warn,
since the IdP is naming users this instance never provisioned and a silent
wipe would hide that drift behind a 200.
* Make adding an existing group member a no-op
Review feedback asked for a test of PUT /api/user/:id re-asserting
is_data_analyst on someone already in the group after a downgrade. Writing it
showed two failures: a 402 from the Data Analysts addition gate, and, with the
feature on, a 500 from the membership unique index. Both because the add path
never checked whether the row already existed.
add-users-to-groups! now drops pairs that already exist before anything else
runs, so the tenant checks, the addition gate, the insert, the flag updates
and the events only ever see genuine additions. Every caller becomes
idempotent, not just the user PUT.
* Treat a bodyless 4xx token check as transient, not as a rejection
Review feedback: nothing shows the store rejecting a token without a body;
observed 403/404s carry JSON. A proxy or WAF can strip one, though, and caching
that as "token is not valid" while an HTML error page from the same proxy is
treated as transient was the wrong way round. A missing or undecodable body now
always takes the transient path: not cached, retried under the breaker, with
the previous canonical status standing meanwhile.
* Only cache a token-status body that satisfies MinimalTokenStatus
Review feedback: any JSON object, even {}, was stamped canonical and cached.
A body must now satisfy MinimalTokenStatus, a boolean :valid, a non-blank
:status and, when present, a features list, before it counts as the store's
answer. The schema's docstring records why it is used instead of TokenStatus:
one surprising optional field in a real answer would otherwise become a
transient failure refetched forever.
* fix icon alignment, small cleanup
* Enable advanced-permissions for the data-analyst MCP tool tests
A data analyst's segment, measure, and transform writes now key off the
advanced-permissions feature, which no OSS build can have. The three MCP
v2 tool tests that drive an analyst were still running with the feature
off, so the analyst was refused where the test expected success.
Follow the pattern used elsewhere on this branch: run the analyst-positive
paths only with EE available and the feature enabled, keep the denials
unconditional, and assert the analyst is refused once the feature is gone.
* Enable advanced-permissions for the metabot transform read tests
A data analyst's transform reads now key off the advanced-permissions
feature: transform-readable? goes through api/entitled-data-analyst?,
which is false for a Data Analysts member while the feature is off. The
metabot get_transform_details tool tests and the transform resource test
still drove rasta as an analyst with the feature off, so every read was
refused where the test expected the source query to render.
Follow the pattern used elsewhere on this branch and add the feature to
those tests' premium-feature sets. The source-card refusal test gets it
too, so its 403 is about the private card rather than the missing
entitlement.
* Make the analyst-driven metabot transform read tests EE-only
The previous fix turned advanced-permissions on for these tests, but
with-premium-features cannot flip a feature on an OSS build: the
entitlement check bottoms out in config/ee-available?, so rasta as a data
analyst was still refused before the test reached the source-export gate
it pins. The EE jobs passed and only the OSS app-db job failed.
Wrap every deftest that reads a transform as an analyst in
mt/when-ee-evailable, as the MCP tool tests on this branch already do.
That covers the four tests that failed and two more that only passed on
OSS because the refusal they assert happened for the wrong reason.
* Expect Monitor to pause with Data Studio for an unentitled analyst
The app-switcher spec was written when only Data Studio paused on a plan
without advanced-permissions, and asserted Monitor stayed. A later commit
on this branch made Monitor diagnostics key off the entitled-analyst
selector too, so the assertion went stale. Assert both are hidden without
the feature, and both shown with it.
* Find membership popover rows by button role in the group-managers e2e spec
The rows became real buttons without an aria-label, so findByLabelText no
longer matches them. Query by role and name instead, as the unit spec does.
---------
Co-authored-by: dan sutton <dan@dpsutton.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Jeff Bruemmer <jeff.bruemmer@gmail.com>
Co-authored-by: Kevin Kelleher <kevin.kelleher@metabase.com>
d60c8f2e3b