feat(oidc): Azure Entra ID support — configurable email claim & verification + Remember Me persistent login (#1126)

feat(oidc): add Azure Entra ID support with configurable email claim resolution

Adds two new OIDC provider fields: email_claim and require_email_verified.
This commit is contained in:
Sn0rrii
2026-04-25 13:32:42 +02:00
committed by GitHub
parent 4304a42542
commit fdaec47378
22 changed files with 2288 additions and 73 deletions
+136 -18
View File
@@ -58,6 +58,7 @@ from backend.app.models.user import User
from backend.app.models.user_otp_code import UserOTPCode
from backend.app.models.user_totp import UserTOTP
from backend.app.schemas.auth import (
AUTO_LINK_REQUIREMENTS_ERROR,
AdminDisable2FARequest,
BackupCodesResponse,
EmailOTPDisableRequest,
@@ -373,6 +374,126 @@ def _assert_totp_not_replayed(totp_obj: pyotp.TOTP, totp_record: UserTOTP, code:
totp_record.accept_counter(accepted_counter)
# ---------------------------------------------------------------------------
# OIDC helpers
# ---------------------------------------------------------------------------
_EMAIL_SHAPE_RE = re.compile(r"[^\s@]+@[^\s@]+\.[^\s@]+")
def _is_valid_email_shaped(value: str | None) -> bool:
# SEC-2: shape check for non-standard claims (upn, preferred_username).
# Requires local@domain.tld — rejects "@", "x@", "@domain", "x@nodot".
if not value or len(value) > 255:
return False
return _EMAIL_SHAPE_RE.fullmatch(value) is not None
def _enforce_auto_link_safety(provider: OIDCProvider) -> None:
"""Raise HTTP 422 if auto_link_existing_accounts is on without safe email settings.
SEC-1/SEC-6: auto_link is only safe when require_email_verified=True AND
email_claim='email'. Called after ORM construction (create) and after the
setattr loop (update) so partial-update bypasses are also caught.
"""
if provider.auto_link_existing_accounts and (
not provider.require_email_verified or provider.email_claim != "email"
):
raise HTTPException(
status_code=status.HTTP_422_UNPROCESSABLE_ENTITY,
detail=AUTO_LINK_REQUIREMENTS_ERROR,
)
def _resolve_provider_email(provider: OIDCProvider, claims: dict, provider_sub: str) -> str | None:
"""Extract and normalise the email address from OIDC ID-token claims.
Implements three resolution paths (Fall A/B/C):
Fall C — custom email_claim (!= "email"): shape-check only, no email_verified gate.
Recommended for Azure Entra ID (preferred_username or upn).
Fall A — email_claim="email" + require_email_verified=True: strict, email_verified must be True.
Fall B — email_claim="email" + require_email_verified=False: permissive, explicit False drops email.
Returns a lowercase-stripped email string, or None when the claim is absent/invalid.
"""
provider_id = provider.id
raw_claim_value = claims.get(provider.email_claim)
if raw_claim_value is not None and not isinstance(raw_claim_value, str):
# TYPE-GUARD: non-string claim (e.g. list, int) would raise AttributeError on .lower().
logger.warning(
"OIDC provider %d: email_claim %r has unexpected type %s for sub=%r, ignoring",
provider_id,
provider.email_claim,
type(raw_claim_value).__name__,
provider_sub,
)
raw_claim_value = None
raw_email: str | None = raw_claim_value.lower().strip() if raw_claim_value else None
if provider.email_claim != "email":
# Fall C: custom claim (preferred_username, upn, …) — no email_verified check.
# SEC-2: _is_valid_email_shaped instead of bare '"@" in value'.
# Recommended for Azure Entra ID: set email_claim="preferred_username" or "upn".
if raw_email and _is_valid_email_shaped(raw_email):
return raw_email
if raw_email:
logger.warning(
"OIDC provider %d: email_claim %r value failed shape check for sub=%r, ignoring",
provider_id,
provider.email_claim,
provider_sub,
)
return None
email_verified = claims.get("email_verified")
if provider.require_email_verified:
# Fall A: standard C1-Guard — fail closed unless email_verified is True.
# SEC-2: apply shape check to standard email claim — providers may set
# email_verified=True on non-email values (e.g. numeric user IDs).
# SEC-3 normalisation applies; existing mixed-case provider_email records
# were normalised to lowercase by run_migrations at startup.
if raw_email and not _is_valid_email_shaped(raw_email):
logger.warning(
"OIDC provider %d: email claim failed shape check for sub=%r, ignoring",
provider_id,
provider_sub,
)
return None
if email_verified is True:
return raw_email
if raw_email:
logger.info(
"OIDC provider %d: ignoring email for sub=%r because email_verified=%r",
provider_id,
provider_sub,
email_verified,
)
return None
# Fall B: permissive — explicit False drops email, absent/None keeps it.
# Required for Azure Entra ID which never sends email_verified.
# SEC-2: apply shape check before the email_verified=False drop so malformed
# values are rejected regardless of the email_verified claim.
if raw_email and not _is_valid_email_shaped(raw_email):
logger.warning(
"OIDC provider %d: email claim failed shape check for sub=%r, ignoring",
provider_id,
provider_sub,
)
return None
if email_verified is False:
return None
if email_verified is not True:
# SEC-5: log only when the permissive path actually fires (ev absent/None),
# not on every successful login.
logger.info(
"OIDC provider %r (%d): accepting email for sub=%r without email_verified claim (permissive mode)",
provider.name,
provider.id,
provider_sub,
)
return raw_email
# ---------------------------------------------------------------------------
# Settings helpers (email 2FA flag)
# ---------------------------------------------------------------------------
@@ -1056,8 +1177,14 @@ async def create_oidc_provider(
scopes=body.scopes,
is_enabled=body.is_enabled,
auto_create_users=body.auto_create_users,
auto_link_existing_accounts=body.auto_link_existing_accounts,
email_claim=body.email_claim,
require_email_verified=body.require_email_verified,
icon_url=body.icon_url,
)
# SEC-1 + SEC-6: runtime guard mirrors the OIDCProviderCreate model_validator in schemas/auth.py.
# Catches any future path that bypasses Pydantic validation (direct ORM, scripts).
_enforce_auto_link_safety(provider)
db.add(provider)
await db.commit()
await db.refresh(provider)
@@ -1082,6 +1209,11 @@ async def update_oidc_provider(
value = value.rstrip("/")
setattr(provider, field, value)
# SEC-1 + SEC-6: Combined-State-Guard after setattr loop.
# Checks the final in-memory state (DB values + newly set values combined) to catch
# partial updates that each pass schema validation individually but are unsafe together.
_enforce_auto_link_safety(provider)
await db.commit()
await db.refresh(provider)
return OIDCProviderResponse.model_validate(provider)
@@ -1370,23 +1502,8 @@ async def oidc_callback(
if not provider_sub:
return RedirectResponse(url=f"{frontend_error_url}missing_sub_claim", status_code=302)
# C1: Only trust the email claim when the provider explicitly marks it verified.
# Treating absent email_verified as verified enables account-takeover: an attacker
# could register an unverified email with an IdP and auto-link to an existing account.
# Fail closed: require email_verified == True; absent/False both drop the email.
raw_email: str | None = claims.get("email")
email_verified = claims.get("email_verified")
if email_verified is not True:
if raw_email:
logger.info(
"OIDC provider %d: ignoring email for sub=%r because email_verified=%r",
provider_id,
provider_sub,
email_verified,
)
provider_email: str | None = None
else:
provider_email = raw_email
# SEC-3: resolve email via Fall A/B/C logic (see _resolve_provider_email).
provider_email = _resolve_provider_email(provider, claims, provider_sub)
# ── Step 4: Resolve / create user ────────────────────────────────────
try:
@@ -1549,7 +1666,8 @@ async def oidc_callback(
logger.error("Unexpected error in OIDC callback (%s): %s", type(exc).__name__, exc, exc_info=True)
try:
return RedirectResponse(url=f"{frontend_error_url}internal_error", status_code=302)
except Exception:
except Exception as redirect_exc:
logger.error("Failed to construct error redirect in OIDC callback: %s", redirect_exc, exc_info=True)
raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="OIDC callback failed")
+138 -18
View File
@@ -214,22 +214,66 @@ async def init_db():
async def _safe_execute(conn, sql):
"""Execute a migration statement, ignoring 'already exists' errors.
"""Execute a DDL migration statement, silently ignoring idempotency errors.
Uses a savepoint so that a failed statement doesn't poison the
surrounding transaction (required for PostgreSQL).
'already exists', 'duplicate column name' (SQLite ADD COLUMN), 'no such column'
(SQLite RENAME COLUMN), 'duplicate key', and the compound
'column … does not exist' (PostgreSQL RENAME COLUMN idempotency) are swallowed
so that re-running DDL migrations is safe. The compound check additionally
requires the SQL to be a RENAME COLUMN statement so that "does not exist" errors
from ADD COLUMN or CREATE INDEX (which would indicate schema corruption, not
idempotency) are never silently swallowed.
Any other error is logged and re-raised — callers must not assume silent
recovery, as a failure will abort the migration sequence and prevent
application startup.
Only use for DDL statements (ALTER TABLE, CREATE INDEX, etc.).
For DML backfills (UPDATE, DELETE) use conn.execute() directly inside
async with conn.begin_nested() so failures are never silently swallowed.
Uses a savepoint so that a failed statement doesn't poison the surrounding
transaction (required for PostgreSQL).
"""
from sqlalchemy import text
try:
async with conn.begin_nested():
await conn.execute(text(sql))
except (OperationalError, ProgrammingError):
pass
except (OperationalError, ProgrammingError) as exc:
msg = str(exc).lower()
# Only swallow "column … does not exist" for RENAME COLUMN — not for ADD COLUMN
# or CREATE INDEX where it would indicate schema corruption, not idempotency.
column_not_exists = "rename column" in sql.lower() and "column" in msg and "does not exist" in msg
if (
not any(k in msg for k in ("already exists", "duplicate key", "duplicate column name", "no such column"))
and not column_not_exists
):
logger.error("Migration statement failed: %s | SQL: %.200s", exc, sql)
raise
async def _migrate_normalize_printer_ids(conn) -> None:
from sqlalchemy import text
async with conn.begin_nested():
if is_sqlite():
await conn.execute(text("UPDATE api_keys SET printer_ids = NULL WHERE printer_ids = '[]'"))
else:
await conn.execute(text("UPDATE api_keys SET printer_ids = NULL WHERE printer_ids::text = '[]'"))
async def run_migrations(conn):
"""Add new columns to existing tables if they don't exist."""
"""Run all schema migrations and data backfills on startup.
Includes ALTER TABLE (add columns, rename columns, add constraints),
CREATE INDEX, CREATE TRIGGER, data UPDATE backfills, and table recreations
for complex SQLite schema changes that ALTER TABLE cannot handle.
DDL statements are wrapped in _safe_execute for idempotency.
DML backfills (UPDATE/DELETE) are executed directly via conn.execute()
inside begin_nested() so any failure is always fatal and never silently
swallowed.
"""
from sqlalchemy import text
# Migration: Add is_favorite column to print_archives
@@ -1403,7 +1447,9 @@ async def run_migrations(conn):
# Migration: Normalize empty printer_ids [] to NULL (global access) on API keys
# Previously both None and [] meant "all printers"; now [] means "no printers"
await _safe_execute(conn, "UPDATE api_keys SET printer_ids = NULL WHERE printer_ids = '[]'")
# PostgreSQL stores printer_ids as JSONB; comparing JSONB to a string literal fails
# with "operator does not exist: jsonb = unknown" — cast the literal to jsonb explicitly.
await _migrate_normalize_printer_ids(conn)
# Migration: Add auth_source column to users for LDAP support (#794)
await _safe_execute(conn, "ALTER TABLE users ADD COLUMN auth_source VARCHAR(20) DEFAULT 'local' NOT NULL")
@@ -1434,8 +1480,13 @@ async def run_migrations(conn):
)
await conn.execute(text(f"PRAGMA schema_version = {schema_version + 1}"))
await conn.execute(text("PRAGMA writable_schema = OFF"))
except (OperationalError, ProgrammingError):
pass
except (OperationalError, ProgrammingError) as exc:
logger.error(
"Failed to remove NOT NULL from users.password_hash via writable_schema — "
"OIDC/LDAP user creation will fail on this install: %s",
exc,
exc_info=True,
)
else:
await _safe_execute(conn, "ALTER TABLE users ALTER COLUMN password_hash DROP NOT NULL")
@@ -1489,7 +1540,67 @@ async def run_migrations(conn):
await _safe_execute(conn, "ALTER TABLE auth_ephemeral_tokens ADD COLUMN challenge_id VARCHAR(128)")
# Migration: Add auto_link_existing_accounts column to oidc_providers (M-4)
await _safe_execute(conn, "ALTER TABLE oidc_providers ADD COLUMN auto_link_existing_accounts BOOLEAN DEFAULT 1")
# Postgres rejects `DEFAULT 0` for BOOLEAN columns.
if is_sqlite():
await _safe_execute(conn, "ALTER TABLE oidc_providers ADD COLUMN auto_link_existing_accounts BOOLEAN DEFAULT 0")
else:
await _safe_execute(
conn, "ALTER TABLE oidc_providers ADD COLUMN auto_link_existing_accounts BOOLEAN DEFAULT false"
)
# Migration: Azure Entra ID support — configurable email claim and verification requirement
await _safe_execute(conn, "ALTER TABLE oidc_providers ADD COLUMN email_claim VARCHAR(64) DEFAULT 'email'")
# Postgres rejects `DEFAULT 1` for BOOLEAN columns.
if is_sqlite():
await _safe_execute(conn, "ALTER TABLE oidc_providers ADD COLUMN require_email_verified BOOLEAN DEFAULT 1")
else:
await _safe_execute(conn, "ALTER TABLE oidc_providers ADD COLUMN require_email_verified BOOLEAN DEFAULT true")
# SEC-1 backfill: reset auto_link on rows where the combined state is unsafe.
# Runs BEFORE the CHECK constraint below so existing installs that have
# auto_link=TRUE + unsafe email settings self-heal rather than failing when
# PostgreSQL validates ADD CONSTRAINT against existing rows ("check constraint
# is violated by some row"). On fresh installs the column defaults guarantee
# this UPDATE matches zero rows. TRUE/FALSE literals are accepted by both
# SQLite (≥ 3.23) and PostgreSQL — no dialect branch needed.
try:
async with conn.begin_nested():
await conn.execute(
text(
"UPDATE oidc_providers SET auto_link_existing_accounts = FALSE "
"WHERE auto_link_existing_accounts = TRUE "
"AND (require_email_verified = FALSE OR email_claim != 'email')"
)
)
except Exception as exc:
logger.error(
"SEC-1 safety backfill FAILED — auto_link_existing_accounts may remain enabled "
"on providers with unsafe email settings: %s",
exc,
exc_info=True,
)
raise
# SEC-1/SEC-6: Add DB-level CHECK constraint for existing PostgreSQL installs.
# SQLite does not support ALTER TABLE ADD CONSTRAINT — handled by __table_args__ at creation.
# Runs AFTER the backfill so legacy unsafe rows don't fail constraint validation.
if not is_sqlite():
try:
async with conn.begin_nested():
await conn.execute(
text(
"ALTER TABLE oidc_providers ADD CONSTRAINT ck_auto_link_requires_verified_email_claim "
"CHECK (auto_link_existing_accounts = FALSE OR (require_email_verified = TRUE AND email_claim = 'email'))"
)
)
except (OperationalError, ProgrammingError) as exc:
msg = str(exc).lower()
if "already exists" not in msg:
logger.error(
"Security constraint migration FAILED — auto_link safety constraint may not be enforced: %s",
exc,
exc_info=True,
)
raise
# Migration: Add password_changed_at to users (M-R7-B)
# Tracks the last time a user's password was changed/reset. JWTs whose iat
@@ -1505,10 +1616,8 @@ async def run_migrations(conn):
# tokens could never be invalidated via the freshness check. Setting it to
# created_at is conservative: any token issued before the account was created
# is always invalid, so this is a safe lower bound.
await _safe_execute(
conn,
"UPDATE users SET password_changed_at = created_at WHERE password_changed_at IS NULL",
)
async with conn.begin_nested():
await conn.execute(text("UPDATE users SET password_changed_at = created_at WHERE password_changed_at IS NULL"))
# Migration: Provenance columns on library_files for MakerWorld imports.
# source_url is indexed so "already imported" dedupe lookups stay O(log N)
@@ -1542,12 +1651,23 @@ async def run_migrations(conn):
# every restart. Dedupe (keep lowest id per key) and add the missing unique index
# before seeding. Safe/idempotent on both dialects — fresh installs already have
# no dupes and `create_all` already emits the index.
await _safe_execute(
conn,
"DELETE FROM settings WHERE id NOT IN (SELECT MIN(id) FROM settings GROUP BY key)",
)
async with conn.begin_nested():
await conn.execute(text("DELETE FROM settings WHERE id NOT IN (SELECT MIN(id) FROM settings GROUP BY key)"))
await _safe_execute(conn, "CREATE UNIQUE INDEX IF NOT EXISTS ix_settings_key ON settings(key)")
# Migration: Normalise provider_email to lowercase (SEC-3).
# Required for Entra ID where UPN/email claims may arrive in mixed case.
# LOWER() is supported by both SQLite and PostgreSQL; the UPDATE is idempotent.
# Executed directly (not via _safe_execute) so any column-reference failure
# is always fatal and never silently swallowed.
async with conn.begin_nested():
await conn.execute(
text(
"UPDATE user_oidc_links SET provider_email = LOWER(provider_email) "
"WHERE provider_email IS NOT NULL AND provider_email != LOWER(provider_email)"
)
)
# Seed default settings keys that must exist on fresh install
default_settings = [
("advanced_auth_enabled", "false"),
+24 -1
View File
@@ -2,7 +2,7 @@ from __future__ import annotations
from datetime import datetime
from sqlalchemy import Boolean, DateTime, ForeignKey, Integer, String, Text, UniqueConstraint, func
from sqlalchemy import Boolean, CheckConstraint, DateTime, ForeignKey, Integer, String, Text, UniqueConstraint, func
from sqlalchemy.orm import Mapped, mapped_column, relationship
from backend.app.core.database import Base
@@ -21,6 +21,15 @@ class OIDCProvider(Base):
"""
__tablename__ = "oidc_providers"
__table_args__ = (
# DB-level enforcement of SEC-1/SEC-6: auto_link is only safe when
# require_email_verified=True AND email_claim='email'. Enforced on new
# installations; existing tables get this via the PostgreSQL-only migration.
CheckConstraint(
"auto_link_existing_accounts = FALSE OR (require_email_verified = TRUE AND email_claim = 'email')",
name="ck_auto_link_requires_verified_email_claim",
),
)
id: Mapped[int] = mapped_column(primary_key=True)
# Human-readable name shown on the login button (e.g. "PocketID", "Google")
@@ -50,6 +59,20 @@ class OIDCProvider(Base):
# operators must explicitly opt-in to prevent an attacker-controlled IdP from
# silently hijacking local accounts via email matching (M-2 fix).
auto_link_existing_accounts: Mapped[bool] = mapped_column(Boolean, default=False)
# JWT claim name used as the email identity (default "email").
# Set to "preferred_username" or "upn" for Azure Entra ID, which does not send
# email_verified — using a custom claim skips the email_verified check entirely
# and is the recommended Azure configuration.
# Has no interaction with require_email_verified when set to a non-"email" value:
# custom claims never perform an email_verified check regardless of that setting.
email_claim: Mapped[str] = mapped_column(String(64), default="email")
# When True (default), the "email" claim is only trusted when email_verified=True.
# Set to False to accept the email even when email_verified is absent — required
# for providers like Azure Entra ID that never send email_verified and where a
# custom claim (email_claim != "email") is not preferred.
# Has no effect when email_claim is not "email": the custom-claim path never
# performs an email_verified check regardless of this setting.
require_email_verified: Mapped[bool] = mapped_column(Boolean, default=True)
# Optional icon URL (SVG/PNG) shown on the login button
icon_url: Mapped[str | None] = mapped_column(Text, nullable=True, default=None)
created_at: Mapped[datetime] = mapped_column(DateTime, server_default=func.now())
+56 -3
View File
@@ -1,7 +1,7 @@
import re
from typing import Literal
from pydantic import BaseModel, Field, field_validator
from pydantic import BaseModel, Field, field_validator, model_validator
def _validate_password_complexity(v: str) -> str:
@@ -296,6 +296,19 @@ class AdminDisable2FARequest(BaseModel):
# ---------------------------------------------------------------------------
AUTO_LINK_REQUIREMENTS_ERROR = (
"auto_link_existing_accounts requires require_email_verified=True and email_claim='email'"
)
def _validate_email_claim_name(v: str) -> str:
# Accepts only alphanumeric/underscore/hyphen claim names starting with a letter —
# prevents log injection and limits the attack surface of operator-supplied claim names.
if not re.fullmatch(r"[a-zA-Z][a-zA-Z0-9_\-]{0,63}", v):
raise ValueError("Invalid claim name")
return v
def _validate_icon_url(v: str | None) -> str | None:
"""Reject non-HTTPS icon URLs to prevent SSRF / mixed-content issues."""
if v is None:
@@ -355,27 +368,45 @@ class OIDCProviderCreate(BaseModel):
is_enabled: bool = True
auto_create_users: bool = False
auto_link_existing_accounts: bool = False # M-2: conservative default, opt-in only
email_claim: str = Field(default="email", max_length=64)
require_email_verified: bool = True
icon_url: str | None = None
@field_validator("issuer_url")
@classmethod
def validate_issuer_url(cls, v: str) -> str:
result = _validate_issuer_url(v)
assert result is not None
if result is None:
raise ValueError("issuer_url is required")
return result
@field_validator("scopes")
@classmethod
def validate_scopes(cls, v: str) -> str:
result = _validate_scopes(v)
assert result is not None
if result is None:
raise ValueError("scopes is required")
return result
@field_validator("email_claim")
@classmethod
def validate_email_claim(cls, v: str) -> str:
return _validate_email_claim_name(v)
@field_validator("icon_url")
@classmethod
def validate_icon_url(cls, v: str | None) -> str | None:
return _validate_icon_url(v)
# SEC-1 + SEC-6: auto_link requires both require_email_verified=True AND email_claim="email".
# Fall B (require_email_verified=False) accepts absent email_verified → account-takeover risk.
# Fall C (custom claim) skips email_verified entirely → same risk.
@model_validator(mode="after")
def check_auto_link_requires_verified(self) -> "OIDCProviderCreate":
if self.auto_link_existing_accounts and (not self.require_email_verified or self.email_claim != "email"):
raise ValueError(AUTO_LINK_REQUIREMENTS_ERROR)
return self
class OIDCProviderUpdate(BaseModel):
name: str | None = Field(default=None, max_length=100)
@@ -392,6 +423,8 @@ class OIDCProviderUpdate(BaseModel):
is_enabled: bool | None = None
auto_create_users: bool | None = None
auto_link_existing_accounts: bool | None = None
email_claim: str | None = Field(default=None, max_length=64)
require_email_verified: bool | None = None
icon_url: str | None = None
@field_validator("scopes")
@@ -399,11 +432,29 @@ class OIDCProviderUpdate(BaseModel):
def validate_scopes(cls, v: str | None) -> str | None:
return _validate_scopes(v)
@field_validator("email_claim")
@classmethod
def validate_email_claim(cls, v: str | None) -> str | None:
if v is None:
return None
return _validate_email_claim_name(v)
@field_validator("icon_url")
@classmethod
def validate_icon_url(cls, v: str | None) -> str | None:
return _validate_icon_url(v)
# SEC-1 + SEC-6 (schema-level): blocks only when both conflicting fields arrive
# in the same request. Partial updates spanning two requests are caught by the
# Combined-State-Guard in the route handler after the setattr loop.
@model_validator(mode="after")
def check_auto_link_requires_verified(self) -> "OIDCProviderUpdate":
if self.auto_link_existing_accounts is True and (
self.require_email_verified is False or (self.email_claim is not None and self.email_claim != "email")
):
raise ValueError(AUTO_LINK_REQUIREMENTS_ERROR)
return self
class OIDCProviderResponse(BaseModel):
id: int
@@ -414,6 +465,8 @@ class OIDCProviderResponse(BaseModel):
is_enabled: bool
auto_create_users: bool
auto_link_existing_accounts: bool = False
email_claim: str = "email"
require_email_verified: bool = True
icon_url: str | None = None
class Config: