sadpandajoe commented on code in PR #44365:
URL: https://github.com/apache/superset/pull/44365#discussion_r4050260106
##########
superset/security/api.py:
##########
@@ -268,6 +285,140 @@ def guest_token(self) -> Response:
except ValidationError as error:
return self.response_400(message=error.messages)
+ @expose("/login-token/", methods=("POST",))
+ @event_logger.log_this
+ @safe
+ @statsd_metrics
+ @transaction()
+ def login_token(self) -> Response:
+ """Mint a one-time login token for iframe embedding.
+ ---
+ post:
+ summary: Mint a one-time login token
+ description: >-
+ Exchanges a caller-supplied proof of identity for an opaque,
single-use
+ token that GET on this same path trades for a session cookie.
Intended
+ to be called server-to-server by a trusted parent application so
the
+ underlying credential never reaches the browser. The
+ LOGIN_TOKEN_IDENTITY_RESOLVER hook decides what counts as proof.
+ responses:
+ 200:
+ description: The minted token and its expiry
+ content:
+ application/json:
+ schema: LoginTokenResponseSchema
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ if not login_token_utils.is_enabled():
+ # 404 rather than 403: with the feature off there is nothing here
to
+ # be forbidden from, and this keeps the surface closed by default.
+ return self.response_404()
+
+ if (userinfo := login_token_utils.resolve_identity(request)) is None:
+ return self.response_401()
+
+ token, expires_on = login_token_utils.mint(userinfo)
+ logger.info(
+ "One-time login token minted for '%s' from %s",
+ userinfo.get("username") or userinfo.get("email"),
+ request.remote_addr,
+ )
+ return self.response(
+ 200,
+ access_token=token,
+ expires_at=int(expires_on.timestamp()),
+ )
+
+ @expose("/login-token/", methods=("GET",))
+ @event_logger.log_this
+ @statsd_metrics
+ @safe
+ @transaction()
+ def login_with_token(self) -> Response:
+ """Consume a one-time login token and establish a session.
+ ---
+ get:
+ summary: Consume a one-time login token
+ description: >-
+ Reached by navigating an iframe to this URL. Exchanges the token
for a
+ standard session cookie and redirects to `next`, so the frame
holds an
+ ordinary Superset session with the user's own roles and row-level
+ security. The token is deleted on use.
+ parameters:
+ - in: query
+ name: token
+ required: true
+ schema:
+ type: string
+ description: The opaque token returned by POST on this path
+ - in: query
+ name: next
+ required: false
+ schema:
+ type: string
+ description: Internal URL to redirect to; rejected if not internal
+ responses:
+ 302:
+ description: Session established; redirect to `next`
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ if not login_token_utils.is_enabled():
+ return self.response_404()
+
+ token = request.args.get("token", "")
+ # A single failure mode for unknown, malformed, expired and
already-spent
+ # tokens, so the response cannot be used to probe which one it was.
+ if not token or (userinfo := login_token_utils.consume(token)) is None:
+ return self.response_401()
+
+ # ``consume`` has deleted the entry, but ``@transaction()`` does not
nest
+ # and only commits when this handler returns normally. An exception
+ # escaping from here would roll the deletion back and resurrect a token
+ # that has already been handed out, so provisioning failures are caught
+ # and reported as a denial: the burn stays durable and a spent token is
+ # never redeemable a second time.
+ try:
+ user = self.appbuilder.sm.auth_user_oauth(userinfo)
Review Comment:
The token can still be redeemed again when FAB handles a provisioning error
internally. In pinned FAB 5.2.2, `add_user`/`update_user` catch failures and
roll back the shared session, undoing `consume()`'s pending DELETE without
raising here. `auth_user_oauth` even ignores an `update_user` failure and
returns the user, so a failing `user_updating` hook can produce a successful
login while leaving the token live. Could the burn be committed independently
before provisioning, with a route-level regression that makes FAB roll back and
then attempts a second redemption? The current burn test commits before
simulating rollback, so it misses this ordering.
##########
superset/security/api.py:
##########
@@ -268,6 +285,140 @@ def guest_token(self) -> Response:
except ValidationError as error:
return self.response_400(message=error.messages)
+ @expose("/login-token/", methods=("POST",))
+ @event_logger.log_this
Review Comment:
This decorator persists the mint request's JSON/form payload in the metadata
database through `collect_request_payload` → `DBEventLogger`. A resolver that
reads an upstream ID token or other proof from the body therefore leaves that
credential in `logs.json`, including when the resolver rejects it; the one-time
token's TTL does not limit the upstream credential's lifetime. Could this
endpoint disable request-data logging and audit only an explicit non-secret
allowlist, as the guest-token endpoint does?
##########
superset/security/api.py:
##########
@@ -268,6 +285,140 @@ def guest_token(self) -> Response:
except ValidationError as error:
return self.response_400(message=error.messages)
+ @expose("/login-token/", methods=("POST",))
+ @event_logger.log_this
+ @safe
+ @statsd_metrics
+ @transaction()
+ def login_token(self) -> Response:
+ """Mint a one-time login token for iframe embedding.
+ ---
+ post:
+ summary: Mint a one-time login token
+ description: >-
+ Exchanges a caller-supplied proof of identity for an opaque,
single-use
+ token that GET on this same path trades for a session cookie.
Intended
+ to be called server-to-server by a trusted parent application so
the
+ underlying credential never reaches the browser. The
+ LOGIN_TOKEN_IDENTITY_RESOLVER hook decides what counts as proof.
+ responses:
+ 200:
+ description: The minted token and its expiry
+ content:
+ application/json:
+ schema: LoginTokenResponseSchema
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ if not login_token_utils.is_enabled():
+ # 404 rather than 403: with the feature off there is nothing here
to
+ # be forbidden from, and this keeps the surface closed by default.
+ return self.response_404()
+
+ if (userinfo := login_token_utils.resolve_identity(request)) is None:
+ return self.response_401()
+
+ token, expires_on = login_token_utils.mint(userinfo)
+ logger.info(
+ "One-time login token minted for '%s' from %s",
+ userinfo.get("username") or userinfo.get("email"),
+ request.remote_addr,
+ )
+ return self.response(
+ 200,
+ access_token=token,
+ expires_at=int(expires_on.timestamp()),
+ )
+
+ @expose("/login-token/", methods=("GET",))
+ @event_logger.log_this
+ @statsd_metrics
+ @safe
+ @transaction()
+ def login_with_token(self) -> Response:
+ """Consume a one-time login token and establish a session.
+ ---
+ get:
+ summary: Consume a one-time login token
+ description: >-
+ Reached by navigating an iframe to this URL. Exchanges the token
for a
+ standard session cookie and redirects to `next`, so the frame
holds an
+ ordinary Superset session with the user's own roles and row-level
+ security. The token is deleted on use.
+ parameters:
+ - in: query
+ name: token
+ required: true
+ schema:
+ type: string
+ description: The opaque token returned by POST on this path
+ - in: query
+ name: next
+ required: false
+ schema:
+ type: string
+ description: Internal URL to redirect to; rejected if not internal
+ responses:
+ 302:
+ description: Session established; redirect to `next`
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ if not login_token_utils.is_enabled():
+ return self.response_404()
+
+ token = request.args.get("token", "")
+ # A single failure mode for unknown, malformed, expired and
already-spent
+ # tokens, so the response cannot be used to probe which one it was.
+ if not token or (userinfo := login_token_utils.consume(token)) is None:
+ return self.response_401()
+
+ # ``consume`` has deleted the entry, but ``@transaction()`` does not
nest
+ # and only commits when this handler returns normally. An exception
+ # escaping from here would roll the deletion back and resurrect a token
+ # that has already been handed out, so provisioning failures are caught
+ # and reported as a denial: the burn stays durable and a spent token is
+ # never redeemable a second time.
+ try:
+ user = self.appbuilder.sm.auth_user_oauth(userinfo)
+ except Exception: # pylint: disable=broad-except
+ logger.exception("Provisioning failed for a one-time login token")
+ user = None
+
+ if user is None:
+ # Provisioning declined the identity: the user is deactivated, or
+ # AUTH_USER_REGISTRATION is off and they have no account yet.
+ logger.warning(
+ "One-time login token resolved an identity that could not be "
+ "provisioned: '%s'",
+ userinfo.get("username") or userinfo.get("email"),
+ )
+ return self.response_401()
+
+ login_user(user)
+ logger.info("Session established from a one-time login token for
'%s'", user)
+
+ # Only ever redirect to a value that has passed the internal-URL check.
+ # Assigning into a separate variable inside the guarded branch — rather
+ # than reassigning the request-derived one — keeps the sanitizer on the
+ # path to the redirect, which taint analysis can follow.
+ requested_next = request.args.get("next") or "/"
+ safe_next_url = "/"
+ if is_safe_redirect_url(requested_next):
Review Comment:
A valid same-origin absolute `next` can silently send the iframe to `/`
instead of its dashboard. This helper derives allowed hosts from
`WEBDRIVER_BASEURL*`, which defaults to `0.0.0.0:8080`, not from the public
Superset origin. For a deployment at `https://superset.example.com` without
report-worker URL configuration,
`next=https://superset.example.com/superset/dashboard/42/` is rejected even
though the API describes it as an internal URL. Could the redirect contract use
the trusted public origin, or explicitly require relative paths, with a
route-level test for this deployment?
--
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]
