sha174n commented on code in PR #45452: URL: https://github.com/apache/superset/pull/45452#discussion_r4363342424
########## tests/unit_tests/security/test_guest_token_identity.py: ########## @@ -0,0 +1,188 @@ +# 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. +"""Validation of the identity strings carried in a guest token's user claim.""" + +from typing import Any, cast +from unittest.mock import patch + +import pytest +from flask import current_app +from pytest_mock import MockerFixture + +from superset.extensions import appbuilder +from superset.security.guest_token import ( + enforce_guest_token_identity, + GUEST_TOKEN_IDENTITY_FIELDS, + GUEST_TOKEN_IDENTITY_MAX_LENGTH, + GuestTokenIdentityError, + GuestTokenIdentityShapeError, + GuestTokenResources, + GuestTokenResourceType, + GuestTokenUser, + validate_guest_token_identity, +) +from superset.security.manager import SupersetSecurityManager + +RESOURCES: GuestTokenResources = [ + {"type": GuestTokenResourceType.DASHBOARD, "id": "some-uuid"} +] + + +def _identity(field: str, value: Any) -> GuestTokenUser: + """A single-field user claim, keyed dynamically for parametrized tests.""" + return cast(GuestTokenUser, {field: value}) + + +def test_plain_identity_is_accepted() -> None: + validate_guest_token_identity( + {"username": "[email protected]", "first_name": "Ada", "last_name": "Lovelace"} + ) + + +def test_missing_fields_are_accepted() -> None: + # every identity field is optional; absence must not raise + validate_guest_token_identity({}) + validate_guest_token_identity({"username": "only-username"}) + + +def test_missing_user_claim_is_accepted() -> None: + validate_guest_token_identity(None) + + +def test_non_object_user_claim_is_rejected() -> None: + with pytest.raises(GuestTokenIdentityShapeError, match="must be an object"): + validate_guest_token_identity(cast(GuestTokenUser, "[email protected]")) + + +def test_non_object_user_claim_is_rejected_while_warn_only(app_context: None) -> None: + # a non-object claim cannot become a guest user, so it is never warn-only + with pytest.raises(GuestTokenIdentityShapeError): + enforce_guest_token_identity(cast(GuestTokenUser, "[email protected]")) + + +def test_warn_only_violation_is_logged_and_counted( + mocker: MockerFixture, app_context: None +) -> None: + incr = mocker.patch("superset.extensions.stats_logger_manager.instance.incr") + warning = mocker.patch("superset.security.guest_token.logger.warning") + + enforce_guest_token_identity({"username": "bob\nalice"}, RESOURCES) + + incr.assert_called_once_with("security.GuestTokenIdentityViolation") + assert warning.call_args.args[-1] == ["some-uuid"] + + [email protected]("field", GUEST_TOKEN_IDENTITY_FIELDS) [email protected]( + "value", + [ + "line-one\nline-two", + "carriage\rreturn", + "tab\tseparated", + "trailing-newline\n", + "null\x00byte", + "line\u2028separator", + "paragraph\u2029separator", + ], +) +def test_control_characters_are_rejected(field: str, value: str) -> None: + with pytest.raises(ValueError, match="invalid characters"): + validate_guest_token_identity(_identity(field, value)) + + [email protected]("field", GUEST_TOKEN_IDENTITY_FIELDS) +def test_overlong_value_is_rejected(field: str) -> None: + with pytest.raises(ValueError, match="maximum length"): + validate_guest_token_identity( + _identity(field, "x" * (GUEST_TOKEN_IDENTITY_MAX_LENGTH + 1)) + ) + + +def test_value_at_maximum_length_is_accepted() -> None: + validate_guest_token_identity({"username": "x" * GUEST_TOKEN_IDENTITY_MAX_LENGTH}) + + [email protected]("field", GUEST_TOKEN_IDENTITY_FIELDS) +def test_non_string_value_is_rejected(field: str) -> None: + with pytest.raises(ValueError, match="must be a string"): + validate_guest_token_identity(_identity(field, 123)) + + +def test_mint_rejects_invalid_identity(app_context: None) -> None: + # enforce path: with the rollout flag on, minting a bad identity is rejected + sm = SupersetSecurityManager(appbuilder) + with ( + patch.dict(current_app.config, {"GUEST_TOKEN_IDENTITY_ENFORCE": True}), + pytest.raises(GuestTokenIdentityError), + ): + sm.create_guest_access_token({"username": "bob\nalice"}, RESOURCES, []) + + +def _guest_request(mocker: MockerFixture, raw_token: bytes) -> Any: + request = mocker.MagicMock() + request.headers = {current_app.config["GUEST_TOKEN_HEADER_NAME"]: raw_token} + request.form = {} + return request + + [email protected]( + "username", + ["x" * (GUEST_TOKEN_IDENTITY_MAX_LENGTH + 1), "trailing-newline\n"], + ids=["overlong", "trailing-newline"], +) +def test_invalid_identity_rejected_through_request_flow( + mocker: MockerFixture, app_context: None, username: str +) -> None: + """Enforce path: a token carrying a bad identity no longer resolves to a guest.""" + sm = SupersetSecurityManager(appbuilder) + # Stand in for a token minted before identity validation existed (skip the + # mint-time check so the consume-side check is what rejects it). + with patch("superset.security.manager.enforce_guest_token_identity"): + raw_token = sm.create_guest_access_token({"username": username}, RESOURCES, []) + + with patch.dict(current_app.config, {"GUEST_TOKEN_IDENTITY_ENFORCE": True}): + assert sm.get_guest_user_from_request(_guest_request(mocker, raw_token)) is None Review Comment: Added `test_invalid_identity_still_resolves_under_default_config` in fcc4d67084: with the default config it mints and consumes `{"username": "bob\nalice"}` and asserts the guest resolves with that username (it fails if the consume site is switched to the strict validator). -- 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]
