sadpandajoe commented on code in PR #44365:
URL: https://github.com/apache/superset/pull/44365#discussion_r4198560214
##########
superset/security/api.py:
##########
@@ -268,6 +284,154 @@ def guest_token(self) -> Response:
except ValidationError as error:
return self.response_400(message=error.messages)
+ @expose("/login-token/", methods=("POST",))
+ # Request data is deliberately excluded from the event log. A resolver may
+ # read the caller's proof of identity -- an OIDC id token, an internal
+ # service credential -- from the body or query string, and
+ # ``collect_request_payload`` would otherwise persist it verbatim into
+ # ``logs.json``, including on rejection. The one-time token's TTL bounds
+ # nothing about that upstream credential's lifetime.
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs:
f"{self.__class__.__name__}.login_token",
+ include_request_data=False,
+ )
+ @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: >-
+ Site-relative path to redirect to, e.g. `/dashboard/1/`. Must
begin
+ with a single `/`; absolute URLs and protocol-relative values are
+ rejected and fall back to `/`.
+ 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 already committed the burn, so nothing here can make
a
+ # spent token redeemable again. Provisioning failures are still caught
+ # and reported as a denial rather than allowed to escape: an exception
+ # would otherwise surface as a 500, which is indistinguishable from an
+ # outage to the parent application and invites a retry with a token
that
+ # no longer exists.
+ 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 relative-path
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 login_token_utils.is_safe_next_path(requested_next):
+ safe_next_url = requested_next
Review Comment:
`is_safe_next_path` validates the *normalized* value (CR/LF/tab and
`%0A`/`%09` removed), but the redirect uses the original `requested_next`. For
`next=%2F%0Adashboard%2F` the check passes, then `redirect()` is handed a
`Location` containing a newline, which Werkzeug rejects. The token delete was
already committed in `consume()`, so the parent gets a 500 and the retry gets
401, with no session established. Could this redirect to the normalized value,
or reject raw CR/LF in the check? A test with an encoded newline in `next`
would pin it.
##########
superset/security/login_token.py:
##########
@@ -0,0 +1,334 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""One-time login tokens for establishing a session inside an iframe.
+
+An SSO redirect flow cannot run inside an iframe: identity providers commonly
+refuse to be framed, and the redirect chain depends on cookies that browsers
+treat as third-party in an embedded context. A parent application that already
+holds a trustworthy proof of the user's identity uses these tokens to turn that
+proof into an ordinary Superset session in two steps:
+
+1. its backend mints a token, presenting a credential that an operator-supplied
+ resolver validates (server-to-server, so the credential never reaches the
+ browser), and
+2. the browser navigates the iframe to the consume endpoint, which exchanges
the
+ token for a session cookie.
+
+The token is an opaque handle to a short-lived server-side record; no identity
+data travels in the URL.
+
+**This module must stay importable from ``superset_config.py``.** The
documented
+way to write a resolver begins with ``from superset.security.login_token import
+LoginTokenUserInfo``, and the config is read before the application is
+initialized. Anything reaching ``superset.models`` -- the key-value DAO and its
+model both do, via ``superset.models.helpers`` -- builds encrypted columns and
+``relationship(security_manager.user_model, ...)`` at class-definition time and
+raises "App not initialized yet". Those imports are therefore deferred into the
+functions that need them; ``key_value.types`` and ``key_value.utils`` pull in
no
+models and are safe at module scope.
+"""
+
+from __future__ import annotations
+
+import logging
+import re
+from datetime import datetime, timedelta
+from typing import Any, Callable, cast, TypedDict
+from uuid import UUID, uuid4
+
+from flask import current_app, Request
+
+from superset.key_value.types import JsonKeyValueCodec, KeyValueResource
+from superset.key_value.utils import get_filter
+
+logger = logging.getLogger(__name__)
+
+LOGIN_TOKEN_RESOURCE = KeyValueResource.LOGIN_TOKEN
+LOGIN_TOKEN_CODEC = JsonKeyValueCodec()
+
+# Kept short deliberately: the token is handed to a browser as an iframe URL,
so
+# it lands in access logs and the parent page's DOM. A lifetime measured in
+# seconds bounds the window in which an observer could replay it.
+DEFAULT_LOGIN_TOKEN_TTL_SECONDS = 60
+
+
+class LoginTokenUserInfo(TypedDict, total=False):
+ """The identity a resolver returns for a one-time login token.
+
+ This mirrors the ``userinfo`` contract of Flask-AppBuilder's
+ ``auth_user_oauth``, which the consume step delegates to. ``username``
+ identifies the user, falling back to ``email`` when absent. ``role_keys``
are
+ resolved through ``AUTH_ROLES_MAPPING``, so a caller can only ever request
+ roles the operator has already mapped -- authorization stays with the
+ operator rather than moving to the parent application.
+ """
+
+ username: str
+ email: str
+ first_name: str
+ last_name: str
+ role_keys: list[str]
+
+
+LoginTokenIdentityResolver = Callable[..., LoginTokenUserInfo | None]
+
+# Characters a WHATWG URL parser strips before resolving, plus their
+# percent-encoded forms, which some browsers also remove when following a
+# Location header. Normalizing them first stops `/\tx` or `/%09x` smuggling a
+# different target past the checks below.
+_URL_STRIPPED_CONTROL_CHARS = re.compile(r"[\t\n\r]|%09|%0[ADad]")
+
+
+def is_safe_next_path(url: str) -> bool:
+ """Whether ``url`` is a site-relative path this endpoint may redirect to.
+
+ Deliberately stricter than :func:`superset.utils.link_redirect.
+ is_safe_redirect_url`, which resolves "internal" against
+ ``WEBDRIVER_BASEURL`` / ``WEBDRIVER_BASEURL_USER_FRIENDLY``. Those are
+ report-worker settings defaulting to ``http://0.0.0.0:8080/`` and need bear
+ no relation to the public origin, so a deployment that has never configured
+ reports would see a legitimate same-origin absolute URL rejected and be
+ redirected to ``/`` instead of the requested dashboard.
+
+ Requiring a relative path avoids the question entirely: the parent
+ application always knows the path it wants, the browser resolves it against
+ Superset's own origin, and there is no host to compare.
+ """
+ if not url or not url.strip():
+ return False
+
+ normalized = _URL_STRIPPED_CONTROL_CHARS.sub("", url.strip())
+ # Browsers treat backslashes as forward slashes in special schemes, so
+ # `/\evil.com` would resolve as the protocol-relative `//evil.com`.
+ normalized = normalized.replace("\\", "/")
+
+ # A single leading slash, and nothing that could be read as a host or a
+ # scheme. `//host`, `https://host` and `mailto:x` are all rejected.
+ return normalized.startswith("/") and not normalized.startswith("//")
+
+
+def is_enabled() -> bool:
+ """Whether the flow is usable: feature flag on and a resolver
configured."""
+ # Deferred: ``superset/__init__`` imports the app factory, so a
module-level
+ # import here would be circular via superset.daos / superset.security.
+ from superset import ( # pylint: disable=import-outside-toplevel
+ is_feature_enabled,
+ )
+
+ if not is_feature_enabled("LOGIN_TOKEN"):
+ return False
+
+ return current_app.config.get("LOGIN_TOKEN_IDENTITY_RESOLVER") is not None
+
+
+def get_ttl_seconds() -> int:
+ """The configured token lifetime, falling back to the default."""
+ configured = current_app.config.get(
+ "LOGIN_TOKEN_TTL_SECONDS", DEFAULT_LOGIN_TOKEN_TTL_SECONDS
+ )
+ try:
+ ttl = int(configured)
+ except (TypeError, ValueError):
+ logger.warning(
+ "LOGIN_TOKEN_TTL_SECONDS is not an integer (%r); using %s",
+ configured,
+ DEFAULT_LOGIN_TOKEN_TTL_SECONDS,
+ )
+ return DEFAULT_LOGIN_TOKEN_TTL_SECONDS
+
+ return ttl if ttl > 0 else DEFAULT_LOGIN_TOKEN_TTL_SECONDS
+
+
+# Each rejection below is a distinct failure with its own diagnostic — an
+# unconfigured resolver is normal and silent, a misconfigured or misbehaving
one
+# is logged. Merging the branches to satisfy the return-count limit would lose
+# that distinction, which is the only signal an operator gets.
+def resolve_identity( # pylint: disable=too-many-return-statements
+ request: Request, **kwargs: Any
+) -> LoginTokenUserInfo | None:
+ """Run the operator-supplied resolver against an inbound mint request.
+
+ The resolver is the whole authentication boundary for minting: it decides
+ what counts as proof of identity (an OIDC id token, an existing session, a
+ credential checked against an internal service). A resolver that raises, or
+ returns something other than a mapping, is treated as a rejection rather
+ than a server error, so a failing validation can never be mistaken for a
+ successful one.
+ """
+ resolver = current_app.config.get("LOGIN_TOKEN_IDENTITY_RESOLVER")
+ if resolver is None:
+ return None
+
+ if not callable(resolver):
+ logger.error("LOGIN_TOKEN_IDENTITY_RESOLVER is not callable")
+ return None
+
+ try:
+ userinfo = resolver(request, **kwargs)
+ except Exception: # pylint: disable=broad-except
+ logger.exception("LOGIN_TOKEN_IDENTITY_RESOLVER rejected the request")
+ return None
+
+ if not userinfo:
+ return None
+
+ if not isinstance(userinfo, dict):
+ # A truthy non-mapping would otherwise raise on the ``.get`` below and
+ # surface as a 500, contradicting the rejection contract above.
+ logger.error(
+ "LOGIN_TOKEN_IDENTITY_RESOLVER returned %s, expected a mapping",
+ type(userinfo).__name__,
+ )
+ return None
+
+ # Normalize before validating, because ``auth_user_oauth`` selects on key
+ # *presence* rather than truthiness: ``if "username" in userinfo`` wins
even
+ # when the value is empty, and the empty username is then rejected
outright.
+ # A resolver returning {"username": "", "email": "[email protected]"} would
+ # otherwise mint a perfectly good token that always fails redemption with a
+ # 401, instead of falling back to the email. Stripping and dropping empty
+ # string values keeps this check and FAB's in agreement.
+ userinfo = {
+ key: value.strip() if isinstance(value, str) else value
Review Comment:
Stripping the identifier after the resolver has validated it can change
which account is authenticated. If the metastore has both a user `" admin "`
and a user `admin`, a resolver that correctly returns the padded username mints
a token whose stored username is `admin`, and `auth_user_oauth` then logs the
caller in as the other account (FAB looks up by username only, and roles are
kept when role sync is off). Should this preserve the resolver's value as-is,
or reject identifiers that change under `strip()` instead of silently aliasing
them?
##########
superset/security/login_token.py:
##########
@@ -0,0 +1,334 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""One-time login tokens for establishing a session inside an iframe.
+
+An SSO redirect flow cannot run inside an iframe: identity providers commonly
+refuse to be framed, and the redirect chain depends on cookies that browsers
+treat as third-party in an embedded context. A parent application that already
+holds a trustworthy proof of the user's identity uses these tokens to turn that
+proof into an ordinary Superset session in two steps:
+
+1. its backend mints a token, presenting a credential that an operator-supplied
+ resolver validates (server-to-server, so the credential never reaches the
+ browser), and
+2. the browser navigates the iframe to the consume endpoint, which exchanges
the
+ token for a session cookie.
+
+The token is an opaque handle to a short-lived server-side record; no identity
+data travels in the URL.
+
+**This module must stay importable from ``superset_config.py``.** The
documented
+way to write a resolver begins with ``from superset.security.login_token import
+LoginTokenUserInfo``, and the config is read before the application is
+initialized. Anything reaching ``superset.models`` -- the key-value DAO and its
+model both do, via ``superset.models.helpers`` -- builds encrypted columns and
+``relationship(security_manager.user_model, ...)`` at class-definition time and
+raises "App not initialized yet". Those imports are therefore deferred into the
+functions that need them; ``key_value.types`` and ``key_value.utils`` pull in
no
+models and are safe at module scope.
+"""
+
+from __future__ import annotations
+
+import logging
+import re
+from datetime import datetime, timedelta
+from typing import Any, Callable, cast, TypedDict
+from uuid import UUID, uuid4
+
+from flask import current_app, Request
+
+from superset.key_value.types import JsonKeyValueCodec, KeyValueResource
+from superset.key_value.utils import get_filter
+
+logger = logging.getLogger(__name__)
+
+LOGIN_TOKEN_RESOURCE = KeyValueResource.LOGIN_TOKEN
+LOGIN_TOKEN_CODEC = JsonKeyValueCodec()
+
+# Kept short deliberately: the token is handed to a browser as an iframe URL,
so
+# it lands in access logs and the parent page's DOM. A lifetime measured in
+# seconds bounds the window in which an observer could replay it.
+DEFAULT_LOGIN_TOKEN_TTL_SECONDS = 60
+
+
+class LoginTokenUserInfo(TypedDict, total=False):
+ """The identity a resolver returns for a one-time login token.
+
+ This mirrors the ``userinfo`` contract of Flask-AppBuilder's
+ ``auth_user_oauth``, which the consume step delegates to. ``username``
+ identifies the user, falling back to ``email`` when absent. ``role_keys``
are
+ resolved through ``AUTH_ROLES_MAPPING``, so a caller can only ever request
+ roles the operator has already mapped -- authorization stays with the
+ operator rather than moving to the parent application.
+ """
+
+ username: str
+ email: str
+ first_name: str
+ last_name: str
+ role_keys: list[str]
+
+
+LoginTokenIdentityResolver = Callable[..., LoginTokenUserInfo | None]
+
+# Characters a WHATWG URL parser strips before resolving, plus their
+# percent-encoded forms, which some browsers also remove when following a
+# Location header. Normalizing them first stops `/\tx` or `/%09x` smuggling a
+# different target past the checks below.
+_URL_STRIPPED_CONTROL_CHARS = re.compile(r"[\t\n\r]|%09|%0[ADad]")
+
+
+def is_safe_next_path(url: str) -> bool:
+ """Whether ``url`` is a site-relative path this endpoint may redirect to.
+
+ Deliberately stricter than :func:`superset.utils.link_redirect.
+ is_safe_redirect_url`, which resolves "internal" against
+ ``WEBDRIVER_BASEURL`` / ``WEBDRIVER_BASEURL_USER_FRIENDLY``. Those are
+ report-worker settings defaulting to ``http://0.0.0.0:8080/`` and need bear
+ no relation to the public origin, so a deployment that has never configured
+ reports would see a legitimate same-origin absolute URL rejected and be
+ redirected to ``/`` instead of the requested dashboard.
+
+ Requiring a relative path avoids the question entirely: the parent
+ application always knows the path it wants, the browser resolves it against
+ Superset's own origin, and there is no host to compare.
+ """
+ if not url or not url.strip():
+ return False
+
+ normalized = _URL_STRIPPED_CONTROL_CHARS.sub("", url.strip())
+ # Browsers treat backslashes as forward slashes in special schemes, so
+ # `/\evil.com` would resolve as the protocol-relative `//evil.com`.
+ normalized = normalized.replace("\\", "/")
+
+ # A single leading slash, and nothing that could be read as a host or a
+ # scheme. `//host`, `https://host` and `mailto:x` are all rejected.
+ return normalized.startswith("/") and not normalized.startswith("//")
+
+
+def is_enabled() -> bool:
+ """Whether the flow is usable: feature flag on and a resolver
configured."""
+ # Deferred: ``superset/__init__`` imports the app factory, so a
module-level
+ # import here would be circular via superset.daos / superset.security.
+ from superset import ( # pylint: disable=import-outside-toplevel
+ is_feature_enabled,
+ )
+
+ if not is_feature_enabled("LOGIN_TOKEN"):
+ return False
+
+ return current_app.config.get("LOGIN_TOKEN_IDENTITY_RESOLVER") is not None
+
+
+def get_ttl_seconds() -> int:
+ """The configured token lifetime, falling back to the default."""
+ configured = current_app.config.get(
+ "LOGIN_TOKEN_TTL_SECONDS", DEFAULT_LOGIN_TOKEN_TTL_SECONDS
+ )
+ try:
+ ttl = int(configured)
+ except (TypeError, ValueError):
+ logger.warning(
+ "LOGIN_TOKEN_TTL_SECONDS is not an integer (%r); using %s",
+ configured,
+ DEFAULT_LOGIN_TOKEN_TTL_SECONDS,
+ )
+ return DEFAULT_LOGIN_TOKEN_TTL_SECONDS
+
+ return ttl if ttl > 0 else DEFAULT_LOGIN_TOKEN_TTL_SECONDS
+
+
+# Each rejection below is a distinct failure with its own diagnostic — an
+# unconfigured resolver is normal and silent, a misconfigured or misbehaving
one
+# is logged. Merging the branches to satisfy the return-count limit would lose
+# that distinction, which is the only signal an operator gets.
+def resolve_identity( # pylint: disable=too-many-return-statements
+ request: Request, **kwargs: Any
+) -> LoginTokenUserInfo | None:
+ """Run the operator-supplied resolver against an inbound mint request.
+
+ The resolver is the whole authentication boundary for minting: it decides
+ what counts as proof of identity (an OIDC id token, an existing session, a
+ credential checked against an internal service). A resolver that raises, or
+ returns something other than a mapping, is treated as a rejection rather
+ than a server error, so a failing validation can never be mistaken for a
+ successful one.
+ """
+ resolver = current_app.config.get("LOGIN_TOKEN_IDENTITY_RESOLVER")
+ if resolver is None:
+ return None
+
+ if not callable(resolver):
+ logger.error("LOGIN_TOKEN_IDENTITY_RESOLVER is not callable")
+ return None
+
+ try:
+ userinfo = resolver(request, **kwargs)
+ except Exception: # pylint: disable=broad-except
+ logger.exception("LOGIN_TOKEN_IDENTITY_RESOLVER rejected the request")
+ return None
+
+ if not userinfo:
+ return None
+
+ if not isinstance(userinfo, dict):
+ # A truthy non-mapping would otherwise raise on the ``.get`` below and
+ # surface as a 500, contradicting the rejection contract above.
+ logger.error(
+ "LOGIN_TOKEN_IDENTITY_RESOLVER returned %s, expected a mapping",
+ type(userinfo).__name__,
+ )
+ return None
+
+ # Normalize before validating, because ``auth_user_oauth`` selects on key
+ # *presence* rather than truthiness: ``if "username" in userinfo`` wins
even
+ # when the value is empty, and the empty username is then rejected
outright.
+ # A resolver returning {"username": "", "email": "[email protected]"} would
+ # otherwise mint a perfectly good token that always fails redemption with a
+ # 401, instead of falling back to the email. Stripping and dropping empty
+ # string values keeps this check and FAB's in agreement.
+ userinfo = {
+ key: value.strip() if isinstance(value, str) else value
+ for key, value in userinfo.items()
+ if not (isinstance(value, str) and not value.strip())
Review Comment:
The normalization drops empty strings but keeps `None`, which is the same
key-present-but-unusable shape that the comment above describes for `""`. A
resolver written as `{"username": claims.get("preferred_username"), "email":
claims.get("email")}` returns `username: None` when the claim is absent; the
`username or email` check passes, a token mints, and FAB's `"username" in
userinfo` branch then selects the `None` username and rejects it, so every
redemption is a 401. Should `None` values be dropped here as well (or the
identity fields validated as strings)?
##########
tests/integration_tests/security/login_token_api_tests.py:
##########
@@ -0,0 +1,443 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Route-level tests for the one-time login token endpoints.
+
+These complement ``tests/unit_tests/security/login_token_test.py``, which
+exercises mint and consume directly. Driving the HTTP routes additionally
covers
+the decorator stack -- ``@transaction()`` in particular, which does not nest --
+and the session cookie that ``login_user`` writes, neither of which the unit
+tests can see.
+"""
+
+from contextlib import contextmanager
+from typing import Any
+from unittest.mock import patch
+from uuid import UUID
+
+from flask_appbuilder.security.sqla.manager import user_updating
+
+from superset import db
+from superset.daos.key_value import KeyValueDAO
+from superset.extensions import csrf
+from superset.key_value.models import KeyValueEntry
+from superset.key_value.types import KeyValueResource
+from superset.utils import json
+from tests.conftest import with_config
+from tests.integration_tests.base_tests import SupersetTestCase
+from tests.integration_tests.conftest import with_feature_flags
+from tests.integration_tests.constants import GAMMA_USERNAME
+
+ENDPOINT = "/api/v1/security/login-token/"
+MINT_SECRET = "integration-test-secret" # noqa: S105
+
+
+def _resolver(request: Any, **kwargs: Any) -> dict[str, Any] | None:
+ """Stand-in for a deployment resolver: a shared header plus a username.
+
+ A real one would validate an id token or call an internal service; all this
+ needs to do is distinguish an authorized caller from an unauthorized one.
+ """
+ if request.headers.get("X-Test-Mint-Secret") != MINT_SECRET:
+ return None
+ username = (request.get_json(silent=True) or {}).get("username")
+ return {"username": username} if username else None
+
+
+def _verbatim_resolver(request: Any, **kwargs: Any) -> dict[str, Any] | None:
+ """Return the request body as ``userinfo``, unmodified.
+
+ Lets a test drive an arbitrary resolver result through the real route,
which
+ is the only way to cover shapes a sensible resolver would not produce but a
+ misbehaving one will.
+ """
+ if request.headers.get("X-Test-Mint-Secret") != MINT_SECRET:
+ return None
+ return request.get_json(silent=True) or None
+
+
+@contextmanager
+def csrf_enabled(app: Any) -> Any:
+ """Turn real CSRF protection on for the duration of a test.
+
+ The integration config sets ``WTF_CSRF_ENABLED = False``, so
+ ``SupersetAppInitializer.configure_wtf`` never ran ``csrf.init_app`` and
+ never applied ``WTF_CSRF_EXEMPT_LIST``. Flipping the config alone does
+ nothing: there is no ``before_request`` hook to flip, and the defaults
+ ``validate_csrf`` reads (``WTF_CSRF_METHODS``, ``WTF_CSRF_FIELD_NAME`` and
+ the rest) are only set by ``init_app``.
+
+ So the real ``init_app`` runs, rather than those defaults being restated
+ here -- restating them would mean reimplementing part of what this is meant
+ to verify. It registers a ``before_request`` hook and a context processor,
+ which Flask refuses once a request has been served, so the setup guard is
+ lifted for that call and everything it appended is removed afterwards. This
+ keeps the test independent of its position in the suite.
+ """
+ got_first_request = app._got_first_request # noqa: SLF001
+ before = list(app.before_request_funcs.get(None, []))
+ processors = list(app.template_context_processors.get(None, []))
+ extension = app.extensions.get("csrf")
+ exempt_views = set(csrf._exempt_views) # noqa: SLF001
+ config = {
+ key: app.config[key] for key in list(app.config) if
key.startswith("WTF_CSRF_")
+ }
+
+ try:
+ app._got_first_request = False # noqa: SLF001
+ csrf.init_app(app)
+ for view in app.config["WTF_CSRF_EXEMPT_LIST"]:
+ csrf.exempt(view)
+ app._got_first_request = got_first_request # noqa: SLF001
+ app.config["WTF_CSRF_ENABLED"] = True
+ yield
+ finally:
+ app._got_first_request = got_first_request # noqa: SLF001
+ app.before_request_funcs[None] = before
+ app.template_context_processors[None] = processors
+ csrf._exempt_views = exempt_views # noqa: SLF001
+ for key in [k for k in list(app.config) if k.startswith("WTF_CSRF_")]:
+ if key in config:
+ app.config[key] = config[key]
+ else:
+ del app.config[key]
+ if extension is None:
+ app.extensions.pop("csrf", None)
+ else:
+ app.extensions["csrf"] = extension
+
+
+class TestLoginTokenApi(SupersetTestCase):
+ def _mint(self, username: str = GAMMA_USERNAME) -> str:
+ """Mint a token through the route and return it."""
+ response = self.client.post(
+ ENDPOINT,
+ data=json.dumps({"username": username}),
+ content_type="application/json",
+ headers={"X-Test-Mint-Secret": MINT_SECRET},
+ )
+ assert response.status_code == 200, response.data
+ token = json.loads(response.data)["access_token"]
+ assert token
+ return token
+
+ @staticmethod
+ def _stored_tokens() -> int:
+ return (
+ db.session.query(KeyValueEntry)
+ .filter(KeyValueEntry.resource ==
KeyValueResource.LOGIN_TOKEN.value)
+ .count()
+ )
+
+ @staticmethod
+ def _is_stored(token: str) -> bool:
+ """Whether this specific token still has a row.
+
+ Scoped to one key rather than counting the table: the metadata database
+ is shared across the suite, so an absolute count couples these
+ assertions to whatever other tests happen to have left behind.
+ """
+ return (
+ KeyValueDAO.get_entry(KeyValueResource.LOGIN_TOKEN, UUID(token))
is not None
+ )
+
+ # ---------------------------------------------------------------- closed
off
+
+ def test_endpoints_are_404_without_the_feature_flag(self):
+ """Closed by default: neither verb exists until the flag is on."""
+ assert self.client.post(ENDPOINT).status_code == 404
+ assert self.client.get(f"{ENDPOINT}?token=x").status_code == 404
+
+ @with_feature_flags(LOGIN_TOKEN=True)
+ @with_config({"LOGIN_TOKEN_IDENTITY_RESOLVER": None})
+ def test_endpoints_are_404_without_a_resolver(self):
+ """The flag alone is not enough -- a resolver must also be
configured."""
+ assert self.client.post(ENDPOINT).status_code == 404
+ assert self.client.get(f"{ENDPOINT}?token=x").status_code == 404
+
+ # ---------------------------------------------------------------------
mint
+
+ @with_feature_flags(LOGIN_TOKEN=True)
+ @with_config({"LOGIN_TOKEN_IDENTITY_RESOLVER": _resolver})
+ def test_mint_rejects_a_caller_the_resolver_declines(self):
+ """No shared header, no token -- and nothing written to the store."""
+ before = self._stored_tokens()
+ response = self.client.post(
+ ENDPOINT,
+ data=json.dumps({"username": GAMMA_USERNAME}),
+ content_type="application/json",
+ )
+ assert response.status_code == 401
+ assert self._stored_tokens() == before
+
+ # ------------------------------------------------------------------
consume
+
+ @with_feature_flags(LOGIN_TOKEN=True)
+ @with_config({"LOGIN_TOKEN_IDENTITY_RESOLVER": _resolver})
+ def test_consume_establishes_a_session_and_redirects(self):
Review Comment:
Every successful route test redeems an identity for an existing Gamma user,
so nothing exercises registration or `role_keys` through the real
`auth_user_oauth`. If the handoff at `api.py:402` dropped everything but
`username`, these tests would stay green while mapped roles were lost. Could
one integration test mint then redeem for a nonexistent user with
`AUTH_USER_REGISTRATION` on, `AUTH_ROLES_MAPPING` mapping one key, and
`role_keys` containing that key plus an unmapped `Admin`, and assert the
persisted roles are exactly the registration role plus the mapped one?
--
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]
