From f506a613f3e22e2b175d354f9a382a37fce20961 Mon Sep 17 00:00:00 2001 From: Juan Date: Mon, 7 Sep 2026 15:06:22 -0500 Subject: [PATCH 1/2] feat: add generic OIDC authentication provider Adds a generic OpenID Connect provider so a self-hosted instance can sign in against any compliant IdP (Okta, Entra ID, Keycloak, Auth0, Authentik, Google). Every endpoint is resolved from the admin-configured issuer via discovery, so a single implementation covers all of them rather than one provider per IdP. The existing providers obtain identity by calling a provider-specific userinfo endpoint with the access token. OIDC instead carries identity in a signed ID token, so the trust anchor moves to the signature. plane/authentication/utils/oidc.py implements that check: discovery document fetch (with the mandatory issuer match), JWKS resolution, and full claim verification (iss, aud, exp, iat, sub, azp) plus a session-bound nonce. Signing algorithms are restricted to asymmetric ones -- a symmetric algorithm would let anyone forge a token by using the issuer's published public key as the HMAC secret. Identity is read only from the verified ID token. The userinfo endpoint is consulted just to fill in profile fields the ID token omitted, and its response is merged only when its sub matches; a response for a different subject is discarded rather than trusted. Unverified emails are rejected by default, as they are for the Google and GitLab providers (GHSA-7j95-vh8g-f365): Plane matches accounts by email, so an address the IdP has not verified is an account-takeover vector. Since some IdPs never emit email_verified, OIDC_ALLOW_UNVERIFIED_EMAIL=1 lets an admin opt out explicitly. Changes to shared auth code are additive only, so no existing provider changes behaviour. Covered by 32 unit tests, including forged-HS256, wrong-audience, wrong-issuer, expired-token, nonce-replay and mismatched-userinfo cases. Co-Authored-By: Claude Opus 5 (1M context) --- apps/api/plane/authentication/adapter/base.py | 11 + .../api/plane/authentication/adapter/error.py | 4 + .../api/plane/authentication/adapter/oauth.py | 2 + .../authentication/provider/oauth/oidc.py | 267 ++++++++++++ apps/api/plane/authentication/urls.py | 17 + apps/api/plane/authentication/utils/oidc.py | 303 +++++++++++++ .../plane/authentication/views/__init__.py | 3 + .../plane/authentication/views/app/oidc.py | 120 +++++ .../plane/authentication/views/space/oidc.py | 125 ++++++ apps/api/plane/license/api/views/instance.py | 13 + .../authentication/test_avatar_download.py | 64 +++ .../tests/unit/authentication/test_oidc.py | 276 ++++++++++++ .../unit/authentication/test_oidc_provider.py | 409 ++++++++++++++++++ .../unit/authentication/test_oidc_views.py | 134 ++++++ .../utils/instance_config_variables/core.py | 51 +++ 15 files changed, 1799 insertions(+) create mode 100644 apps/api/plane/authentication/provider/oauth/oidc.py create mode 100644 apps/api/plane/authentication/utils/oidc.py create mode 100644 apps/api/plane/authentication/views/app/oidc.py create mode 100644 apps/api/plane/authentication/views/space/oidc.py create mode 100644 apps/api/plane/tests/unit/authentication/test_avatar_download.py create mode 100644 apps/api/plane/tests/unit/authentication/test_oidc.py create mode 100644 apps/api/plane/tests/unit/authentication/test_oidc_provider.py create mode 100644 apps/api/plane/tests/unit/authentication/test_oidc_views.py diff --git a/apps/api/plane/authentication/adapter/base.py b/apps/api/plane/authentication/adapter/base.py index 3a616a21c38..342b2a2b388 100644 --- a/apps/api/plane/authentication/adapter/base.py +++ b/apps/api/plane/authentication/adapter/base.py @@ -129,6 +129,7 @@ def check_sync_enabled(self): "github": "ENABLE_GITHUB_SYNC", "gitlab": "ENABLE_GITLAB_SYNC", "gitea": "ENABLE_GITEA_SYNC", + "oidc": "ENABLE_OIDC_SYNC", } config_key = provider_config_map.get(self.provider) if config_key: @@ -144,6 +145,16 @@ def download_and_upload_avatar(self, avatar_url, user): if not avatar_url: return None + # Not every provider hands back a fetchable URL. Authentik, for one, generates a + # `data:image/svg+xml;base64,...` avatar for users with no uploaded picture, and + # OIDC providers in general are free to do the same. There is nothing to download + # from those, and passing one to the SSRF-safe fetcher below raises + # ValueError("Invalid URL scheme...") which log_exception records as a full stack + # trace on *every* sign-in. Returning early leaves the caller to store the value + # as-is, so such avatars still render. + if not str(avatar_url).lower().startswith(("http://", "https://")): + return None + try: headers = self.get_avatar_download_headers() # Download the avatar image over an SSRF-safe client: the avatar URL diff --git a/apps/api/plane/authentication/adapter/error.py b/apps/api/plane/authentication/adapter/error.py index 6d789311020..ca3d2935fef 100644 --- a/apps/api/plane/authentication/adapter/error.py +++ b/apps/api/plane/authentication/adapter/error.py @@ -50,6 +50,10 @@ "GITLAB_OAUTH_PROVIDER_ERROR": 5121, "GITEA_OAUTH_PROVIDER_ERROR": 5123, "OAUTH_PROVIDER_UNVERIFIED_EMAIL": 5124, + # OIDC + "OIDC_NOT_CONFIGURED": 5113, + "OIDC_OAUTH_PROVIDER_ERROR": 5114, + "OIDC_INVALID_ID_TOKEN": 5116, # Reset Password "INVALID_PASSWORD_TOKEN": 5125, "EXPIRED_PASSWORD_TOKEN": 5130, diff --git a/apps/api/plane/authentication/adapter/oauth.py b/apps/api/plane/authentication/adapter/oauth.py index afb1a31325d..843284680c2 100644 --- a/apps/api/plane/authentication/adapter/oauth.py +++ b/apps/api/plane/authentication/adapter/oauth.py @@ -55,6 +55,8 @@ def authentication_error_code(self): return "GITLAB_OAUTH_PROVIDER_ERROR" elif self.provider == "gitea": return "GITEA_OAUTH_PROVIDER_ERROR" + elif self.provider == "oidc": + return "OIDC_OAUTH_PROVIDER_ERROR" else: return "OAUTH_NOT_CONFIGURED" diff --git a/apps/api/plane/authentication/provider/oauth/oidc.py b/apps/api/plane/authentication/provider/oauth/oidc.py new file mode 100644 index 00000000000..fb77be0ab34 --- /dev/null +++ b/apps/api/plane/authentication/provider/oauth/oidc.py @@ -0,0 +1,267 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +# Python imports +import base64 +import os +from datetime import datetime, timedelta +from urllib.parse import urlencode + +import pytz + +# Module imports +from plane.authentication.adapter.oauth import OauthAdapter +from plane.authentication.adapter.error import ( + AUTHENTICATION_ERROR_CODES, + AuthenticationException, +) +from plane.authentication.utils.oidc import ( + get_discovery_document, + normalize_issuer, + validate_id_token, +) +from plane.license.utils.instance_value import get_configuration_value + + +class OIDCProvider(OauthAdapter): + """Generic OpenID Connect provider. + + Unlike the other providers in this package, every endpoint is discovered from + the admin-configured issuer rather than hardcoded, so one implementation serves + any compliant IdP (Okta, Entra ID, Keycloak, Auth0, Authentik, Google). + + Identity comes from the ID token, which is verified in set_token_data() before + any claim is read. The userinfo endpoint is consulted only to fill in profile + fields the ID token omitted, and never to establish who the user is. + """ + + provider = "oidc" + scope = "openid email profile" + + def __init__(self, request, code=None, state=None, nonce=None, callback=None, is_space=False): + ( + OIDC_ISSUER_URL, + OIDC_CLIENT_ID, + OIDC_CLIENT_SECRET, + ) = get_configuration_value( + [ + {"key": "OIDC_ISSUER_URL", "default": os.environ.get("OIDC_ISSUER_URL")}, + {"key": "OIDC_CLIENT_ID", "default": os.environ.get("OIDC_CLIENT_ID")}, + {"key": "OIDC_CLIENT_SECRET", "default": os.environ.get("OIDC_CLIENT_SECRET")}, + ] + ) + + if not (OIDC_ISSUER_URL and OIDC_CLIENT_ID and OIDC_CLIENT_SECRET): + raise AuthenticationException( + error_code=AUTHENTICATION_ERROR_CODES["OIDC_NOT_CONFIGURED"], + error_message="OIDC_NOT_CONFIGURED", + ) + + self.issuer = normalize_issuer(OIDC_ISSUER_URL) + self.nonce = nonce + # Populated by set_token_data() once the ID token has been verified. + self.id_token_claims = None + + # Resolve the provider's endpoints. Cached, so this is a network call only + # on the first login after the cache expires. + self.discovery_document = get_discovery_document(self.issuer) + + authorization_endpoint = self.discovery_document["authorization_endpoint"] + token_url = self.discovery_document["token_endpoint"] + # userinfo_endpoint is optional in the spec; profile fallback is skipped without it. + userinfo_url = self.discovery_document.get("userinfo_endpoint") + + # The /app and /spaces flows have separate callback endpoints and separate + # session keys, so the IdP must be sent back to whichever one started this. + # OAuth also requires the redirect_uri on the token exchange to match the one + # on the authorization request, so the callback leg builds it the same way. + callback_path = "/auth/spaces/oidc/callback/" if is_space else "/auth/oidc/callback/" + redirect_uri = f"""{"https" if request.is_secure() else "http"}://{request.get_host()}{callback_path}""" + + url_params = { + "client_id": OIDC_CLIENT_ID, + "redirect_uri": redirect_uri, + "response_type": "code", + "scope": self.scope, + "state": state, + } + # The nonce is only meaningful on the authorization request; on the callback + # leg it is the value we compare the returned token against instead. + if nonce and state: + url_params["nonce"] = nonce + + auth_url = f"{authorization_endpoint}?{urlencode(url_params)}" + + super().__init__( + request, + self.provider, + OIDC_CLIENT_ID, + self.scope, + redirect_uri, + auth_url, + token_url, + userinfo_url, + OIDC_CLIENT_SECRET, + code, + callback=callback, + ) + + def _token_request_auth(self): + """Return (extra_form_fields, headers) for authenticating to the token endpoint. + + Providers advertise which client authentication methods they accept. Sending + the secret in the form body (client_secret_post) matches what the other + providers here do, but some IdPs only accept HTTP Basic (client_secret_basic), + so honour whatever the discovery document says. + """ + supported = self.discovery_document.get("token_endpoint_auth_methods_supported") or [] + + # OIDC Discovery 1.0 §3 makes this field OPTIONAL and states that when it is + # omitted "the default is client_secret_basic". So an absent list means basic, + # not a free choice: a compliant provider that only accepts the default and + # does not publish the list would reject a form-body secret outright. + # + # When the list *is* published, prefer client_secret_post where offered, which + # is what the other providers in this package send. + use_basic = "client_secret_basic" in supported if supported else True + if supported and "client_secret_post" in supported: + use_basic = False + + if use_basic: + credentials = f"{self.client_id}:{self.client_secret}".encode("utf-8") + encoded = base64.b64encode(credentials).decode("ascii") + return {}, {"Accept": "application/json", "Authorization": f"Basic {encoded}"} + + return ( + {"client_id": self.client_id, "client_secret": self.client_secret}, + {"Accept": "application/json"}, + ) + + def set_token_data(self): + auth_fields, headers = self._token_request_auth() + data = { + "grant_type": "authorization_code", + "code": self.code, + "redirect_uri": self.redirect_uri, + **auth_fields, + } + token_response = self.get_user_token(data=data, headers=headers) + + # Verify the ID token before anything downstream reads a claim from it. + # This is the step that makes the whole flow trustworthy, so it happens + # here rather than in set_user_data(). + self.id_token_claims = validate_id_token( + id_token=token_response.get("id_token"), + discovery_document=self.discovery_document, + client_id=self.client_id, + nonce=self.nonce, + ) + + # expires_in is a duration in seconds from now (RFC 6749 §5.1), unlike the + # absolute timestamps some of the other providers here return. + expires_in = token_response.get("expires_in") + access_token_expired_at = datetime.now(tz=pytz.utc) + timedelta(seconds=int(expires_in)) if expires_in else None + refresh_expires_in = token_response.get("refresh_expires_in") + refresh_token_expired_at = ( + datetime.now(tz=pytz.utc) + timedelta(seconds=int(refresh_expires_in)) if refresh_expires_in else None + ) + + super().set_token_data( + { + "access_token": token_response.get("access_token"), + "refresh_token": token_response.get("refresh_token", None), + "access_token_expired_at": access_token_expired_at, + "refresh_token_expired_at": refresh_token_expired_at, + "id_token": token_response.get("id_token", ""), + } + ) + + def _get_profile_claims(self): + """Merge ID token claims with userinfo, preferring the signed ID token.""" + claims = dict(self.id_token_claims) + + # Consult userinfo only when the ID token is genuinely short of something we + # need: an email to identify the account by, the verification status we gate + # on, or any name to display. Plenty of IdPs put all of it in the ID token, + # and fetching userinfo regardless would add a round trip to every login. + # + # email_verified has to be part of this test, not just email. Providers vary + # in which claims reach the ID token versus userinfo -- Authentik, for one, + # only puts scope-mapped claims in the ID token when the provider enables it + # -- so treating a missing email_verified as "nothing more to fetch" rejects + # logins over a claim the userinfo endpoint would have supplied. + has_email = claims.get("email") is not None + has_verification = claims.get("email_verified") is not None + has_name = any(claims.get(field) is not None for field in ("given_name", "family_name", "name")) + if not self.userinfo_url or (has_email and has_verification and has_name): + return claims + + userinfo = self.get_user_response() + + # The userinfo response is unsigned, so it is merged only once it proves it + # describes the same subject as the verified ID token (OIDC Core 1.0 5.3.2). + # A response that does not is discarded rather than trusted: otherwise a + # provider bug or a swapped response could graft another user's profile, or + # another user's email, onto this session. Discarding rather than failing + # keeps a quirky userinfo endpoint from locking out logins that the ID token + # alone could satisfy; if that leaves no email at all, the adapter's own + # email validation rejects the login further down. + if userinfo.get("sub") != claims.get("sub"): + self.logger.warning("OIDC: discarding userinfo response whose sub does not match the id_token") + return claims + + for field in ("email", "email_verified", "given_name", "family_name", "name", "picture"): + if claims.get(field) is None and userinfo.get(field) is not None: + claims[field] = userinfo[field] + + return claims + + def set_user_data(self): + claims = self._get_profile_claims() + + # Reject unverified emails. An IdP that has not verified the address cannot + # vouch for it, and Plane matches accounts by email — so accepting one lets + # whoever controls that unverified address take over an existing account + # (GHSA-7j95-vh8g-f365). Fail closed: an absent claim counts as unverified. + # + # Not every IdP emits email_verified (Entra ID commonly omits it), so an + # admin whose provider verifies addresses out of band can opt out with + # OIDC_ALLOW_UNVERIFIED_EMAIL=1. Doing so means trusting that IdP to only + # ever assert addresses it controls. + (OIDC_ALLOW_UNVERIFIED_EMAIL,) = get_configuration_value( + [ + { + "key": "OIDC_ALLOW_UNVERIFIED_EMAIL", + "default": os.environ.get("OIDC_ALLOW_UNVERIFIED_EMAIL", "0"), + } + ] + ) + if OIDC_ALLOW_UNVERIFIED_EMAIL != "1" and claims.get("email_verified") is not True: + raise AuthenticationException( + error_code=AUTHENTICATION_ERROR_CODES["OAUTH_PROVIDER_UNVERIFIED_EMAIL"], + error_message="OAUTH_PROVIDER_UNVERIFIED_EMAIL", + ) + + # Some IdPs send only a full `name`; split it so first/last are populated. + first_name = claims.get("given_name") + last_name = claims.get("family_name") + if not first_name and claims.get("name"): + first_name, _, last_name_fallback = str(claims["name"]).partition(" ") + last_name = last_name or last_name_fallback + + super().set_user_data( + { + "email": claims.get("email"), + "user": { + # `sub` is the only claim guaranteed stable for the user at this + # issuer; email can be reassigned, so it must not key the account. + "provider_id": claims.get("sub"), + "email": claims.get("email"), + "avatar": claims.get("picture"), + "first_name": first_name, + "last_name": last_name, + "is_password_autoset": True, + }, + } + ) diff --git a/apps/api/plane/authentication/urls.py b/apps/api/plane/authentication/urls.py index 4bec07db00b..047a96e690c 100644 --- a/apps/api/plane/authentication/urls.py +++ b/apps/api/plane/authentication/urls.py @@ -18,6 +18,8 @@ GitHubOauthInitiateEndpoint, GoogleCallbackEndpoint, GoogleOauthInitiateEndpoint, + OIDCCallbackEndpoint, + OIDCOauthInitiateEndpoint, MagicGenerateEndpoint, MagicSignInEndpoint, MagicSignUpEndpoint, @@ -34,6 +36,8 @@ GitHubOauthInitiateSpaceEndpoint, GoogleCallbackSpaceEndpoint, GoogleOauthInitiateSpaceEndpoint, + OIDCCallbackSpaceEndpoint, + OIDCOauthInitiateSpaceEndpoint, MagicGenerateSpaceEndpoint, MagicSignInSpaceEndpoint, MagicSignUpSpaceEndpoint, @@ -150,4 +154,17 @@ GiteaCallbackSpaceEndpoint.as_view(), name="space-gitea-callback", ), + ## OIDC + path("oidc/", OIDCOauthInitiateEndpoint.as_view(), name="oidc-initiate"), + path("oidc/callback/", OIDCCallbackEndpoint.as_view(), name="oidc-callback"), + path( + "spaces/oidc/", + OIDCOauthInitiateSpaceEndpoint.as_view(), + name="space-oidc-initiate", + ), + path( + "spaces/oidc/callback/", + OIDCCallbackSpaceEndpoint.as_view(), + name="space-oidc-callback", + ), ] diff --git a/apps/api/plane/authentication/utils/oidc.py b/apps/api/plane/authentication/utils/oidc.py new file mode 100644 index 00000000000..d1f1dcb6774 --- /dev/null +++ b/apps/api/plane/authentication/utils/oidc.py @@ -0,0 +1,303 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +"""OpenID Connect discovery and ID token validation. + +The existing OAuth providers (google, github, gitlab, gitea) obtain identity by +calling a provider-specific userinfo endpoint with the access token. OIDC instead +carries identity in a signed ID token, so the trust anchor is the token signature +rather than the transport. Everything in this module exists to make that signature +check correct: getting the right keys, and rejecting every token that does not +prove it came from the configured issuer for this client. +""" + +# Python imports +import hmac +import logging +from urllib.parse import urlparse + +# Third party imports +import jwt +import requests +from jwt import PyJWKClient + +# Django imports +from django.core.cache import cache + +# Module imports +from plane.authentication.adapter.error import ( + AUTHENTICATION_ERROR_CODES, + AuthenticationException, +) + +logger = logging.getLogger("plane.authentication") + +# How long a provider's discovery document is reused before being re-fetched. +DISCOVERY_CACHE_TTL = 60 * 60 # 1 hour +DISCOVERY_CACHE_PREFIX = "oidc:discovery:" + +# Network timeout for discovery and JWKS fetches, in seconds. +NETWORK_TIMEOUT = 10 + +# Signing algorithms accepted for an ID token. +# +# Asymmetric only, and this is a security control rather than a preference. If a +# symmetric algorithm (HS256) were permitted, an attacker could forge a token by +# signing it with the issuer's *public* key as the HMAC secret — the public key +# is published in the JWKS, so it is not a secret at all. PyJWT would then verify +# that forgery successfully because the key material matches. "none" is excluded +# for the same class of reason: it asserts identity with no proof whatsoever. +ALLOWED_SIGNING_ALGORITHMS = frozenset( + {"RS256", "RS384", "RS512", "ES256", "ES384", "ES512", "PS256", "PS384", "PS512"} +) + +# Endpoints a discovery document must advertise for the login flow to work. +REQUIRED_DISCOVERY_FIELDS = ("issuer", "authorization_endpoint", "token_endpoint", "jwks_uri") + +# Every endpoint this module actually calls. userinfo_endpoint is optional in the +# spec, so it is checked only when present. +DISCOVERY_ENDPOINT_FIELDS = ("authorization_endpoint", "token_endpoint", "jwks_uri", "userinfo_endpoint") + +# Tolerated clock skew between this instance and the identity provider, in seconds. +LEEWAY = 60 + + +def _provider_error(message): + """Raise the generic OIDC provider error, logging the specific cause server-side. + + The detail is deliberately not surfaced to the caller: the error travels back to + the browser as a query parameter on the sign-in page. + + Callers must pass a constant description and never interpolate configuration + values. Issuer, client id and client secret all arrive from the same + get_configuration_value() call, so letting any of them reach this log makes the + secret one edit away from being written to disk in clear text. An instance has a + single configured issuer, so a constant message is still unambiguous. + """ + logger.warning("OIDC: %s", message) + raise AuthenticationException( + error_code=AUTHENTICATION_ERROR_CODES["OIDC_OAUTH_PROVIDER_ERROR"], + error_message="OIDC_OAUTH_PROVIDER_ERROR", + ) + + +def normalize_issuer(issuer, *, from_configuration=True): + """Return the issuer without a trailing slash, validating its form. + + The issuer identifier is compared byte-for-byte against the `iss` claim later, + so it has to be normalized once, here, and used consistently everywhere. + + Two kinds of caller reach this, and they differ in whose mistake a bad value is. + The configured issuer is the administrator's to fix, so it fails as + OIDC_NOT_CONFIGURED and the sign-in page tells the user to contact their + administrator. The issuer echoed back inside a discovery document is the + provider's, so that path fails as the generic provider error and says to try + again. Passing both through one error code sent an admin who typed `http://` + chasing a transient fault that would never clear. + """ + + def reject(reason): + if from_configuration: + # Reason strings here are constants: the issuer arrives from the same + # configuration that holds OIDC_CLIENT_SECRET, so none of it is logged. + logger.warning("OIDC: %s", reason) + raise AuthenticationException( + error_code=AUTHENTICATION_ERROR_CODES["OIDC_NOT_CONFIGURED"], + error_message="OIDC_NOT_CONFIGURED", + ) + _provider_error(reason) + + if not issuer: + reject("issuer is empty") + + issuer = str(issuer).strip().rstrip("/") + parsed = urlparse(issuer) + + # OIDC Discovery requires the issuer to be an https URL with no query or + # fragment. Enforcing it here means a misconfigured instance fails at the + # admin panel rather than silently downgrading every login to plaintext. + if parsed.scheme != "https" or not parsed.netloc or parsed.query or parsed.fragment: + reject("issuer is not a valid https URL") + + return issuer + + +def get_discovery_document(issuer): + """Fetch and cache the provider's OpenID configuration. + + Returns the parsed document. Raises AuthenticationException if the provider is + unreachable, malformed, or asserts an issuer other than the configured one. + """ + issuer = normalize_issuer(issuer) + cache_key = f"{DISCOVERY_CACHE_PREFIX}{issuer}" + + document = cache.get(cache_key) + if document: + return document + + discovery_url = f"{issuer}/.well-known/openid-configuration" + try: + response = requests.get(discovery_url, timeout=NETWORK_TIMEOUT) + response.raise_for_status() + document = response.json() + except requests.RequestException: + _provider_error("could not fetch the provider's discovery document") + except ValueError: + _provider_error("discovery document is not valid JSON") + + if not isinstance(document, dict): + _provider_error("discovery document is not a JSON object") + + missing = [field for field in REQUIRED_DISCOVERY_FIELDS if not document.get(field)] + if missing: + # The field names are not interpolated: anything derived from the document + # carries taint from the configured issuer, and that config also holds the + # client secret. REQUIRED_DISCOVERY_FIELDS lists what a document must carry. + _provider_error("discovery document is missing one or more required fields") + + # Validating the issuer's scheme is not enough: the endpoints inside the document + # are separate URLs and may point anywhere. OIDC Discovery 1.0 requires https for + # all of them, and the consequences here are concrete rather than theoretical -- + # token_endpoint receives the authorization code together with the client secret, + # and jwks_uri supplies the keys every ID token signature is checked against. A + # document advertising http:// for either would put the secret on the wire in + # clear text, or let anyone on the path serve their own signing keys. + insecure = [ + field + for field in DISCOVERY_ENDPOINT_FIELDS + if document.get(field) and urlparse(str(document[field])).scheme != "https" + ] + if insecure: + _provider_error("discovery document advertises one or more non-https endpoints") + + # The document must claim the issuer we asked about. Without this check a + # provider could hand back another issuer's endpoints, and tokens minted by + # that third party would then satisfy the `iss` check below (OIDC Discovery + # 1.0 §4.3 makes this comparison mandatory for exactly that reason). + if normalize_issuer(document.get("issuer"), from_configuration=False) != issuer: + _provider_error("discovery document issuer does not match the configured issuer") + + cache.set(cache_key, document, DISCOVERY_CACHE_TTL) + return document + + +def get_signing_algorithms(discovery_document): + """Return the algorithms to accept: the provider's advertised set, filtered to the allowlist.""" + advertised = discovery_document.get("id_token_signing_alg_values_supported") or [] + algorithms = [alg for alg in advertised if alg in ALLOWED_SIGNING_ALGORITHMS] + + if not algorithms: + # Either the provider advertises nothing usable, or it only offers + # symmetric/none signing. Both are refusals rather than fallbacks: there is + # no safe default to substitute here. + # The advertised list is deliberately not logged, for the reason above. Check + # id_token_signing_alg_values_supported in the provider's own metadata. + _provider_error("provider advertises no supported ID token signing algorithm") + + return algorithms + + +# PyJWKClient instances, keyed by jwks_uri. Each one holds its own key-set cache +# with a TTL, so keeping them alive is what makes that cache effective. The set of +# issuers on an instance is bounded by configuration, so this cannot grow unbounded. +_jwk_clients = {} + + +def _get_jwk_client(jwks_uri): + """Return a cached PyJWKClient for the given JWKS endpoint.""" + client = _jwk_clients.get(jwks_uri) + if client is None: + client = PyJWKClient(jwks_uri, cache_jwk_set=True, lifespan=DISCOVERY_CACHE_TTL) + _jwk_clients[jwks_uri] = client + return client + + +def validate_id_token(id_token, discovery_document, client_id, nonce=None): + """Verify an ID token's signature and claims, returning its payload. + + Every parameter is part of the security boundary: + * the signature is checked against the issuer's published JWKS, + * `iss` must equal the configured issuer, + * `aud` must contain this instance's client_id, + * `exp`/`iat` must be current, + * `nonce` must match the value this instance put in the session. + """ + if not id_token: + logger.warning("OIDC: token response contained no id_token") + raise AuthenticationException( + error_code=AUTHENTICATION_ERROR_CODES["OIDC_INVALID_ID_TOKEN"], + error_message="OIDC_INVALID_ID_TOKEN", + ) + + issuer = discovery_document["issuer"] + algorithms = get_signing_algorithms(discovery_document) + + try: + # PyJWKClient reads the token header to select the matching key by `kid`. + # Its key-set cache lives on the instance, so the client itself is reused + # across logins — a fresh client per request would refetch the JWKS every + # single time and make the cache pointless. + jwk_client = _get_jwk_client(discovery_document["jwks_uri"]) + signing_key = jwk_client.get_signing_key_from_jwt(id_token) + except (jwt.PyJWTError, requests.RequestException) as e: + logger.warning("OIDC: could not resolve signing key for id_token: %s", e) + raise AuthenticationException( + error_code=AUTHENTICATION_ERROR_CODES["OIDC_INVALID_ID_TOKEN"], + error_message="OIDC_INVALID_ID_TOKEN", + ) + + try: + claims = jwt.decode( + id_token, + signing_key.key, + algorithms=algorithms, + audience=client_id, + issuer=issuer, + leeway=LEEWAY, + options={ + "verify_signature": True, + "verify_exp": True, + "verify_iat": True, + "verify_aud": True, + "verify_iss": True, + "require": ["iss", "sub", "aud", "exp", "iat"], + }, + ) + except jwt.PyJWTError as e: + # Covers expired, wrong audience, wrong issuer, bad signature and missing + # required claims. The specific reason is logged but never returned. + logger.warning("OIDC: id_token rejected: %s", e) + raise AuthenticationException( + error_code=AUTHENTICATION_ERROR_CODES["OIDC_INVALID_ID_TOKEN"], + error_message="OIDC_INVALID_ID_TOKEN", + ) + + # When the token is issued to more than one audience, OIDC Core §3.1.3.7 requires + # an `azp` claim naming the party the token was actually issued to. PyJWT's + # audience check passes as long as our client_id appears anywhere in the list, + # so without this a token minted for a different client at the same issuer would + # be accepted here. + audience = claims.get("aud") + if isinstance(audience, (list, tuple)) and len(audience) > 1: + if claims.get("azp") != client_id: + # Neither value is logged: client_id comes from instance configuration. + logger.warning("OIDC: id_token lists multiple audiences and azp does not name this client") + raise AuthenticationException( + error_code=AUTHENTICATION_ERROR_CODES["OIDC_INVALID_ID_TOKEN"], + error_message="OIDC_INVALID_ID_TOKEN", + ) + + # The nonce binds this token to the browser session that started the flow, which + # is what stops a token replayed from another session being accepted here. + # Compared in constant time, and a missing claim fails closed. + if nonce is not None: + token_nonce = claims.get("nonce") + if not token_nonce or not hmac.compare_digest(str(token_nonce), str(nonce)): + logger.warning("OIDC: id_token nonce mismatch") + raise AuthenticationException( + error_code=AUTHENTICATION_ERROR_CODES["OIDC_INVALID_ID_TOKEN"], + error_message="OIDC_INVALID_ID_TOKEN", + ) + + return claims diff --git a/apps/api/plane/authentication/views/__init__.py b/apps/api/plane/authentication/views/__init__.py index a9c816ae9ea..109d18db96e 100644 --- a/apps/api/plane/authentication/views/__init__.py +++ b/apps/api/plane/authentication/views/__init__.py @@ -11,6 +11,7 @@ from .app.gitlab import GitLabCallbackEndpoint, GitLabOauthInitiateEndpoint from .app.gitea import GiteaCallbackEndpoint, GiteaOauthInitiateEndpoint from .app.google import GoogleCallbackEndpoint, GoogleOauthInitiateEndpoint +from .app.oidc import OIDCCallbackEndpoint, OIDCOauthInitiateEndpoint from .app.magic import MagicGenerateEndpoint, MagicSignInEndpoint, MagicSignUpEndpoint from .app.signout import SignOutAuthEndpoint @@ -26,6 +27,8 @@ from .space.google import GoogleCallbackSpaceEndpoint, GoogleOauthInitiateSpaceEndpoint +from .space.oidc import OIDCCallbackSpaceEndpoint, OIDCOauthInitiateSpaceEndpoint + from .space.magic import ( MagicGenerateSpaceEndpoint, MagicSignInSpaceEndpoint, diff --git a/apps/api/plane/authentication/views/app/oidc.py b/apps/api/plane/authentication/views/app/oidc.py new file mode 100644 index 00000000000..1435e1b6b60 --- /dev/null +++ b/apps/api/plane/authentication/views/app/oidc.py @@ -0,0 +1,120 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +# Python imports +import uuid + +# Django import +from django.http import HttpResponseRedirect +from django.views import View + +# Module imports +from plane.authentication.provider.oauth.oidc import OIDCProvider +from plane.authentication.utils.login import user_login +from plane.authentication.utils.redirection_path import get_redirection_path +from plane.authentication.utils.user_auth_workflow import post_user_auth_workflow +from plane.license.models import Instance +from plane.authentication.utils.host import base_host +from plane.authentication.adapter.error import ( + AuthenticationException, + AUTHENTICATION_ERROR_CODES, +) +from plane.utils.path_validator import get_safe_redirect_url + +# Session keys. Namespaced so a concurrent sign-in through another provider cannot +# overwrite the values this flow will check on the way back. +STATE_SESSION_KEY = "oidc_state" +NONCE_SESSION_KEY = "oidc_nonce" + + +class OIDCOauthInitiateEndpoint(View): + def get(self, request): + request.session["host"] = base_host(request=request, is_app=True) + next_path = request.GET.get("next_path") + if next_path: + request.session["next_path"] = str(next_path) + + # Check instance configuration + instance = Instance.objects.first() + if instance is None or not instance.is_setup_done: + exc = AuthenticationException( + error_code=AUTHENTICATION_ERROR_CODES["INSTANCE_NOT_CONFIGURED"], + error_message="INSTANCE_NOT_CONFIGURED", + ) + params = exc.get_error_dict() + url = get_safe_redirect_url( + base_url=base_host(request=request, is_app=True), next_path=next_path, params=params + ) + return HttpResponseRedirect(url) + + try: + state = uuid.uuid4().hex + nonce = uuid.uuid4().hex + provider = OIDCProvider(request=request, state=state, nonce=nonce) + request.session[STATE_SESSION_KEY] = state + request.session[NONCE_SESSION_KEY] = nonce + auth_url = provider.get_auth_url() + return HttpResponseRedirect(auth_url) + except AuthenticationException as e: + params = e.get_error_dict() + url = get_safe_redirect_url( + base_url=base_host(request=request, is_app=True), next_path=next_path, params=params + ) + return HttpResponseRedirect(url) + + +class OIDCCallbackEndpoint(View): + def get(self, request): + code = request.GET.get("code") + state = request.GET.get("state") + next_path = request.session.get("next_path") + + # Both values are single-use: popping them means a replayed callback finds + # nothing to match against, even if the authorization code is still live. + expected_state = request.session.pop(STATE_SESSION_KEY, None) + nonce = request.session.pop(NONCE_SESSION_KEY, None) + + # A missing nonce means this session did not start the flow: state and nonce + # are always stored together. Without it the ID token would be validated with + # no session binding at all, so this fails closed exactly like a bad state. + if not state or not expected_state or state != expected_state or not nonce: + exc = AuthenticationException( + error_code=AUTHENTICATION_ERROR_CODES["OIDC_OAUTH_PROVIDER_ERROR"], + error_message="OIDC_OAUTH_PROVIDER_ERROR", + ) + params = exc.get_error_dict() + url = get_safe_redirect_url( + base_url=base_host(request=request, is_app=True), next_path=next_path, params=params + ) + return HttpResponseRedirect(url) + + if not code: + exc = AuthenticationException( + error_code=AUTHENTICATION_ERROR_CODES["OIDC_OAUTH_PROVIDER_ERROR"], + error_message="OIDC_OAUTH_PROVIDER_ERROR", + ) + params = exc.get_error_dict() + url = get_safe_redirect_url( + base_url=base_host(request=request, is_app=True), next_path=next_path, params=params + ) + return HttpResponseRedirect(url) + + try: + provider = OIDCProvider(request=request, code=code, nonce=nonce, callback=post_user_auth_workflow) + user = provider.authenticate() + # Login the user and record his device info + user_login(request=request, user=user, is_app=True) + # Get the redirection path + if next_path: + path = next_path + else: + path = get_redirection_path(user=user) + url = get_safe_redirect_url(base_url=base_host(request=request, is_app=True), next_path=path, params={}) + return HttpResponseRedirect(url) + except AuthenticationException as e: + params = e.get_error_dict() + url = get_safe_redirect_url( + base_url=base_host(request=request, is_app=True), next_path=next_path, params=params + ) + return HttpResponseRedirect(url) diff --git a/apps/api/plane/authentication/views/space/oidc.py b/apps/api/plane/authentication/views/space/oidc.py new file mode 100644 index 00000000000..8b580097c98 --- /dev/null +++ b/apps/api/plane/authentication/views/space/oidc.py @@ -0,0 +1,125 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +# Python imports +import uuid + +# Django import +from django.http import HttpResponseRedirect +from django.views import View +from django.utils.http import url_has_allowed_host_and_scheme + +# Module imports +from plane.authentication.provider.oauth.oidc import OIDCProvider +from plane.authentication.utils.login import user_login +from plane.license.models import Instance +from plane.authentication.utils.host import base_host +from plane.authentication.adapter.error import ( + AuthenticationException, + AUTHENTICATION_ERROR_CODES, +) +from plane.utils.path_validator import get_safe_redirect_url, validate_next_path, get_allowed_hosts + +# Session keys, namespaced separately from the /app flow so the two cannot clobber +# each other if a user has both open. +STATE_SESSION_KEY = "oidc_space_state" +NONCE_SESSION_KEY = "oidc_space_nonce" + + +class OIDCOauthInitiateSpaceEndpoint(View): + def get(self, request): + request.session["host"] = base_host(request=request, is_space=True) + next_path = request.GET.get("next_path") + # Stored in the session because the IdP redirects to a bare redirect_uri that + # carries no query string, so the callback has no other way to recover it. + if next_path: + request.session["next_path"] = str(next_path) + + # Check instance configuration + instance = Instance.objects.first() + if instance is None or not instance.is_setup_done: + exc = AuthenticationException( + error_code=AUTHENTICATION_ERROR_CODES["INSTANCE_NOT_CONFIGURED"], + error_message="INSTANCE_NOT_CONFIGURED", + ) + params = exc.get_error_dict() + url = get_safe_redirect_url( + base_url=base_host(request=request, is_space=True), next_path=next_path, params=params + ) + return HttpResponseRedirect(url) + + try: + state = uuid.uuid4().hex + nonce = uuid.uuid4().hex + provider = OIDCProvider(request=request, state=state, nonce=nonce, is_space=True) + request.session[STATE_SESSION_KEY] = state + request.session[NONCE_SESSION_KEY] = nonce + auth_url = provider.get_auth_url() + return HttpResponseRedirect(auth_url) + except AuthenticationException as e: + params = e.get_error_dict() + url = get_safe_redirect_url( + base_url=base_host(request=request, is_space=True), next_path=next_path, params=params + ) + return HttpResponseRedirect(url) + + +class OIDCCallbackSpaceEndpoint(View): + def get(self, request): + code = request.GET.get("code") + state = request.GET.get("state") + next_path = request.session.get("next_path") + + # Single-use: see the /app callback for why these are popped. + expected_state = request.session.pop(STATE_SESSION_KEY, None) + nonce = request.session.pop(NONCE_SESSION_KEY, None) + + # A missing nonce means this session did not start the flow: state and nonce + # are always stored together. Without it the ID token would be validated with + # no session binding at all, so this fails closed exactly like a bad state. + if not state or not expected_state or state != expected_state or not nonce: + exc = AuthenticationException( + error_code=AUTHENTICATION_ERROR_CODES["OIDC_OAUTH_PROVIDER_ERROR"], + error_message="OIDC_OAUTH_PROVIDER_ERROR", + ) + params = exc.get_error_dict() + url = get_safe_redirect_url( + base_url=base_host(request=request, is_space=True), next_path=next_path, params=params + ) + return HttpResponseRedirect(url) + + if not code: + exc = AuthenticationException( + error_code=AUTHENTICATION_ERROR_CODES["OIDC_OAUTH_PROVIDER_ERROR"], + error_message="OIDC_OAUTH_PROVIDER_ERROR", + ) + params = exc.get_error_dict() + url = get_safe_redirect_url( + base_url=base_host(request=request, is_space=True), next_path=next_path, params=params + ) + return HttpResponseRedirect(url) + + try: + provider = OIDCProvider(request=request, code=code, nonce=nonce, is_space=True) + user = provider.authenticate() + # Login the user and record his device info + user_login(request=request, user=user, is_space=True) + # redirect to referer path + next_path = validate_next_path(next_path=next_path) + + # base_host() ends with SPACE_BASE_PATH's trailing slash. Strip it only + # when a next_path follows, otherwise the redirect lands on /spaces and the + # app rejects it: its configured base URL is /spaces/, so the bare form 404s. + space_base_url = base_host(request=request, is_space=True) + url = f"{space_base_url.rstrip('/')}{next_path}" if next_path else space_base_url + if url_has_allowed_host_and_scheme(url, allowed_hosts=get_allowed_hosts()): + return HttpResponseRedirect(url) + else: + return HttpResponseRedirect(base_host(request=request, is_space=True)) + except AuthenticationException as e: + params = e.get_error_dict() + url = get_safe_redirect_url( + base_url=base_host(request=request, is_space=True), next_path=next_path, params=params + ) + return HttpResponseRedirect(url) diff --git a/apps/api/plane/license/api/views/instance.py b/apps/api/plane/license/api/views/instance.py index a805411eee6..088fd2101a0 100644 --- a/apps/api/plane/license/api/views/instance.py +++ b/apps/api/plane/license/api/views/instance.py @@ -55,6 +55,8 @@ def get(self, request): GITHUB_APP_NAME, IS_GITLAB_ENABLED, IS_GITEA_ENABLED, + IS_OIDC_ENABLED, + OIDC_PROVIDER_NAME, EMAIL_HOST, ENABLE_MAGIC_LINK_LOGIN, ENABLE_EMAIL_PASSWORD, @@ -91,6 +93,14 @@ def get(self, request): "key": "IS_GITEA_ENABLED", "default": os.environ.get("IS_GITEA_ENABLED", "0"), }, + { + "key": "IS_OIDC_ENABLED", + "default": os.environ.get("IS_OIDC_ENABLED", "0"), + }, + { + "key": "OIDC_PROVIDER_NAME", + "default": os.environ.get("OIDC_PROVIDER_NAME", "SSO"), + }, {"key": "EMAIL_HOST", "default": os.environ.get("EMAIL_HOST", "")}, { "key": "ENABLE_MAGIC_LINK_LOGIN", @@ -123,6 +133,9 @@ def get(self, request): data["is_github_enabled"] = IS_GITHUB_ENABLED == "1" data["is_gitlab_enabled"] = IS_GITLAB_ENABLED == "1" data["is_gitea_enabled"] = IS_GITEA_ENABLED == "1" + data["is_oidc_enabled"] = IS_OIDC_ENABLED == "1" + # Label for the sign-in button; the provider is generic so the admin names it. + data["oidc_provider_name"] = OIDC_PROVIDER_NAME or "SSO" data["is_magic_login_enabled"] = ENABLE_MAGIC_LINK_LOGIN == "1" data["is_email_password_enabled"] = ENABLE_EMAIL_PASSWORD == "1" diff --git a/apps/api/plane/tests/unit/authentication/test_avatar_download.py b/apps/api/plane/tests/unit/authentication/test_avatar_download.py new file mode 100644 index 00000000000..b8be461f1eb --- /dev/null +++ b/apps/api/plane/tests/unit/authentication/test_avatar_download.py @@ -0,0 +1,64 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +"""Avatar download guard on the shared OAuth adapter. + +Providers are free to return something other than a fetchable URL. Authentik generates a +`data:image/svg+xml;base64,...` avatar for users with no uploaded picture, and passing that +to the SSRF-safe fetcher raises ValueError("Invalid URL scheme..."), which the surrounding +`except Exception` records via log_exception — a full stack trace on every single sign-in. +The guard returns early instead, so the caller stores the value as-is and the avatar still +renders. +""" + +from unittest.mock import MagicMock, patch + +import pytest + +from plane.authentication.adapter.base import Adapter + +DATA_URI = "data:image/svg+xml;base64,PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciPjwvc3ZnPg==" + + +@pytest.fixture +def adapter(): + return Adapter(request=MagicMock(), provider="oidc") + + +@pytest.mark.unit +class TestDownloadAndUploadAvatar: + @pytest.mark.parametrize( + "avatar_url", + [DATA_URI, "ftp://example.com/a.png", "file:///etc/passwd", "javascript:alert(1)"], + ids=["data-uri", "ftp", "file", "javascript"], + ) + def test_non_http_urls_are_skipped_without_fetching_or_logging(self, adapter, avatar_url): + with ( + patch("plane.authentication.adapter.base.pinned_fetch_following_redirects") as fetch, + patch("plane.authentication.adapter.base.log_exception") as log_exception, + ): + assert adapter.download_and_upload_avatar(avatar_url, user=MagicMock()) is None + + fetch.assert_not_called() + # The point of the guard: no stack trace per sign-in. + log_exception.assert_not_called() + + @pytest.mark.parametrize("avatar_url", [None, ""], ids=["none", "empty"]) + def test_absent_avatar_is_skipped(self, adapter, avatar_url): + with patch("plane.authentication.adapter.base.pinned_fetch_following_redirects") as fetch: + assert adapter.download_and_upload_avatar(avatar_url, user=MagicMock()) is None + fetch.assert_not_called() + + @pytest.mark.parametrize( + "avatar_url", + ["https://example.com/a.png", "http://example.com/a.png", "HTTPS://EXAMPLE.COM/a.png"], + ids=["https", "http", "uppercase-scheme"], + ) + def test_http_urls_still_reach_the_fetcher(self, adapter, avatar_url): + """The guard must not change behaviour for the providers that return real URLs.""" + with patch("plane.authentication.adapter.base.pinned_fetch_following_redirects") as fetch: + fetch.side_effect = RuntimeError("stop here") + with patch("plane.authentication.adapter.base.log_exception"): + adapter.download_and_upload_avatar(avatar_url, user=MagicMock()) + fetch.assert_called_once() diff --git a/apps/api/plane/tests/unit/authentication/test_oidc.py b/apps/api/plane/tests/unit/authentication/test_oidc.py new file mode 100644 index 00000000000..610bdba586c --- /dev/null +++ b/apps/api/plane/tests/unit/authentication/test_oidc.py @@ -0,0 +1,276 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +"""ID token validation tests for the generic OIDC provider. + +The ID token is the only thing proving who the user is, so every rejection path +here is a security control rather than input validation. The forged-HS256 case in +particular guards the classic algorithm-confusion attack: the issuer's public key +is published in its JWKS, so if a symmetric algorithm were ever accepted, anyone +could mint a valid-looking token by using that public key as the HMAC secret. +""" + +import base64 +import hashlib +import hmac +import json +import time +from datetime import datetime, timedelta, timezone + +import jwt +import pytest +from cryptography.hazmat.primitives import serialization +from cryptography.hazmat.primitives.asymmetric import rsa + +from plane.authentication.adapter.error import AuthenticationException +from plane.authentication.utils import oidc as oidc_utils + +ISSUER = "https://idp.example.com" +CLIENT_ID = "plane-client" + + +def _generate_keypair(): + key = rsa.generate_private_key(public_exponent=65537, key_size=2048) + private_pem = key.private_bytes( + serialization.Encoding.PEM, + serialization.PrivateFormat.PKCS8, + serialization.NoEncryption(), + ).decode() + public_pem = ( + key.public_key() + .public_bytes(serialization.Encoding.PEM, serialization.PublicFormat.SubjectPublicKeyInfo) + .decode() + ) + return private_pem, public_pem + + +PRIVATE_KEY, PUBLIC_KEY = _generate_keypair() +OTHER_PRIVATE_KEY, _ = _generate_keypair() + +DISCOVERY = { + "issuer": ISSUER, + "authorization_endpoint": f"{ISSUER}/authorize", + "token_endpoint": f"{ISSUER}/token", + "jwks_uri": f"{ISSUER}/jwks", + "id_token_signing_alg_values_supported": ["RS256"], +} + + +@pytest.fixture(autouse=True) +def stub_jwks(monkeypatch): + """Serve the public key directly instead of fetching the issuer's JWKS.""" + + class _Key: + key = PUBLIC_KEY + + class _Client: + def get_signing_key_from_jwt(self, token): + return _Key() + + monkeypatch.setattr(oidc_utils, "_get_jwk_client", lambda uri: _Client()) + + +def make_token(claims=None, key=PRIVATE_KEY): + now = datetime.now(tz=timezone.utc) + payload = { + "iss": ISSUER, + "sub": "user-123", + "aud": CLIENT_ID, + "iat": now, + "exp": now + timedelta(minutes=5), + } + payload.update(claims or {}) + return jwt.encode(payload, key, algorithm="RS256") + + +def validate(token, nonce=None): + return oidc_utils.validate_id_token(token, DISCOVERY, CLIENT_ID, nonce=nonce) + + +@pytest.mark.unit +class TestValidateIdToken: + def test_valid_token_is_accepted(self): + claims = validate(make_token()) + assert claims["sub"] == "user-123" + + def test_missing_token_is_rejected(self): + with pytest.raises(AuthenticationException): + validate(None) + + def test_expired_token_is_rejected(self): + expired = make_token({"exp": datetime.now(tz=timezone.utc) - timedelta(hours=1)}) + with pytest.raises(AuthenticationException): + validate(expired) + + def test_wrong_audience_is_rejected(self): + with pytest.raises(AuthenticationException): + validate(make_token({"aud": "a-different-client"})) + + def test_wrong_issuer_is_rejected(self): + with pytest.raises(AuthenticationException): + validate(make_token({"iss": "https://evil.example.com"})) + + def test_token_signed_by_another_key_is_rejected(self): + with pytest.raises(AuthenticationException): + validate(make_token(key=OTHER_PRIVATE_KEY)) + + def test_missing_sub_is_rejected(self): + now = datetime.now(tz=timezone.utc) + token = jwt.encode( + {"iss": ISSUER, "aud": CLIENT_ID, "iat": now, "exp": now + timedelta(minutes=5)}, + PRIVATE_KEY, + algorithm="RS256", + ) + with pytest.raises(AuthenticationException): + validate(token) + + def test_forged_hs256_token_is_rejected(self): + """Algorithm confusion: HMAC the token with the issuer's public key. + + Assembled by hand because PyJWT's encode() refuses to build it — which is + precisely what an attacker would do. + """ + + def b64u(raw): + return base64.urlsafe_b64encode(raw).rstrip(b"=") + + now = int(time.time()) + header = b64u(json.dumps({"alg": "HS256", "typ": "JWT", "kid": "k1"}).encode()) + payload = b64u( + json.dumps({"iss": ISSUER, "sub": "attacker", "aud": CLIENT_ID, "iat": now, "exp": now + 300}).encode() + ) + signing_input = header + b"." + payload + signature = b64u(hmac.new(PUBLIC_KEY.encode(), signing_input, hashlib.sha256).digest()) + forged = (signing_input + b"." + signature).decode() + + with pytest.raises(AuthenticationException): + validate(forged) + + +@pytest.mark.unit +class TestNonceBinding: + def test_matching_nonce_is_accepted(self): + assert validate(make_token({"nonce": "abc"}), nonce="abc")["nonce"] == "abc" + + def test_mismatched_nonce_is_rejected(self): + with pytest.raises(AuthenticationException): + validate(make_token({"nonce": "abc"}), nonce="xyz") + + def test_absent_nonce_claim_is_rejected_when_one_was_sent(self): + with pytest.raises(AuthenticationException): + validate(make_token(), nonce="abc") + + +@pytest.mark.unit +class TestMultipleAudiences: + def test_correct_azp_is_accepted(self): + token = make_token({"aud": [CLIENT_ID, "other-client"], "azp": CLIENT_ID}) + assert validate(token)["sub"] == "user-123" + + def test_azp_naming_another_client_is_rejected(self): + token = make_token({"aud": [CLIENT_ID, "other-client"], "azp": "other-client"}) + with pytest.raises(AuthenticationException): + validate(token) + + def test_missing_azp_is_rejected(self): + token = make_token({"aud": [CLIENT_ID, "other-client"]}) + with pytest.raises(AuthenticationException): + validate(token) + + +@pytest.mark.unit +class TestDiscoveryEndpointSchemes: + """Every endpoint in the document is a separate URL and may point anywhere. + + token_endpoint receives the authorization code together with the client secret, + and jwks_uri supplies the keys every signature is checked against, so a document + advertising http:// for either is refused rather than followed. + """ + + @pytest.fixture(autouse=True) + def isolated_cache(self, monkeypatch): + """Give each test its own discovery cache. + + The module caches documents through Django's cache, which under the test + settings is Redis. Reaching for it would make these tests order-dependent on + each other and dependent on a running backend, neither of which belongs in a + unit test. + """ + + class _Cache: + def __init__(self): + self.store = {} + + def get(self, key): + return self.store.get(key) + + def set(self, key, value, ttl=None): + self.store[key] = value + + monkeypatch.setattr(oidc_utils, "cache", _Cache()) + + def _fetch(self, document, monkeypatch): + class _Response: + @staticmethod + def raise_for_status(): + return None + + @staticmethod + def json(): + return document + + monkeypatch.setattr(oidc_utils.requests, "get", lambda *a, **k: _Response()) + return oidc_utils.get_discovery_document(ISSUER) + + def test_an_https_document_is_accepted(self, monkeypatch): + assert self._fetch(DISCOVERY, monkeypatch)["issuer"] == ISSUER + + @pytest.mark.parametrize( + "field", + ["authorization_endpoint", "token_endpoint", "jwks_uri", "userinfo_endpoint"], + ) + def test_a_plaintext_endpoint_is_refused(self, field, monkeypatch): + document = {**DISCOVERY, "userinfo_endpoint": f"{ISSUER}/userinfo"} + document[field] = document[field].replace("https://", "http://") + with pytest.raises(AuthenticationException) as exc: + self._fetch(document, monkeypatch) + assert exc.value.error_message == "OIDC_OAUTH_PROVIDER_ERROR" + + +@pytest.mark.unit +class TestSigningAlgorithms: + def test_symmetric_only_provider_is_refused(self): + document = {**DISCOVERY, "id_token_signing_alg_values_supported": ["HS256", "none"]} + with pytest.raises(AuthenticationException): + oidc_utils.get_signing_algorithms(document) + + def test_asymmetric_algorithms_are_kept(self): + document = {**DISCOVERY, "id_token_signing_alg_values_supported": ["RS256", "HS256", "ES256"]} + assert oidc_utils.get_signing_algorithms(document) == ["RS256", "ES256"] + + +@pytest.mark.unit +class TestNormalizeIssuer: + def test_trailing_slash_is_stripped(self): + assert oidc_utils.normalize_issuer("https://idp.example.com/") == "https://idp.example.com" + + @pytest.mark.parametrize( + "issuer", + ["", None, "http://idp.example.com", "https://idp.example.com?x=1", "https://idp.example.com#f", "https://"], + ids=["empty", "none", "plaintext", "querystring", "fragment", "no-host"], + ) + def test_a_bad_configured_issuer_reads_as_misconfiguration(self, issuer): + """The admin typed this, so the sign-in page must say "contact your + administrator" rather than "try again" — the latter sends someone chasing a + transient fault that will never clear.""" + with pytest.raises(AuthenticationException) as exc: + oidc_utils.normalize_issuer(issuer) + assert exc.value.error_message == "OIDC_NOT_CONFIGURED" + + @pytest.mark.parametrize("issuer", ["", "http://idp.example.com"], ids=["empty", "plaintext"]) + def test_a_bad_issuer_inside_a_discovery_document_blames_the_provider(self, issuer): + """Same validation, different fault: this value came back from the provider.""" + with pytest.raises(AuthenticationException) as exc: + oidc_utils.normalize_issuer(issuer, from_configuration=False) + assert exc.value.error_message == "OIDC_OAUTH_PROVIDER_ERROR" diff --git a/apps/api/plane/tests/unit/authentication/test_oidc_provider.py b/apps/api/plane/tests/unit/authentication/test_oidc_provider.py new file mode 100644 index 00000000000..1e7d3b63175 --- /dev/null +++ b/apps/api/plane/tests/unit/authentication/test_oidc_provider.py @@ -0,0 +1,409 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +"""Flow-level tests for OIDCProvider against a mock identity provider. + +These cover what the provider does with a token response once the signature has +been checked: which claims become the Plane user, when the userinfo endpoint is +consulted, and which responses are refused. Token signature and claim validation +itself lives in test_oidc.py. +""" + +import base64 +from datetime import datetime, timedelta, timezone +from unittest.mock import MagicMock, patch + +import jwt +import pytest +from cryptography.hazmat.primitives import serialization +from cryptography.hazmat.primitives.asymmetric import rsa + +from plane.authentication.adapter.error import AuthenticationException +from plane.authentication.provider.oauth import oidc as provider_module +from plane.authentication.provider.oauth.oidc import OIDCProvider +from plane.authentication.utils import oidc as oidc_utils + +ISSUER = "https://idp.example.com" +CLIENT_ID = "plane-client" +CLIENT_SECRET = "client-secret" +SUBJECT = "sub-abc" + + +def _generate_keypair(): + key = rsa.generate_private_key(public_exponent=65537, key_size=2048) + private_pem = key.private_bytes( + serialization.Encoding.PEM, + serialization.PrivateFormat.PKCS8, + serialization.NoEncryption(), + ).decode() + public_pem = ( + key.public_key() + .public_bytes(serialization.Encoding.PEM, serialization.PublicFormat.SubjectPublicKeyInfo) + .decode() + ) + return private_pem, public_pem + + +PRIVATE_KEY, PUBLIC_KEY = _generate_keypair() + +BASE_CONFIG = { + "OIDC_ISSUER_URL": ISSUER, + "OIDC_CLIENT_ID": CLIENT_ID, + "OIDC_CLIENT_SECRET": CLIENT_SECRET, + "OIDC_ALLOW_UNVERIFIED_EMAIL": "0", +} + + +def discovery(**overrides): + document = { + "issuer": ISSUER, + "authorization_endpoint": f"{ISSUER}/authorize", + "token_endpoint": f"{ISSUER}/token", + "userinfo_endpoint": f"{ISSUER}/userinfo", + "jwks_uri": f"{ISSUER}/jwks", + "id_token_signing_alg_values_supported": ["RS256"], + } + document.update(overrides) + return document + + +class FakeRequest: + """Stands in for the Django request the provider reads the host from.""" + + session = {} + # save_user_data() copies REMOTE_ADDR and the user agent onto the user row, and both + # users.last_login_ip and users.last_login_uagent are NOT NULL, so an empty META fails + # the insert on any test that provisions a user. + META = {"REMOTE_ADDR": "127.0.0.1", "HTTP_USER_AGENT": "pytest"} + + def is_secure(self): + return True + + def get_host(self): + return "plane.example.com" + + +@pytest.fixture(autouse=True) +def stub_jwks(monkeypatch): + class _Key: + key = PUBLIC_KEY + + class _Client: + def get_signing_key_from_jwt(self, token): + return _Key() + + monkeypatch.setattr(oidc_utils, "_get_jwk_client", lambda uri: _Client()) + + +def make_id_token(claims=None, nonce=None): + now = datetime.now(tz=timezone.utc) + payload = { + "iss": ISSUER, + "sub": SUBJECT, + "aud": CLIENT_ID, + "iat": now, + "exp": now + timedelta(minutes=5), + } + if nonce: + payload["nonce"] = nonce + payload.update(claims or {}) + return jwt.encode(payload, PRIVATE_KEY, algorithm="RS256") + + +def fake_config(keys, config=None): + resolved = config or BASE_CONFIG + return [resolved.get(key["key"], key.get("default")) for key in keys] + + +def build_provider(document=None, config=None, nonce=None, code="auth-code", state=None, is_space=False): + with ( + patch.object(provider_module, "get_configuration_value", lambda keys: fake_config(keys, config)), + patch.object(provider_module, "get_discovery_document", lambda issuer: document or discovery()), + ): + return OIDCProvider(request=FakeRequest(), code=code, state=state, nonce=nonce, is_space=is_space) + + +def run_flow(id_token_claims=None, userinfo=None, document=None, config=None, nonce=None): + """Drive one full callback leg: token exchange, validation, claim mapping.""" + provider = build_provider(document, config, nonce) + + token_response = MagicMock() + token_response.json.return_value = { + "access_token": "access-token", + "refresh_token": "refresh-token", + "expires_in": 3600, + "id_token": make_id_token(id_token_claims, nonce=nonce), + } + token_response.raise_for_status = lambda: None + + userinfo_response = MagicMock() + userinfo_response.json.return_value = userinfo or {} + userinfo_response.raise_for_status = lambda: None + + with ( + patch.object(provider_module, "get_configuration_value", lambda keys: fake_config(keys, config)), + patch("plane.authentication.adapter.oauth.requests.post", return_value=token_response), + patch("plane.authentication.adapter.oauth.requests.get", return_value=userinfo_response), + ): + provider.set_token_data() + provider.set_user_data() + + return provider + + +@pytest.mark.unit +class TestClaimMapping: + def test_user_data_comes_from_the_id_token(self): + provider = run_flow( + { + "email": "ada@example.com", + "email_verified": True, + "given_name": "Ada", + "family_name": "Lovelace", + "picture": "https://example.com/ada.png", + } + ) + user = provider.user_data["user"] + assert provider.user_data["email"] == "ada@example.com" + # `sub`, not email: email can be reassigned, sub cannot. + assert user["provider_id"] == SUBJECT + assert user["first_name"] == "Ada" + assert user["last_name"] == "Lovelace" + assert user["avatar"] == "https://example.com/ada.png" + assert user["is_password_autoset"] is True + + def test_full_name_is_split_when_given_name_is_absent(self): + provider = run_flow( + {"email": "alan@example.com", "email_verified": True, "name": "Alan Turing"}, + userinfo={"sub": SUBJECT}, + ) + user = provider.user_data["user"] + assert user["first_name"] == "Alan" + assert user["last_name"] == "Turing" + + def test_access_token_expiry_is_relative_to_now(self): + provider = run_flow({"email": "ada@example.com", "email_verified": True, "name": "Ada"}) + assert provider.token_data["access_token_expired_at"] > datetime.now(tz=timezone.utc) + + +@pytest.mark.unit +class TestUnverifiedEmail: + def test_unverified_email_is_rejected(self): + with pytest.raises(AuthenticationException) as exc: + run_flow({"email": "ada@example.com", "email_verified": False, "name": "Ada"}) + assert exc.value.error_message == "OAUTH_PROVIDER_UNVERIFIED_EMAIL" + + def test_absent_email_verified_claim_fails_closed(self): + with pytest.raises(AuthenticationException) as exc: + run_flow({"email": "ada@example.com", "name": "Ada"}) + assert exc.value.error_message == "OAUTH_PROVIDER_UNVERIFIED_EMAIL" + + def test_opt_out_allows_unverified_email(self): + config = {**BASE_CONFIG, "OIDC_ALLOW_UNVERIFIED_EMAIL": "1"} + provider = run_flow({"email": "ada@example.com", "name": "Ada"}, config=config) + assert provider.user_data["email"] == "ada@example.com" + + +@pytest.mark.unit +class TestUserinfoFallback: + def test_userinfo_fills_claims_the_id_token_omitted(self): + provider = run_flow( + {"email": "grace@example.com", "email_verified": True}, + userinfo={"sub": SUBJECT, "given_name": "Grace", "family_name": "Hopper"}, + ) + assert provider.user_data["user"]["first_name"] == "Grace" + + def test_userinfo_supplies_a_missing_email_verified(self): + """An otherwise complete ID token that omits email_verified must still be + topped up from userinfo rather than rejected. Providers differ on which claims + reach the ID token, so rejecting here would fail a login that the userinfo + call can satisfy.""" + provider = run_flow( + {"email": "ada@example.com", "given_name": "Ada", "family_name": "Lovelace"}, + userinfo={"sub": SUBJECT, "email_verified": True}, + ) + assert provider.user_data["email"] == "ada@example.com" + + def test_userinfo_that_denies_verification_still_rejects(self): + """Topping up from userinfo must not become a way around the check.""" + with pytest.raises(AuthenticationException) as exc: + run_flow( + {"email": "ada@example.com", "given_name": "Ada", "family_name": "Lovelace"}, + userinfo={"sub": SUBJECT, "email_verified": False}, + ) + assert exc.value.error_message == "OAUTH_PROVIDER_UNVERIFIED_EMAIL" + + def test_userinfo_is_not_called_when_the_id_token_suffices(self): + """A complete ID token must not cost an extra network round trip.""" + provider = build_provider() + token_response = MagicMock() + token_response.json.return_value = { + "access_token": "access-token", + "expires_in": 3600, + "id_token": make_id_token( + {"email": "ada@example.com", "email_verified": True, "given_name": "Ada", "family_name": "Lovelace"} + ), + } + token_response.raise_for_status = lambda: None + + with ( + patch.object(provider_module, "get_configuration_value", lambda keys: fake_config(keys)), + patch("plane.authentication.adapter.oauth.requests.post", return_value=token_response), + patch("plane.authentication.adapter.oauth.requests.get") as userinfo_get, + ): + provider.set_token_data() + provider.set_user_data() + + userinfo_get.assert_not_called() + + def test_userinfo_for_another_subject_is_discarded(self): + """A userinfo response that does not match the ID token must not be trusted.""" + provider = run_flow( + {"email": "ada@example.com", "email_verified": True}, + userinfo={ + "sub": "a-different-subject", + "given_name": "Mallory", + "email": "mallory@evil.example.com", + }, + ) + assert provider.user_data["email"] == "ada@example.com" + assert provider.user_data["user"]["first_name"] != "Mallory" + assert provider.user_data["user"]["provider_id"] == SUBJECT + + +@pytest.mark.unit +class TestClientAuthentication: + def test_basic_auth_is_used_when_that_is_all_the_provider_accepts(self): + provider = build_provider(discovery(token_endpoint_auth_methods_supported=["client_secret_basic"])) + fields, headers = provider._token_request_auth() + assert fields == {} + credentials = base64.b64decode(headers["Authorization"].split()[1]).decode() + assert credentials == f"{CLIENT_ID}:{CLIENT_SECRET}" + + def test_secret_goes_in_the_form_body_when_the_provider_offers_it(self): + provider = build_provider(discovery(token_endpoint_auth_methods_supported=["client_secret_post"])) + fields, headers = provider._token_request_auth() + assert fields["client_secret"] == CLIENT_SECRET + assert "Authorization" not in headers + + def test_post_is_preferred_when_the_provider_offers_both(self): + provider = build_provider( + discovery(token_endpoint_auth_methods_supported=["client_secret_basic", "client_secret_post"]) + ) + fields, headers = provider._token_request_auth() + assert fields["client_secret"] == CLIENT_SECRET + assert "Authorization" not in headers + + @pytest.mark.parametrize("advertised", [None, []], ids=["field-absent", "field-empty"]) + def test_an_unpublished_list_falls_back_to_basic_per_the_spec(self, advertised): + """OIDC Discovery 1.0 §3: when token_endpoint_auth_methods_supported is + omitted, "the default is client_secret_basic". A compliant provider that + accepts only the default and publishes no list would reject a form-body + secret, so an absent list is not a free choice.""" + document = discovery() + if advertised is None: + document.pop("token_endpoint_auth_methods_supported", None) + else: + document["token_endpoint_auth_methods_supported"] = advertised + + provider = build_provider(document) + fields, headers = provider._token_request_auth() + assert fields == {} + credentials = base64.b64decode(headers["Authorization"].split()[1]).decode() + assert credentials == f"{CLIENT_ID}:{CLIENT_SECRET}" + + +@pytest.mark.unit +class TestAuthorizationRequest: + def test_authorization_url_carries_state_nonce_and_scope(self): + provider = build_provider(state="state-1", nonce="nonce-1", code=None) + url = provider.get_auth_url() + assert "state=state-1" in url + assert "nonce=nonce-1" in url + assert "scope=openid+email+profile" in url + assert "redirect_uri=https%3A%2F%2Fplane.example.com%2Fauth%2Foidc%2Fcallback%2F" in url + + def test_app_flow_calls_back_to_the_app_endpoint(self): + provider = build_provider(state="state-1", nonce="nonce-1", code=None) + assert provider.redirect_uri == "https://plane.example.com/auth/oidc/callback/" + + def test_spaces_flow_calls_back_to_the_spaces_endpoint(self): + """The two flows keep separate session keys, so the IdP must return to the + endpoint that started the login or the state/nonce lookup finds nothing.""" + provider = build_provider(state="state-1", nonce="nonce-1", code=None, is_space=True) + assert provider.redirect_uri == "https://plane.example.com/auth/spaces/oidc/callback/" + assert "redirect_uri=https%3A%2F%2Fplane.example.com%2Fauth%2Fspaces%2Foidc%2Fcallback%2F" in ( + provider.get_auth_url() + ) + + def test_missing_configuration_is_reported(self): + config = {**BASE_CONFIG, "OIDC_CLIENT_SECRET": None} + with pytest.raises(AuthenticationException) as exc: + build_provider(config=config) + assert exc.value.error_message == "OIDC_NOT_CONFIGURED" + + +@pytest.mark.unit +@pytest.mark.django_db +class TestSignupGating: + """OIDC honours ENABLE_SIGNUP. + + There is no separate sign-up flow for OIDC — the same button serves login and + registration — so hiding a sign-up button in the UI proves nothing here. The only + thing standing between a disabled instance and a brand-new IdP identity is + __check_signup() in the shared adapter, reached through complete_login_or_signup(). + """ + + def _authenticate(self, email, enable_signup): + """Drive the post-token half of the flow with a verified ID token for `email`.""" + provider = build_provider() + provider.id_token_claims = { + "sub": f"sub-for-{email}", + "email": email, + "email_verified": True, + "given_name": "Ada", + "family_name": "Lovelace", + } + provider.token_data = None # skip Account creation; irrelevant to the gate + + with ( + patch.object(provider_module, "get_configuration_value", lambda keys: fake_config(keys)), + patch( + "plane.authentication.adapter.base.get_configuration_value", + lambda keys: (enable_signup,), + ), + ): + provider.set_user_data() + return provider.complete_login_or_signup() + + def test_new_identity_is_refused_when_signup_is_disabled(self): + from plane.db.models import User + + with pytest.raises(AuthenticationException) as exc: + self._authenticate("stranger@example.com", enable_signup="0") + + assert exc.value.error_message == "SIGNUP_DISABLED" + assert not User.objects.filter(email="stranger@example.com").exists() + + def test_new_identity_is_admitted_when_signup_is_enabled(self): + user = self._authenticate("newcomer@example.com", enable_signup="1") + assert user.email == "newcomer@example.com" + + def test_an_invited_address_is_admitted_even_while_signup_is_disabled(self): + """The gate checks for a pending workspace invite before refusing.""" + from plane.db.models import Workspace, WorkspaceMemberInvite + + owner = self._authenticate("owner@example.com", enable_signup="1") + workspace = Workspace.objects.create(name="Acme", slug="acme", owner=owner) + WorkspaceMemberInvite.objects.create(email="invited@example.com", workspace=workspace, role=15, token="tok") + + user = self._authenticate("invited@example.com", enable_signup="0") + assert user.email == "invited@example.com" + + def test_an_existing_user_can_still_sign_in_while_signup_is_disabled(self): + """Gating registration must not lock out people who already have accounts.""" + existing = self._authenticate("regular@example.com", enable_signup="1") + + again = self._authenticate("regular@example.com", enable_signup="0") + assert again.id == existing.id diff --git a/apps/api/plane/tests/unit/authentication/test_oidc_views.py b/apps/api/plane/tests/unit/authentication/test_oidc_views.py new file mode 100644 index 00000000000..536af2e0941 --- /dev/null +++ b/apps/api/plane/tests/unit/authentication/test_oidc_views.py @@ -0,0 +1,134 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +"""Guards on the OIDC callback endpoints. + +Both callbacks reject the request before any provider or database work happens, so +these run on a bare RequestFactory. The nonce check matters as much as the state +check: state proves the redirect belongs to this browser, nonce proves the ID token +does. Losing either one silently would leave the token unbound to the session. +""" + +from unittest.mock import MagicMock, patch + +import pytest +from django.test import RequestFactory + +from plane.authentication.views.app.oidc import ( + NONCE_SESSION_KEY, + STATE_SESSION_KEY, + OIDCCallbackEndpoint, +) +from plane.authentication.views.space.oidc import ( + NONCE_SESSION_KEY as SPACE_NONCE_SESSION_KEY, +) +from plane.authentication.views.space.oidc import ( + STATE_SESSION_KEY as SPACE_STATE_SESSION_KEY, +) +from plane.authentication.views.space.oidc import ( + OIDCCallbackSpaceEndpoint, +) +from plane.authentication.views.space import oidc as space_oidc + +# 5114 = OIDC_OAUTH_PROVIDER_ERROR, the code both callbacks refuse with. +REFUSAL_CODE = "5114" + + +@pytest.fixture(autouse=True) +def base_urls(settings): + """base_host() builds the refusal redirect from these; unset they yield None.""" + settings.WEB_URL = "https://plane.example.com" + settings.APP_BASE_URL = "https://plane.example.com" + settings.SPACE_BASE_URL = "https://plane.example.com" + + +def make_request(session, query): + request = RequestFactory().get("/auth/oidc/callback/", query) + request.session = dict(session) + return request + + +def refused(response): + """A refusal is a redirect back to the sign-in page carrying the error code.""" + return response.status_code == 302 and f"error_code={REFUSAL_CODE}" in response.url + + +@pytest.mark.unit +@pytest.mark.parametrize( + ("view", "state_key", "nonce_key"), + [ + (OIDCCallbackEndpoint, STATE_SESSION_KEY, NONCE_SESSION_KEY), + (OIDCCallbackSpaceEndpoint, SPACE_STATE_SESSION_KEY, SPACE_NONCE_SESSION_KEY), + ], + ids=["app", "spaces"], +) +class TestCallbackGuards: + def test_missing_nonce_is_refused(self, view, state_key, nonce_key): + """State alone is not enough: without the nonce the ID token would be + validated with no session binding, so the login is refused instead.""" + request = make_request({state_key: "s1"}, {"code": "c", "state": "s1"}) + assert refused(view().get(request)) + + def test_mismatched_state_is_refused(self, view, state_key, nonce_key): + request = make_request({state_key: "s1", nonce_key: "n1"}, {"code": "c", "state": "other"}) + assert refused(view().get(request)) + + def test_absent_state_in_session_is_refused(self, view, state_key, nonce_key): + request = make_request({}, {"code": "c", "state": "s1"}) + assert refused(view().get(request)) + + def test_missing_code_is_refused(self, view, state_key, nonce_key): + request = make_request({state_key: "s1", nonce_key: "n1"}, {"state": "s1"}) + assert refused(view().get(request)) + + def test_state_and_nonce_are_single_use(self, view, state_key, nonce_key): + """Both are popped, so a replayed callback finds nothing left to match. + + The state check passes here and the request is refused for the missing code + instead, which keeps the test out of the provider while still proving the pop + happens on a request that got past state validation. + """ + session = {state_key: "s1", nonce_key: "n1"} + request = RequestFactory().get("/auth/oidc/callback/", {"state": "s1"}) + request.session = session + + assert refused(view().get(request)) + assert state_key not in session + assert nonce_key not in session + + # Replaying the same callback now fails the state check outright. + replay = make_request(session, {"code": "c", "state": "s1"}) + assert refused(view().get(replay)) + + +@pytest.mark.unit +class TestSpacesLandingUrl: + """Where a successful Spaces login comes to rest. + + SPACE_BASE_PATH contributes a trailing slash that the callback used to strip + unconditionally, sending the browser to /spaces. The Spaces app declares a base URL of + /spaces/ and 404s on the bare form, so the slash has to survive when there is no + next_path to append. + """ + + def _run(self, session_next_path=None): + session = {SPACE_STATE_SESSION_KEY: "s1", SPACE_NONCE_SESSION_KEY: "n1"} + if session_next_path: + session["next_path"] = session_next_path + request = RequestFactory().get("/auth/spaces/oidc/callback/", {"code": "c", "state": "s1"}) + request.session = session + + provider = MagicMock() + provider.authenticate.return_value = MagicMock() + with ( + patch.object(space_oidc, "OIDCProvider", return_value=provider), + patch.object(space_oidc, "user_login"), + ): + return OIDCCallbackSpaceEndpoint().get(request) + + def test_lands_on_the_spaces_base_path_with_its_trailing_slash(self): + assert self._run().url == "https://plane.example.com/spaces/" + + def test_next_path_is_appended_without_doubling_the_slash(self): + assert self._run(session_next_path="/foo").url == "https://plane.example.com/spaces/foo" diff --git a/apps/api/plane/utils/instance_config_variables/core.py b/apps/api/plane/utils/instance_config_variables/core.py index 6df27d26170..da1e9d0b89b 100644 --- a/apps/api/plane/utils/instance_config_variables/core.py +++ b/apps/api/plane/utils/instance_config_variables/core.py @@ -250,6 +250,56 @@ }, ] +oidc_config_variables = [ + { + "key": "IS_OIDC_ENABLED", + "value": os.environ.get("IS_OIDC_ENABLED", "0"), + "category": "OIDC", + "is_encrypted": False, + }, + { + "key": "OIDC_ISSUER_URL", + "value": os.environ.get("OIDC_ISSUER_URL"), + "category": "OIDC", + "is_encrypted": False, + }, + { + "key": "OIDC_CLIENT_ID", + "value": os.environ.get("OIDC_CLIENT_ID"), + "category": "OIDC", + "is_encrypted": False, + }, + { + "key": "OIDC_CLIENT_SECRET", + "value": os.environ.get("OIDC_CLIENT_SECRET"), + "category": "OIDC", + "is_encrypted": True, + }, + # Label shown on the sign-in button. The provider is generic, so the instance + # admin names it after their own IdP ("Okta", "Entra ID", "Keycloak"). + { + "key": "OIDC_PROVIDER_NAME", + "value": os.environ.get("OIDC_PROVIDER_NAME", "SSO"), + "category": "OIDC", + "is_encrypted": False, + }, + # Escape hatch for IdPs that do not emit email_verified (Entra ID commonly + # omits it). Setting this to "1" trusts the provider to only assert addresses + # it has verified — see the comment in provider/oauth/oidc.py. + { + "key": "OIDC_ALLOW_UNVERIFIED_EMAIL", + "value": os.environ.get("OIDC_ALLOW_UNVERIFIED_EMAIL", "0"), + "category": "OIDC", + "is_encrypted": False, + }, + { + "key": "ENABLE_OIDC_SYNC", + "value": os.environ.get("ENABLE_OIDC_SYNC", "0"), + "category": "OIDC", + "is_encrypted": False, + }, +] + core_config_variables = [ *authentication_config_variables, *workspace_management_config_variables, @@ -257,6 +307,7 @@ *github_config_variables, *gitlab_config_variables, *gitea_config_variables, + *oidc_config_variables, *smtp_config_variables, *llm_config_variables, *unsplash_config_variables, From 0fcec91e88d6e4b058f3556537fa0f7b643d8a19 Mon Sep 17 00:00:00 2001 From: Juan Date: Mon, 7 Sep 2026 15:12:24 -0500 Subject: [PATCH 2/2] feat: surface OIDC authentication in admin, web and spaces Wires the generic OIDC provider into the interfaces: a God Mode page to configure it, and a sign-in button in the web and spaces apps. Because the provider is generic rather than a named vendor, the admin sets OIDC_PROVIDER_NAME and that label is what the sign-in button shows ("Continue with Okta"), falling back to "SSO". The instance endpoint exposes it alongside is_oidc_enabled so the apps can render the button without extra requests. The unverified-email opt-out gets its own control rather than reusing the shared sync switch, so the UI can state plainly what enabling it means: Plane matches accounts by email, so accepting an address the provider has not verified is a takeover vector. It stays off unless an admin deliberately turns it on. Adds the OIDC error codes to the frontend maps, including the previously missing OAUTH_PROVIDER_UNVERIFIED_EMAIL, so a rejected sign-in explains itself instead of falling through to a generic failure. Co-Authored-By: Claude Opus 5 (1M context) --- .../(dashboard)/authentication/oidc/form.tsx | 248 ++++++++++++++++++ .../(dashboard)/authentication/oidc/page.tsx | 120 +++++++++ apps/admin/app/routes.ts | 1 + .../components/authentication/oidc-config.tsx | 69 +++++ apps/admin/components/common/header/core.ts | 1 + apps/admin/hooks/oauth/core.tsx | 11 + apps/admin/hooks/oauth/index.ts | 1 + apps/space/helpers/authentication.helper.tsx | 28 ++ apps/space/hooks/oauth/core.tsx | 25 +- apps/web/core/hooks/oauth/core.tsx | 21 +- apps/web/helpers/authentication.helper.tsx | 28 ++ packages/constants/src/auth/core.ts | 1 + packages/constants/src/auth/index.ts | 5 + packages/types/src/instance/auth.ts | 19 +- packages/types/src/instance/base.ts | 3 + packages/utils/src/auth.ts | 22 ++ 16 files changed, 597 insertions(+), 6 deletions(-) create mode 100644 apps/admin/app/(all)/(dashboard)/authentication/oidc/form.tsx create mode 100644 apps/admin/app/(all)/(dashboard)/authentication/oidc/page.tsx create mode 100644 apps/admin/components/authentication/oidc-config.tsx diff --git a/apps/admin/app/(all)/(dashboard)/authentication/oidc/form.tsx b/apps/admin/app/(all)/(dashboard)/authentication/oidc/form.tsx new file mode 100644 index 00000000000..3dbe81fa317 --- /dev/null +++ b/apps/admin/app/(all)/(dashboard)/authentication/oidc/form.tsx @@ -0,0 +1,248 @@ +/** + * Copyright (c) 2023-present Plane Software, Inc. and contributors + * SPDX-License-Identifier: AGPL-3.0-only + * See the LICENSE file for details. + */ + +import { useState } from "react"; +import { isEmpty } from "lodash-es"; +import Link from "next/link"; +import { Controller, useForm } from "react-hook-form"; +// plane internal packages +import { API_BASE_URL } from "@plane/constants"; +import { Button } from "@makeplane/propel/components/button"; +import { Switch } from "@makeplane/propel/components/switch"; +import { TOAST_TYPE, setToast } from "@/providers/toast"; +import type { IFormattedInstanceConfiguration, TInstanceOIDCAuthenticationConfigurationKeys } from "@plane/types"; +// components +import { CodeBlock } from "@/components/common/code-block"; +import { ConfirmDiscardModal } from "@/components/common/confirm-discard-modal"; +import type { TControllerInputFormField } from "@/components/common/controller-input"; +import type { TControllerSwitchFormField } from "@/components/common/controller-switch"; +import { ControllerSwitch } from "@/components/common/controller-switch"; +import { ControllerInput } from "@/components/common/controller-input"; +import type { TCopyField } from "@/components/common/copy-field"; +import { CopyField } from "@/components/common/copy-field"; +// hooks +import { useInstance } from "@/hooks/store"; + +type Props = { + config: IFormattedInstanceConfiguration; +}; + +type OIDCConfigFormValues = Record; + +const OIDC_FORM_SWITCH_FIELD: TControllerSwitchFormField = { + name: "ENABLE_OIDC_SYNC", + label: "your identity provider", +}; + +export function InstanceOIDCConfigForm(props: Props) { + const { config } = props; + // states + const [isDiscardChangesModalOpen, setIsDiscardChangesModalOpen] = useState(false); + // store hooks + const { updateInstanceConfigurations } = useInstance(); + // form data + const { + handleSubmit, + control, + reset, + formState: { errors, isDirty, isSubmitting }, + } = useForm({ + defaultValues: { + OIDC_PROVIDER_NAME: config["OIDC_PROVIDER_NAME"] || "SSO", + OIDC_ISSUER_URL: config["OIDC_ISSUER_URL"], + OIDC_CLIENT_ID: config["OIDC_CLIENT_ID"], + OIDC_CLIENT_SECRET: config["OIDC_CLIENT_SECRET"], + ENABLE_OIDC_SYNC: config["ENABLE_OIDC_SYNC"] || "0", + OIDC_ALLOW_UNVERIFIED_EMAIL: config["OIDC_ALLOW_UNVERIFIED_EMAIL"] || "0", + }, + }); + + const originURL = !isEmpty(API_BASE_URL) ? API_BASE_URL : typeof window !== "undefined" ? window.location.origin : ""; + + const OIDC_FORM_FIELDS: TControllerInputFormField[] = [ + { + key: "OIDC_PROVIDER_NAME", + type: "text", + label: "Provider name", + description: <>The name shown on the sign-in button, for example Okta, Entra ID, or Keycloak., + placeholder: "SSO", + error: Boolean(errors.OIDC_PROVIDER_NAME), + required: false, + }, + { + key: "OIDC_ISSUER_URL", + type: "text", + label: "Issuer URL", + description: ( + <> + Your provider's issuer identifier. Plane reads every endpoint it needs from{" "} + {"/.well-known/openid-configuration"}, so this must be an{" "} + https URL and must match the iss claim your provider issues. + + ), + placeholder: "https://your-org.okta.com", + error: Boolean(errors.OIDC_ISSUER_URL), + required: true, + }, + { + key: "OIDC_CLIENT_ID", + type: "text", + label: "Client ID", + description: <>The client ID of the application you registered with your identity provider., + placeholder: "0oa1b2c3d4e5f6g7h8i9", + error: Boolean(errors.OIDC_CLIENT_ID), + required: true, + }, + { + key: "OIDC_CLIENT_SECRET", + type: "password", + label: "Client secret", + description: <>The client secret issued alongside the client ID., + placeholder: "*****************************", + error: Boolean(errors.OIDC_CLIENT_SECRET), + required: true, + }, + ]; + + const OIDC_SERVICE_FIELD: TCopyField[] = [ + { + key: "Callback_URL", + label: "Callback URL", + url: `${originURL}/auth/oidc/callback/`, + description: ( + <> + We will auto-generate this. Paste it into the Redirect URI (also called + sign-in redirect URI) field of the application you registered with your identity provider. + + ), + }, + { + key: "Spaces_Callback_URL", + label: "Callback URL for Spaces", + url: `${originURL}/auth/spaces/oidc/callback/`, + description: ( + <> + Add this as a second Redirect URI on the same application. Signing in to + published Spaces comes back here instead, and providers reject any redirect URI they have not been given. + + ), + }, + ]; + + const onSubmit = async (formData: OIDCConfigFormValues) => { + const payload: Partial = { ...formData }; + + try { + const response = await updateInstanceConfigurations(payload); + setToast({ + type: TOAST_TYPE.SUCCESS, + title: "Done!", + message: "Your OIDC authentication is configured. You should test it now.", + }); + reset({ + OIDC_PROVIDER_NAME: response.find((item) => item.key === "OIDC_PROVIDER_NAME")?.value, + OIDC_ISSUER_URL: response.find((item) => item.key === "OIDC_ISSUER_URL")?.value, + OIDC_CLIENT_ID: response.find((item) => item.key === "OIDC_CLIENT_ID")?.value, + OIDC_CLIENT_SECRET: response.find((item) => item.key === "OIDC_CLIENT_SECRET")?.value, + ENABLE_OIDC_SYNC: response.find((item) => item.key === "ENABLE_OIDC_SYNC")?.value, + OIDC_ALLOW_UNVERIFIED_EMAIL: response.find((item) => item.key === "OIDC_ALLOW_UNVERIFIED_EMAIL")?.value, + }); + } catch (err) { + console.error(err); + } + }; + + const handleGoBack = (e: React.MouseEvent) => { + if (isDirty) { + e.preventDefault(); + setIsDiscardChangesModalOpen(true); + } + }; + + return ( + <> + setIsDiscardChangesModalOpen(false)} + /> +
+
+
+
Provider-provided details for Plane
+ {OIDC_FORM_FIELDS.map((field) => ( + + ))} + + + {/* Kept separate from the sync switch: this one relaxes a security check, + so it needs to say plainly what turning it on means. */} +
+
+

Accept accounts your provider has not verified

+

+ Plane matches accounts by email address. Leave this off unless your provider never sends an{" "} + email_verified claim (Entra ID commonly omits it). Turning it on means trusting + your provider to only ever assert addresses it controls. +

+
+
+ { + const isOn = value === "1"; + return onChange(isOn ? "0" : "1")} size="sm" />; + }} + /> +
+
+ +
+
+
+
+
+
+
+
Plane-provided details for your provider
+ {OIDC_SERVICE_FIELD.map((field) => ( + + ))} +
+
+
+
+ + ); +} diff --git a/apps/admin/app/(all)/(dashboard)/authentication/oidc/page.tsx b/apps/admin/app/(all)/(dashboard)/authentication/oidc/page.tsx new file mode 100644 index 00000000000..9912d8cb881 --- /dev/null +++ b/apps/admin/app/(all)/(dashboard)/authentication/oidc/page.tsx @@ -0,0 +1,120 @@ +/** + * Copyright (c) 2023-present Plane Software, Inc. and contributors + * SPDX-License-Identifier: AGPL-3.0-only + * See the LICENSE file for details. + */ + +import { useState } from "react"; +import { observer } from "mobx-react"; +import useSWR from "swr"; +// icons +import { ShieldCheck } from "lucide-react"; +// plane internal packages +import { Switch } from "@makeplane/propel/components/switch"; +// components +import { AuthenticationMethodCard } from "@/components/authentication/authentication-method-card"; +import { PageWrapper } from "@/components/common/page-wrapper"; +import { Skeleton } from "@/components/common/skeleton"; +import { setPromiseToast } from "@/providers/toast"; +// hooks +import { useInstance } from "@/hooks/store"; +// types +import type { Route } from "./+types/page"; +// local +import { InstanceOIDCConfigForm } from "./form"; + +const InstanceOIDCAuthenticationPage = observer(function InstanceOIDCAuthenticationPage(_props: Route.ComponentProps) { + // store + const { fetchInstanceConfigurations, formattedConfig, updateInstanceConfigurations } = useInstance(); + // state + const [isSubmitting, setIsSubmitting] = useState(false); + // config + const enableOIDCConfig = formattedConfig?.IS_OIDC_ENABLED ?? ""; + const isEnabled = Boolean(parseInt(enableOIDCConfig)); + // Every endpoint is discovered from the issuer, so without it plus the client + // credentials the method cannot work at all. + const isOIDCConfigured = + !!formattedConfig?.OIDC_ISSUER_URL && !!formattedConfig?.OIDC_CLIENT_ID && !!formattedConfig?.OIDC_CLIENT_SECRET; + // Block turning it *on* before it is configured, which would leave the instance + // advertising a sign-in button that can only fail. Turning it off stays available + // whatever the config says — otherwise clearing a field would strand the method + // enabled with no way back. + const cannotEnableYet = !isOIDCConfigured && !isEnabled; + + useSWR("INSTANCE_CONFIGURATIONS", () => fetchInstanceConfigurations()); + + const updateConfig = async (key: "IS_OIDC_ENABLED", value: string) => { + setIsSubmitting(true); + + const payload = { + [key]: value, + }; + + const updateConfigPromise = updateInstanceConfigurations(payload); + + setPromiseToast(updateConfigPromise, { + loading: "Saving Configuration", + success: { + title: "Configuration saved", + message: () => `OIDC authentication is now ${value === "1" ? "active" : "disabled"}.`, + }, + error: { + title: "Error", + message: () => "Failed to save configuration", + }, + }); + + // try/finally rather than the .then/.catch pair the sibling provider pages use: + // that shape trips oxlint's promise(always-return), which lint-staged runs with + // --deny-warnings, and this reads better anyway — one place resets the flag. + try { + await updateConfigPromise; + } catch (err) { + console.error(err); + } finally { + setIsSubmitting(false); + } + }; + + return ( + } + config={ + + { + updateConfig("IS_OIDC_ENABLED", isEnabled ? "0" : "1"); + }} + size="sm" + disabled={isSubmitting || !formattedConfig || cannotEnableYet} + /> + + } + disabled={isSubmitting || !formattedConfig} + withBorder={false} + /> + } + > + {formattedConfig ? ( + + ) : ( + + + + + + + + )} + + ); +}); + +export const meta: Route.MetaFunction = () => [{ title: "OIDC Authentication - God Mode" }]; + +export default InstanceOIDCAuthenticationPage; diff --git a/apps/admin/app/routes.ts b/apps/admin/app/routes.ts index 184bed205a7..48bb06f5c5f 100644 --- a/apps/admin/app/routes.ts +++ b/apps/admin/app/routes.ts @@ -19,6 +19,7 @@ export default [ route("authentication/gitlab", "./(all)/(dashboard)/authentication/gitlab/page.tsx"), route("authentication/google", "./(all)/(dashboard)/authentication/google/page.tsx"), route("authentication/gitea", "./(all)/(dashboard)/authentication/gitea/page.tsx"), + route("authentication/oidc", "./(all)/(dashboard)/authentication/oidc/page.tsx"), route("ai", "./(all)/(dashboard)/ai/page.tsx"), route("image", "./(all)/(dashboard)/image/page.tsx"), ]), diff --git a/apps/admin/components/authentication/oidc-config.tsx b/apps/admin/components/authentication/oidc-config.tsx new file mode 100644 index 00000000000..d499f806216 --- /dev/null +++ b/apps/admin/components/authentication/oidc-config.tsx @@ -0,0 +1,69 @@ +/** + * Copyright (c) 2023-present Plane Software, Inc. and contributors + * SPDX-License-Identifier: AGPL-3.0-only + * See the LICENSE file for details. + */ + +import { observer } from "mobx-react"; +import Link from "next/link"; +// icons +import { SettingsOutline } from "@makeplane/propel/icons"; +// plane internal packages +import { AnchorButton } from "@makeplane/propel/components/anchor-button"; +import { Button } from "@makeplane/propel/components/button"; +import { Switch } from "@makeplane/propel/components/switch"; +import type { TInstanceAuthenticationMethodKeys } from "@plane/types"; +// hooks +import { useInstance } from "@/hooks/store"; + +type Props = { + disabled: boolean; + updateConfig: (key: TInstanceAuthenticationMethodKeys, value: string) => void; +}; + +export const OIDCConfiguration = observer(function OIDCConfiguration(props: Props) { + const { disabled, updateConfig } = props; + // store + const { formattedConfig } = useInstance(); + // derived values + const enableOIDCConfig = formattedConfig?.IS_OIDC_ENABLED ?? ""; + // The issuer is what every endpoint is discovered from, so it is required + // alongside the client credentials before the method can be switched on. + const isOIDCConfigured = + !!formattedConfig?.OIDC_ISSUER_URL && !!formattedConfig?.OIDC_CLIENT_ID && !!formattedConfig?.OIDC_CLIENT_SECRET; + + return ( + <> + {isOIDCConfigured ? ( +
+ } + label="Edit" + /> + { + const newEnableOIDCConfig = Boolean(parseInt(enableOIDCConfig)) === true ? "0" : "1"; + updateConfig("IS_OIDC_ENABLED", newEnableOIDCConfig); + }} + size="sm" + disabled={disabled} + /> +
+ ) : ( +