villebro commented on code in PR #43911:
URL: https://github.com/apache/superset/pull/43911#discussion_r3938598130
##########
superset-frontend/src/features/tasks/types.ts:
##########
@@ -69,6 +69,8 @@ export interface TaskPrivateProperties {
};
// Freeform task-type-specific handles (e.g.
cancel_query_id/cancel_database_id).
task?: Record<string, unknown>;
+ // Subscription-policy bookkeeping (e.g. chart-data's per-tab consumer list).
+ subscription?: Record<string, unknown>;
Review Comment:
Nit (non-blocking): the frontend mirror gets the `subscription` namespace
here, but the canonical Python type wasn't given the matching update.
`superset-core/src/superset_core/tasks/types.py` `PrivateProperties` still
declares only `framework` and `task`, and its docstring still says *"Split into
two structurally isolated namespaces."* — even though `subscription.py` in the
same PR now tells readers to *"see
`superset_core.tasks.types.PrivateProperties`"* for this namespace.
Runtime is fine (the merge is generic and writes go through a `cast`), so
this is purely type-contract/doc accuracy. Suggest adding `subscription:
dict[str, Any]` to `PrivateProperties` and updating the docstring to "three ...
namespaces". (Tiny related nit: `merge_properties` in `superset/tasks/utils.py`
still says "clobbers the other" — singular — now three namespaces.)
##########
superset/daos/tasks.py:
##########
@@ -721,6 +722,64 @@ def get_required_by_uuids(cls, task_uuid: UUID) ->
list[UUID]:
)
return [required_by_uuid for (required_by_uuid,) in rows]
+ @classmethod
+ def _with_current_subscription_state(
+ cls, task_uuid: UUID, properties: TaskProperties
+ ) -> TaskProperties:
+ """Return ``properties`` with ``private.subscription`` as it is on the
row.
+
+ Reads the row under ``FOR UPDATE`` so the subsequent UPDATE and a
+ concurrent policy write (:meth:`merge_subscription_state`, which takes
the
+ same lock) serialize instead of racing; a missing row yields the input
+ unchanged and the caller's UPDATE then matches nothing.
+ """
+ from superset.tasks.utils import preserve_subscription_state
+
+ current_raw = (
+ db.session.query(Task.properties)
+ .filter(Task.uuid == task_uuid)
+ .with_for_update()
+ .scalar()
+ )
+ if current_raw is None:
+ return properties
+ return preserve_subscription_state(properties,
parse_properties(current_raw))
+
+ @classmethod
+ def merge_subscription_state(cls, task: Task, updates: dict[str, Any]) ->
None:
Review Comment:
✅ Confirmed correct. The safety of this hinges on the executor never holding
the row lock for long, and it doesn't: `set_properties_and_payload` /
`conditional_status_update` are called from `InternalUpdateTaskCommand` /
`InternalStatusTransitionCommand`, each a short `@transaction` that commits per
write, so the `FOR UPDATE` here and there serialize on the single task row
without either side holding it across the task run. Policy hooks additionally
hold `task_lock(dedup_key)`, so concurrent RMW of the consumer list is
serialized too. No cross-lock deadlock since the executor path never waits on
`task_lock`.
##########
superset/tasks/guest.py:
##########
@@ -39,24 +39,28 @@ def get_current_guest_subscriber_key() -> str | None:
``None`` when the request is not an embedded guest (an authenticated user
subscribes by ``user_id`` instead). The key is an HMAC over the guest
token's
stable identifying claims, keyed with the app ``SECRET_KEY`` so it is
- unguessable to outside callers and reproducible for the same token across
the
- request that schedules a task and the polls that await it.
+ unguessable to outside callers and reproducible across the request that
+ schedules a task and the polls that await it, including polls made with a
+ refreshed token carrying the same scope.
"""
guest_user = security_manager.get_current_guest_user_if_guest()
if not guest_user:
return None
token = guest_user.guest_token
# Bind the key to every authorization-relevant claim so two tokens that
differ
# in their effective access scope derive different keys (and can't see each
- # other's tasks): ``iat``/``exp`` pin it to a single issuance,
``resources``/
- # ``datasets``/``rev`` to the granted resources, and ``rls_rules`` to the
- # row-level scope.
+ # other's tasks): ``resources``/``datasets``/``rev`` to the granted
resources
+ # and ``rls_rules`` to the row-level scope. Issuance claims
(``iat``/``exp``)
+ # are deliberately left out: the embedded SDK re-issues the token on a
fixed
Review Comment:
✅ Reasoning holds. Dropping `iat`/`exp` doesn't widen access: the retained
claims (`user`/`resources`/`aud`/`datasets`/`rev`/`rls_rules`) fully capture
authorization scope, so two tokens sharing a key are the same principal with
identical entitlements — which is exactly what SHARED-scope dedup already
assumes. Token expiry is still enforced at auth, independent of this key.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
