Repository navigation
feat: add organization policy enforcement - #135
kishore7snehil wants to merge 4 commits into
Conversation
Adds organization_policy ("allow"/"required") and organization_id
allowlist options. When required, verify_access_token now rejects
tokens missing org_id or carrying an org_id outside the allowlist.
Passing organization_id with policy "allow" raises ConfigurationError
at construction time.
Adds coverage for missing org_id under "required" policy, org_id outside the allowlist, an allowed org_id succeeding, default "allow" policy not requiring org_id, and the construction-time ConfigurationError when organization_id is set without policy "required". Each test is co-located with the existing tests for the surface it exercises.
19320d8 to
8f87351
Compare
| return "invalid_token" | ||
|
|
||
|
|
||
| class MissingOrganizationError(VerifyAccessTokenError): |
There was a problem hiding this comment.
These two new error classes are not added to __init__.py (__all__), so they do not get exported from the package.
But the docs show from auth0_api_python import MissingOrganizationError and OrganizationNotAllowedError, which will fail with ImportError.
Can we please export both from the package so the documented import works? The tests pass only because they import from the private errors submodule.
| """Error raised when organization_policy is 'required' but the token has no org_id claim.""" | ||
|
|
||
| def get_error_code(self) -> str: | ||
| return "missing_organization" |
There was a problem hiding this comment.
Small concern on the error code here. missing_organization and organization_not_allowed (line 70) end up as the error value in the WWW-Authenticate header.
RFC 6750 registers only invalid_request, invalid_token and insufficient_scope for that field, and all our other verification failures already return invalid_token.
Can we keep invalid_token on the wire and put the org reason in the error_description? We can still keep these as separate classes for except handling.
| raise ConfigurationError( | ||
| "organization_policy must be either 'required' or 'allow'" | ||
| ) | ||
| if options.organization_id is not None and options.organization_policy != "required": |
There was a problem hiding this comment.
Can we also validate the value of organization_id here itself?
Right now organization_id=[] or "" passes construction but then rejects every token later, and organization_id=123 passes here and then raises a raw TypeError at verify time which turns into a 500.
The domains option already does this kind of check at construction, so it will be good to stay consistent.
| raise VerifyAccessTokenError(f"Missing required claim: {rc}") | ||
|
|
||
| # Organization policy enforcement | ||
| org_id = claims.get("org_id") |
There was a problem hiding this comment.
org_id is read on every call but it is used only in the required branch. Can we move this read inside the if so the default allow path stays clean?
| allowed_orgs = [allowed_orgs] | ||
| if org_id not in allowed_orgs: | ||
| raise OrganizationNotAllowedError( | ||
| f"Organization '{org_id}' is not in the allowed list" |
There was a problem hiding this comment.
I am nitpicking now :D
The token org_id goes into the error message and from there into the error_description of the WWW-Authenticate header as is. The value is read only after full verification so the risk is low, but can we keep a static text in the header and log the org_id separately?
| client_id: Optional[str] = None, | ||
| client_secret: Optional[str] = None, | ||
| timeout: float = 10.0, | ||
| organization_policy: str = "allow", |
There was a problem hiding this comment.
Since only required and allow are valid, can we type this as Literal["required", "allow"]?
That way a wrong value gets caught by the type checker and not only at runtime. The earlier shipped option also used a Literal.
There was a problem hiding this comment.
Typed it as Literal["required", "allow"] in ae99659. No option on main uses Literal today, so this is the first one. It works on Python 3.9 since Literal is in typing from 3.8.
| per-file-ignores = { | ||
| "tests/*" = ["S101", "S105", "S106"], # Allow assert and ignore hardcoded password warnings in test files | ||
| } |
There was a problem hiding this comment.
This per-file-ignores is now an inline table spread across multiple lines with a trailing comma, which is not valid TOML 1.0 (strict parsers like tomllib reject it).
Current ruff still reads it so lint is passing, but it is a bit fragile across tools. Can we use the subtable form [lint.per-file-ignores] instead?
Also this ruff change looks unrelated to the feature, maybe it can go in a separate chore PR.
There was a problem hiding this comment.
Dropped the .ruff.toml change from this PR. It now lives in #140, which uses the [lint] and [lint.per-file-ignores] tables.
| assert "failed to parse token" in str(e.value).lower() | ||
|
|
||
|
|
||
| # ===== Organization Policy: verify_access_token Enforcement ===== |
There was a problem hiding this comment.
A single string organization_id (all tests pass a list, so the str to list branch is never hit), required policy with no allowlist but a token that has an org_id (should pass), allow policy with a token that does carry an org_id, and rejection of an invalid organization_policy value like none at construction.
Can we add these?
8f87351 to
f4b287d
Compare
…_token - Export MissingOrganizationError and OrganizationNotAllowedError from the package root - Keep invalid_token as the error code for organization failures - Validate organization_id at construction and normalize it once - Reject a non-string org_id claim and read it only under the required policy - Keep the org_id out of the error message and log it instead - Type organization_policy as Literal["required", "allow"] - Add tests for the single-string allowlist, no allowlist, allow policy, invalid values and the response header
f4b287d to
ae99659
Compare
📋 Changes
This PR adds organization policy enforcement to auth0-api-python, letting an API require that incoming access tokens carry an
org_idclaim and optionally pin accepted tokens to a specific organization or allowlist.✨ Features
organization_policyoption onApiClientOptions. "allow" accepts tokens with or withoutorg_idand matches existing behavior. "required" rejects tokens withoutorg_id.organization_idoption that pins accepted tokens to a single organization or an allowlist. Valid only when the policy is "required".verify_access_token()now enforces the configured organization policy during verification.MissingOrganizationError(subclassesVerifyAccessTokenError), raised when a token has noorg_idand the policy is "required".OrganizationNotAllowedError(subclassesVerifyAccessTokenError), raised when a token'sorg_idis not in theorganization_idallowlist.🔧 API Changes
organization_policy(str, default"allow") toApiClientOptionsorganization_id(str or list of str, defaultNone) toApiClientOptions, valid only with the "required" policyMissingOrganizationError,OrganizationNotAllowedError(both subclassVerifyAccessTokenError)verify_access_token()now enforces the organization policy📖 Documentation
docs/OrganizationPolicy.mddescribing the policy modes and the organization allowlistREADME.mdwith an organization policy section🧪 Testing
Contributor Checklist