From a2048c31fcb764efa19fddb75d1716abc4b28cab Mon Sep 17 00:00:00 2001 From: "Felipe N. Schuch" Date: Wed, 9 Sep 2026 15:06:41 -0300 Subject: [PATCH] feat: replace list with set for permissions in TokenPayload and related components --- armasec_lite/schemas.py | 8 +++--- armasec_lite/token_decoder.py | 14 ++++++---- armasec_lite/token_payload.py | 37 +++++++++++++++++++++----- armasec_lite/token_security.py | 16 +++++++---- docusaurus/docs/index.md | 14 +++++++--- docusaurus/docs/migration.md | 44 +++++++++++++++++++++++++++++++ docusaurus/yarn.lock | 11 +++++--- tests/unit/test_token_decoder.py | 6 ++--- tests/unit/test_token_payload.py | 28 +++++++++++++++++--- tests/unit/test_token_security.py | 12 +++++++++ 10 files changed, 157 insertions(+), 33 deletions(-) diff --git a/armasec_lite/schemas.py b/armasec_lite/schemas.py index 019073e..ef1c53a 100644 --- a/armasec_lite/schemas.py +++ b/armasec_lite/schemas.py @@ -48,7 +48,7 @@ from __future__ import annotations -from collections.abc import Callable +from collections.abc import Callable, Collection from enum import Enum from typing import Any from urllib.parse import urlparse @@ -269,7 +269,9 @@ class DomainConfig(BaseModel): match_keys: Key/value pairs that must be present in a decoded token. A mismatch raises 403. permission_extractor: Optional function that extracts permissions from the - decoded token when they are not a top level claim. + decoded token when they are not a top level claim. May + return any collection of strings; pydantic coerces the + result into `TokenPayload.permissions`, a set. """ domain: str @@ -279,7 +281,7 @@ class DomainConfig(BaseModel): use_https: bool = True verify_issuer: bool = True match_keys: dict[str, Any] = {} - permission_extractor: Callable[[dict[str, Any]], list[str]] | None = None + permission_extractor: Callable[[dict[str, Any]], Collection[str]] | None = None @field_validator("domain") @classmethod diff --git a/armasec_lite/token_decoder.py b/armasec_lite/token_decoder.py index ee3396d..cdf0670 100644 --- a/armasec_lite/token_decoder.py +++ b/armasec_lite/token_decoder.py @@ -41,7 +41,7 @@ from __future__ import annotations -from collections.abc import Callable +from collections.abc import Callable, Collection from functools import partial from typing import Any @@ -86,7 +86,7 @@ def __init__( algorithm: str = "RS256", debug_logger: Callable[..., None] | None = None, decode_options_override: dict[str, Any] | None = None, - permission_extractor: Callable[[dict[str, Any]], list[str]] | None = None, + permission_extractor: Callable[[dict[str, Any]], Collection[str]] | None = None, jwks_refresher: Callable[[], JWKs] | None = None, ): """ @@ -103,6 +103,9 @@ def __init__( See the `armasec_lite.jwt` module docstring. permission_extractor: Optional function that extracts permissions from the decoded token when they are not a top level claim. + It may return any collection of strings: a list, set + or tuple all validate, since pydantic coerces the + result into `TokenPayload.permissions`, a set. Consider the example token: @@ -356,7 +359,7 @@ def decode(self, token: str, **claims: Any) -> TokenPayload: return token_payload -def extract_keycloak_permissions(decoded_token: dict[str, Any]) -> list[str]: +def extract_keycloak_permissions(decoded_token: dict[str, Any]) -> set[str]: """ Extract permissions from a Keycloak token. @@ -374,7 +377,8 @@ def extract_keycloak_permissions(decoded_token: dict[str, Any]) -> list[str]: } ``` - this extractor returns `["read:stuff"]`. + this extractor returns `{"read:stuff"}`. A set rather than a list, matching the + `TokenPayload.permissions` field it feeds; Keycloak roles carry no meaningful order. Pass it as `DomainConfig(permission_extractor=extract_keycloak_permissions)`. It is called only after the signature has verified, so the claims it reads are trustworthy. @@ -393,4 +397,4 @@ def extract_keycloak_permissions(decoded_token: dict[str, Any]) -> list[str]: configuration mistake, not a bad token. """ resource_key = decoded_token["azp"] - return list(decoded_token["resource_access"][resource_key]["roles"]) + return set(decoded_token["resource_access"][resource_key]["roles"]) diff --git a/armasec_lite/token_payload.py b/armasec_lite/token_payload.py index 3dd2b70..61527cb 100644 --- a/armasec_lite/token_payload.py +++ b/armasec_lite/token_payload.py @@ -19,6 +19,21 @@ provider's tokens may be absent on another's, and the model will not tell you in advance. Read unknown claims with `getattr(payload, name, default)`. +### `permissions` is a set + +Upstream armasec typed `permissions` as a list. Here it is a `set[str]`, because every +consumer of the field, the scope check above all, tests membership and intersection and +never order, and a set makes those checks constant time instead of linear. Pydantic +coerces the JSON array in the token's claim on validation, so duplicates collapse and +issuers need not change anything. Only code that indexed or ordered `payload.permissions` +notices; `to_dict` still emits a sorted list, keeping the compatibility shim's shape. + +The consumer-visible caveat is serialization order. In code the field is an ordinary +Python set, but when a handler returns the payload as its response body, the JSON encoder +turns the set into a list in iteration order, which for strings varies per process. A +snapshot test or downstream contract asserting that JSON byte-for-byte becomes flaky. +Where a stable shape matters, return `to_dict()` or sort the field before serializing. + ### Aliases `expire` and `client_id` accept two source names each, via `AliasChoices`: the registered @@ -55,9 +70,14 @@ class TokenPayload(BaseModel): The only required field. permissions: The permissions the token grants, checked against a route's scopes. Read from a top level `permissions` claim, or produced by - a `permission_extractor` for providers that nest them. Defaults to - empty, so a token with no permissions authenticates but authorizes - nothing. + a `permission_extractor` for providers that nest them. A set, + deliberately: permissions are only ever tested for membership and + intersection, never for order, and validation collapses a claim + that repeats a permission into granting it once. Serializing the + payload renders the set in nondeterministic order; where a stable + shape matters, use `to_dict`, which emits a sorted list. Defaults + to empty, so a token with no permissions authenticates but + authorizes nothing. expire: The "exp" (or "expire") claim, as a datetime. Informational here: expiry was already enforced during decoding, so a payload in hand is not expired. @@ -70,7 +90,7 @@ class TokenPayload(BaseModel): """ sub: str - permissions: list[str] = Field(default_factory=list) + permissions: set[str] = Field(default_factory=set) expire: datetime | None = Field(None, validation_alias=AliasChoices("exp", "expire")) client_id: str | None = Field(None, validation_alias=AliasChoices("azp", "client_id")) original_token: str | None = None @@ -87,12 +107,15 @@ def to_dict(self) -> dict[str, Any]: `exp` again. Prefer reading attributes off the model directly. Returns: - A dictionary with `sub`, `permissions`, `exp` and `client_id`. `exp` is None - when the token carried no expiry. + A dictionary with `sub`, `permissions`, `exp` and `client_id`. `permissions` + is a sorted list rather than the set on the model, because the shim's whole + point is upstream's JSON-friendly shape and a set is neither + JSON-serializable nor deterministically ordered. `exp` is None when the token + carried no expiry. """ return { "sub": self.sub, - "permissions": self.permissions, + "permissions": sorted(self.permissions), "exp": int(self.expire.timestamp()) if self.expire is not None else None, "client_id": self.client_id, } diff --git a/armasec_lite/token_security.py b/armasec_lite/token_security.py index b5890b7..cb06730 100644 --- a/armasec_lite/token_security.py +++ b/armasec_lite/token_security.py @@ -101,7 +101,10 @@ class TokenSecurity(APIKeyBase): Attributes: domain_configs: The OIDC domains a token may be authenticated against. A token is accepted if any one of them can decode it. - scopes: Permissions the token must carry, or None to check none. + scopes: Permissions the token must carry, as a frozenset. Empty means + authentication only, with no permission check. Materialized + once at construction because `_check_scopes` runs per request + and must not rebuild it there. permission_mode: How `scopes` is matched. ALL requires every one, SOME requires at least one. debug_logger: A callable such as `logger.debug`. Defaults to `noop`, which @@ -140,7 +143,10 @@ def __init__( token is accepted if any one of them decodes it. scopes: Optional permission scopes that should be checked. When empty or None, authentication is required but no permission - check is performed. + check is performed. Any iterable of strings is accepted and + materialized into a frozenset here, so a generator is + consumed exactly once instead of silently emptying after + the first request. permission_mode: How the scopes are matched. ALL or SOME. debug_logger: A callable such as `logger.debug`. Defaults to `noop`. debug_exceptions: If True, raise original exceptions instead of translating @@ -149,7 +155,7 @@ def __init__( skip_plugins: If True, do not evaluate plugin validators. """ self.domain_configs = domain_configs - self.scopes = scopes + self.scopes = frozenset(scopes or ()) self.permission_mode = permission_mode self.debug_logger = debug_logger if debug_logger else noop @@ -324,8 +330,8 @@ def _check_scopes(self, token_payload: TokenPayload) -> None: branch recognizes. Maps to 403: the caller is authenticated, just not allowed. """ - token_permissions = set(token_payload.permissions) - my_permissions = set(self.scopes or ()) + token_permissions = token_payload.permissions + my_permissions = self.scopes # Guarded rather than left to the logger to discard: this runs once per request, # and `unwrap` splits and rejoins the whole composed string. diff --git a/docusaurus/docs/index.md b/docusaurus/docs/index.md index b95315d..95d03b6 100644 --- a/docusaurus/docs/index.md +++ b/docusaurus/docs/index.md @@ -41,10 +41,11 @@ upstream, instead of breaking all three to save zero bytes. | `fastapi` | kept | | (new) | `cryptography` | -## Four differences from upstream +## Five differences from upstream -`armasec-lite` targets a drop-in migration, but four things differ. Each is covered in -full on the [migration guide](./migration.md). +`armasec-lite` targets a drop-in migration, but five things differ. Each is covered in +full on the [migration guide](./migration.md), which carries the complete, authoritative +list. 1. **Import name.** The package imports as `armasec_lite`, not `armasec`. The distribution name changes too: `armasec-lite`, not `armasec`. @@ -60,6 +61,13 @@ full on the [migration guide](./migration.md). additive and requires no action: the first positional argument is still `JWKs`, so every existing construction site is unaffected. It is what lets a JWKS key rotation recover without a process restart. +5. **`TokenPayload.permissions` is a `set[str]`, not a `list[str]`.** Permissions are + only ever tested for membership and intersection, so the scope check runs in constant + time and a claim repeating a permission collapses into granting it once. Tokens need + no changes; pydantic coerces the claim's JSON array. Two things to watch: code that + indexes or orders the field, and a handler that returns the payload as its response + body, whose `permissions` now serialize into the response JSON in nondeterministic + order. Where a stable JSON shape matters, use `to_dict()`, which emits a sorted list. ## Request flow diff --git a/docusaurus/docs/migration.md b/docusaurus/docs/migration.md index 53ef621..a95a74f 100644 --- a/docusaurus/docs/migration.md +++ b/docusaurus/docs/migration.md @@ -118,6 +118,31 @@ project combined. It may ship later as a separate `armasec-lite-cli` distributio installed alongside `armasec-lite` if you rely on it, or wait for a future `armasec-lite-cli`. +## 7. `TokenPayload.permissions` is a `set[str]` + +Upstream types `permissions` as `list[str]`. `armasec-lite` types it as `set[str]`, +because every consumer of the field, the scope check above all, tests membership and +intersection and never order, and a set makes those checks constant time. Nothing changes +on the way in: the claim in the token is still a JSON array, pydantic coerces it on +validation, and a claim that repeats a permission now collapses into granting it once. +`extract_keycloak_permissions` returns a set as well, matching the field it feeds. + +Two things on the way out can break: + +- Code that indexes or orders the field (`payload.permissions[0]`, `sorted` assumptions, + strict equality against a list) stops working. Compare against a set, or sort + explicitly where you need an order. +- A handler that returns the `TokenPayload` model as its response body now serializes + `permissions` in nondeterministic order: JSON has no set type, so the encoder turns the + set into a list in iteration order, and set iteration order for strings varies per + process. Nothing changes inside your code, where the field is an ordinary Python set; + only the JSON a client receives is affected. Anything asserting or diffing that JSON + byte-for-byte, such as a snapshot test or a downstream contract test, becomes flaky. + +**Fix:** compare permissions with set operations. Where a stable JSON shape matters, use +`payload.to_dict()`, which deliberately emits `permissions` as a sorted list, or sort the +field yourself before serializing. + ## Requires no action A few more differences round out the API diff. None of them breaks anything migrating, so @@ -172,6 +197,23 @@ message that names neither the domain nor the configuration. load a provider, and a `DomainConfig` with an unsupported algorithm could not decode a token. `Armasec()` with no domain at all still raises the same 422 it always did. +### `TokenSecurity.scopes` is a frozenset, and `permission_extractor` may return any collection + +`TokenSecurity` materializes the `scopes` it is given into a `frozenset` at construction, +so the per-request scope check rebuilds nothing. The constructor still accepts any +iterable of strings, exactly as upstream did; only code reading `security.scopes` back +and expecting the original list or tuple notices. Materializing also means a generator +passed as `scopes` is consumed once, at construction, instead of silently emptying after +the first request. + +Relatedly, the `permission_extractor` contract widened from returning `list[str]` to +returning any `Collection[str]`. Every existing extractor that returns a list remains +valid; pydantic coerces whatever collection comes back into the `permissions` set. + +**Why it is safe:** `scopes` was only ever consumed by the scope check, which compares by +set semantics regardless of input type, and widening an accepted return type breaks no +existing implementation. + ### `handle_errors` does not re-wrap an error that is already ours py-buzz's `handle_errors` wraps every exception raised in its block, including one of its @@ -198,6 +240,7 @@ The tables below are the complete list; nothing above adds to or contradicts the | The pytest fixtures live behind the `[test]` extra | A ported test suite cannot import the fixtures from a plain install, because upstream forced `pytest` into every install and this does not | Depend on `armasec-lite[test]` | | The OIDC loader cache is process-wide | Tests that expect per-instance provider state now share it | `openid_config_loader.clear_cache()`, or the `mock_openid_server` fixture, which calls it automatically | | The CLI is not included | `armasec` console script is gone | Out of scope; see [Security](./security/index.md#what-is-out-of-scope) | +| `TokenPayload.permissions` is a `set[str]` | Code that indexes or orders the field, compares it to a list, or asserts byte-stable JSON from a handler that returns the payload as its response body (the encoder serializes the set in nondeterministic order) | Use set operations; for a stable JSON shape use `to_dict()`, which emits a sorted list | **Requires no action, listed so the API diff is complete:** @@ -207,3 +250,4 @@ The tables below are the complete list; nothing above adds to or contradicts the | `JWK` no longer requires `n` and `e` | Strictly more permissive; an EC or OKP key that upstream rejected now parses | | `DomainConfig` requires a non-empty `domain` and a supported `algorithm` | Neither shape ever worked; the failure just moved from request time to construction time. `Armasec()` with no domain still raises its own 422 | | `handle_errors` re-raises an `ArmasecError` subclass unchanged | Everything that is not already ours is still wrapped. Re-wrapping would let the `PayloadMappingError` block turn a genuine 401 into a 500 | +| `TokenSecurity.scopes` is materialized into a frozenset at construction, and `permission_extractor` may return any `Collection[str]` | The constructor accepts the same iterables it always did, the scope check always compared by set semantics, and widening an accepted return type breaks no existing extractor | diff --git a/docusaurus/yarn.lock b/docusaurus/yarn.lock index d2e8387..ebbd90a 100644 --- a/docusaurus/yarn.lock +++ b/docusaurus/yarn.lock @@ -3163,10 +3163,10 @@ dependencies: "@types/yargs-parser" "*" -"@typescript/typescript-linux-x64@7.0.2": +"@typescript/typescript-darwin-arm64@7.0.2": version "7.0.2" - resolved "https://registry.npmjs.org/@typescript/typescript-linux-x64/-/typescript-linux-x64-7.0.2.tgz" - integrity sha512-EYdf2cNg7rgCWJnxCdJ+F3V39O8ihb37eHAu1LK8oAFizgTQbPOK7zHHXbPt8rX24COqODXeI3sIf0fCXG7H/A== + resolved "https://registry.npmjs.org/@typescript/typescript-darwin-arm64/-/typescript-darwin-arm64-7.0.2.tgz" + integrity sha512-gowzar9MwS/aRWp6f3a4KUqzRjAZjOsmGNCM6LcTgXum+dBfgsBVMN+AgvOCCbguXyick6LJhpBszxMebJ8syA== "@ungap/structured-clone@^1.0.0": version "1.4.0" @@ -5373,6 +5373,11 @@ fs-extra@^11.1.1, fs-extra@^11.2.0: jsonfile "^6.0.1" universalify "^2.0.0" +fsevents@~2.3.2: + version "2.3.3" + resolved "https://registry.npmjs.org/fsevents/-/fsevents-2.3.3.tgz" + integrity sha512-5xoDfX+fL7faATnagmWPpbFtwh/R77WmMMqqHGS65C3vvB0YHrgF+B1YmZ3441tMj5n63k0212XNoJwzlhffQw== + function-bind@^1.1.2: version "1.1.2" resolved "https://registry.npmjs.org/function-bind/-/function-bind-1.1.2.tgz" diff --git a/tests/unit/test_token_decoder.py b/tests/unit/test_token_decoder.py index 3a843a5..7178704 100644 --- a/tests/unit/test_token_decoder.py +++ b/tests/unit/test_token_decoder.py @@ -104,7 +104,7 @@ def test_decode_builds_a_token_payload(jwks, rsa_private, now): token = _sign(rsa_private, {"sub": "abc", "exp": now + 60, "permissions": ["read:x"]}) payload = decoder.decode(token) assert payload.sub == "abc" - assert payload.permissions == ["read:x"] + assert payload.permissions == {"read:x"} assert payload.original_token == token @@ -128,7 +128,7 @@ def test_decode_applies_the_permission_extractor(jwks, rsa_private, now): "resource_access": {"my-client": {"roles": ["read:stuff"]}}, }, ) - assert decoder.decode(token).permissions == ["read:stuff"] + assert decoder.decode(token).permissions == {"read:stuff"} def test_decode_raises_payload_mapping_error_when_the_extractor_misses(jwks, rsa_private, now): @@ -168,4 +168,4 @@ def test_decode_restricts_the_algorithm_to_the_configured_one(jwks, rsa_private, def test_extract_keycloak_permissions_reads_the_nested_roles(): decoded = {"azp": "my-client", "resource_access": {"my-client": {"roles": ["read:stuff"]}}} - assert extract_keycloak_permissions(decoded) == ["read:stuff"] + assert extract_keycloak_permissions(decoded) == {"read:stuff"} diff --git a/tests/unit/test_token_payload.py b/tests/unit/test_token_payload.py index 9cd0a0b..cda35da 100644 --- a/tests/unit/test_token_payload.py +++ b/tests/unit/test_token_payload.py @@ -19,7 +19,7 @@ def test_construction_maps_upstream_aliases(): ) assert payload.sub == "abc" assert payload.client_id == "my-client" - assert payload.permissions == ["read:x"] + assert payload.permissions == {"read:x"} assert payload.expire == datetime.fromtimestamp(1735689600, tz=UTC) assert payload.original_token == "the-token" @@ -32,8 +32,18 @@ def test_construction_accepts_the_unaliased_names(): def test_construction_defaults_permissions_to_empty(): - """`permissions` defaults to an empty list when the claim is absent.""" - assert TokenPayload(sub="abc").permissions == [] + """`permissions` defaults to an empty set when the claim is absent.""" + assert TokenPayload(sub="abc").permissions == set() + + +def test_permissions_coerce_from_a_list_and_collapse_duplicates(): + """A `permissions` claim arrives as a JSON array; validation coerces it to a set. + + A token repeating a permission grants it once, which is what the scope check + always meant anyway. + """ + payload = TokenPayload(sub="abc", permissions=["read:x", "read:x", "write:x"]) + assert payload.permissions == {"read:x", "write:x"} def test_construction_requires_sub(): @@ -59,7 +69,7 @@ def test_unknown_attribute_raises_attribute_error(): def test_extra_does_not_shadow_declared_fields(): """A declared field always wins over any same-named extra claim.""" payload = TokenPayload(sub="abc", permissions=["a"]) - assert payload.permissions == ["a"] + assert payload.permissions == {"a"} assert "permissions" not in (payload.model_extra or {}) @@ -74,6 +84,16 @@ def test_to_dict_matches_the_upstream_shape(): } +def test_to_dict_emits_permissions_as_a_sorted_list(): + """`to_dict` renders the permissions set as a sorted list. + + The shim exists to reproduce upstream's JSON-friendly shape, and a set is + neither JSON-serializable nor deterministically ordered. + """ + payload = TokenPayload(sub="abc", permissions=["b", "a", "c"]) + assert payload.to_dict()["permissions"] == ["a", "b", "c"] + + def test_to_dict_without_an_expiry_omits_the_timestamp(): """`to_dict` reports `None` for `exp` when no expiry claim was present.""" payload = TokenPayload(sub="abc") diff --git a/tests/unit/test_token_security.py b/tests/unit/test_token_security.py index 934154f..7e2d649 100644 --- a/tests/unit/test_token_security.py +++ b/tests/unit/test_token_security.py @@ -95,6 +95,18 @@ def _security(**kwargs): return TokenSecurity(**kwargs) +def test_scopes_are_precomputed_as_a_frozenset(): + """Construction materializes `scopes` into a frozenset once. + + `_check_scopes` runs per request, so the set must not be rebuilt there. Materializing + also means a generator passed as `scopes` is consumed exactly once, at construction, + instead of silently emptying after the first request. + """ + security = _security(scopes=(scope for scope in ["read:x", "read:x", "write:x"])) + assert security.scopes == frozenset({"read:x", "write:x"}) + assert isinstance(security.scopes, frozenset) + + async def test_call_returns_the_token_payload(fake_get, make_token): security = _security() payload = await security(_Request({"Authorization": f"Bearer {make_token()}"}))