[PR #1118] [MERGED] feat(oidc): Azure Entra ID support — configurable email claim & verification + Remember Me persistent login #1160

New issue

Closed

opened 2026-05-06 12:35:25 +02:00 by BreizhHardware · 0 comments

BreizhHardware commented

2026-05-06 12:35:25 +02:00

Owner

📋 Pull Request Information

Original PR: https://github.com/maziggy/bambuddy/pull/1118
Author: @netscout2001
Created: 4/24/2026
Status: ✅ Merged
Merged: 4/25/2026
Merged by: @maziggy

Base: dev ← Head: feature/2fa-oidc-authentication

📝 Commits (10+)

a3e4b96 feat(oidc): add Azure Entra ID support with configurable email claim resolution
50e9cd3 fix(oidc): harden email claim resolution, guards, and test coverage
460818b fix(oidc): apply maintainer review fixes for Azure Entra ID support
fad989d feat(auth): add Remember Me option to login page
0640101 fix(db): swallow 'no such column' in _safe_execute for idempotent RENAME COLUMN migrations
190c268 fix(oidc): harden _safe_execute, extract email/autolink helpers, add test coverage
9c18b7f fix(db): restore 'no such column' and 'duplicate column name' to _safe_execute swallow list
d7fb0e6 fix(db): swallow PostgreSQL 'does not exist' error for idempotent RENAME COLUMN migrations
216d16e fix(db,auth): move DML backfills out of _safe_execute; split setAuthToken storage writes
c990775 fix(oidc,db): address PR #1118 maintainer review — migration safety, email shape checks, dialect branching

📊 Changes

22 files changed (+2237 additions, -74 deletions)

View changed files

📝 backend/app/api/routes/mfa.py (+136 -18)
📝 backend/app/core/database.py (+132 -19)
📝 backend/app/models/oidc_provider.py (+24 -1)
📝 backend/app/schemas/auth.py (+56 -3)
📝 backend/tests/integration/test_mfa_api.py (+777 -0)
📝 backend/tests/unit/test_db_dialect.py (+313 -4)
➕ docs/authentication/entra-id.md (+82 -0)
📝 frontend/src/__tests__/api/client.test.ts (+56 -0)
➕ frontend/src/__tests__/components/OIDCProviderSettings.test.tsx (+125 -0)
📝 frontend/src/__tests__/pages/LoginPage.test.tsx (+328 -1)
📝 frontend/src/api/client.ts (+25 -11)
📝 frontend/src/components/OIDCProviderSettings.tsx (+50 -1)
📝 frontend/src/contexts/AuthContext.tsx (+9 -9)
📝 frontend/src/i18n/locales/de.ts (+8 -0)
📝 frontend/src/i18n/locales/en.ts (+8 -0)
📝 frontend/src/i18n/locales/fr.ts (+8 -0)
📝 frontend/src/i18n/locales/it.ts (+8 -0)
📝 frontend/src/i18n/locales/ja.ts (+8 -0)
📝 frontend/src/i18n/locales/pt-BR.ts (+8 -0)
📝 frontend/src/i18n/locales/zh-CN.ts (+8 -0)

...and 2 more files

📄 Description

Description

This PR combines two improvements to the OIDC / authentication flow:

1 — Azure Entra ID support + hardening (maintainer review fixes)

Adds Azure Entra ID support to the OIDC provider configuration by introducing two new fields: email_claim and require_email_verified.

Azure Entra ID never includes the email_verified claim in its ID tokens. The previous code dropped the email whenever email_verified was absent (fail-closed), breaking Azure logins. This PR introduces three configurable email resolution paths:

Case A (default): email_claim="email" + require_email_verified=true — standard behaviour, unchanged
Case B: email_claim="email" + require_email_verified=false — permissive mode, accepts email when email_verified is absent
Case C: email_claim != "email" — custom claim (preferred_username, upn), no email_verified check; recommended for Azure Entra ID

auto_link_existing_accounts is blocked at schema level, route level, and DB level when either new field would make auto-linking unsafe (SEC-1/SEC-6).

This PR also addresses the subsequent maintainer review feedback:

Regex hardening: _EMAIL_SHAPE_RE is now precompiled at module level and uses fullmatch() with a stricter pattern that requires a dot in the domain (local@domain.tld) — rejects x@nodot, @domain, x@, etc.
PII removed from logs: The raw email value is no longer logged when the shape check fails; only provider_id, email_claim name, and sub are emitted.
PostgreSQL-compatible CheckConstraint: Literals changed from = 0 / = 1 to = FALSE / = TRUE so the constraint compiles correctly on PostgreSQL.
_safe_execute re-raises on real errors: Non-idempotency errors are now logged with logger.error and re-raised so a failed migration aborts startup loudly instead of silently continuing.
_safe_execute swallows PostgreSQL RENAME COLUMN idempotency errors: column "xyz" does not exist (raised by PostgreSQL when re-running an already-applied RENAME COLUMN migration) is now swallowed correctly, fixing a startup failure on existing PostgreSQL installations.
DML backfills moved out of _safe_execute: The three data-backfill statements (UPDATE api_keys, UPDATE users SET password_changed_at, DELETE FROM settings) now use direct conn.execute() inside a savepoint instead of going through _safe_execute. This ensures any column-reference failure is always fatal and never silently swallowed — most importantly for the security-critical password_changed_at backfill.
provider_email lowercasing migration: Normalises existing mixed-case UPN values (common with Azure Entra ID) to lowercase for consistent matching.
Azure Entra ID setup guide: New docs/authentication/entra-id.md covering app registration, client secret, issuer URL, BamBuddy provider config, preferred_username vs email claim, and a troubleshooting table.
New tests: Real in-memory SQLite integration tests for _safe_execute reraise/swallow behaviour, provider_email lowercasing migration, PostgreSQL RENAME COLUMN idempotency, and FALSE/TRUE CheckConstraint enforcement.

"Remember Me" checkbox added to the login form (unchecked by default).
When checked, the JWT is written to both sessionStorage and localStorage, so the session survives a tab close / browser restart.
Introduces TokenPersistence = 'session' | 'persistent' type, replacing the previous persist?: boolean parameter throughout client.ts, AuthContext, and LoginPage.
OIDC round-trip support: The preference is saved to sessionStorage as auth_remember_me=1 before the provider redirect and consumed (read + removed atomically) in the OIDC callback useEffect, so the setting carries through the external redirect where React state is lost.
Persistence also threads through the OIDC + 2FA combined flow.
Logout storage safety: sessionStorage and localStorage writes/removals now happen in independent try/catch blocks so a failure on one storage (e.g. localStorage blocked in private mode) cannot prevent the other from being cleared — a previously possible scenario where a user could believe they were logged out while their localStorage token remained active.
All storage writes are wrapped in try-catch with console.warn to handle private browsing / quota errors gracefully (in-memory token still works for the current tab).
rememberMe i18n key added to all 8 locale files (en, de, fr, it, pt-BR, zh-CN, ja, zh-TW).
Full test coverage: persistence mode tests in client.test.ts; Remember Me checkbox, session/persistent storage, OIDC carry-through, OIDC+2FA carry-through, error cleanup, and negative cases in LoginPage.test.tsx.

Fixes #902 #1088

Documentation

https://github.com/maziggy/bambuddy-wiki/pull/19

Type of Change

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Documentation update
Test addition or update

Changes Made

Backend

backend/app/api/routes/mfa.py — precompiled _EMAIL_SHAPE_RE, PII-free log, three-case email resolution (Case A/B/C), _is_valid_email_shaped helper, Combined-State-Guard in PUT handler
backend/app/models/oidc_provider.py — email_claim and require_email_verified ORM fields, CheckConstraint uses FALSE/TRUE
backend/app/core/database.py — column migrations for new fields, PostgreSQL ADD CONSTRAINT migration, _safe_execute logs + re-raises non-idempotency errors + swallows PostgreSQL RENAME COLUMN idempotency errors, DML backfills moved to direct conn.execute(), provider_email lowercasing migration
backend/app/schemas/auth.py — new fields on OIDCProviderCreate / OIDCProviderUpdate / OIDCProviderResponse, model_validator for SEC-1/SEC-6 on create and update
backend/tests/integration/test_mfa_api.py — 19 new tests: 13-case parametrized matrix (Case A/B/C), SEC-1/SEC-6 guard tests, Combined-State-Guard tests, response field coverage, no-dot-in-domain matrix case
backend/tests/unit/test_db_dialect.py — real SQLite integration tests for _safe_execute and migrations, PostgreSQL RENAME COLUMN idempotency mock test
docs/authentication/entra-id.md — new Azure Entra ID setup guide

Frontend

frontend/src/api/client.ts — TokenPersistence type, updated setAuthToken with split try/catch for independent storage failure handling, new fields on OIDCProvider and OIDCProviderCreate interfaces
frontend/src/contexts/AuthContext.tsx — login/loginWithToken accept TokenPersistence
frontend/src/pages/LoginPage.tsx — Remember Me checkbox, consumeSavedRememberMe(), toPersistence() helper, OIDC sessionStorage flag
frontend/src/components/OIDCProviderSettings.tsx — toggle for require_email_verified, input for email_claim, both shown in provider info view
frontend/src/i18n/locales/*.ts — rememberMe key + OIDC settings keys in all 8 locale files
frontend/src/__tests__/api/client.test.ts — 5 new persistence mode tests (including independent storage-failure paths on logout)
frontend/src/__tests__/pages/LoginPage.test.tsx — 9 new Remember Me / OIDC carry-through tests

Screenshots

Testing

I have tested this on my local machine

149 backend tests pass (pytest tests/integration/test_mfa_api.py tests/unit/test_db_dialect.py). 1479 frontend tests pass (vitest run). No ruff or TypeScript warnings.

Checklist

My code follows the project's coding style
My changes generate no new warnings
I have tested my changes thoroughly

Additional Notes

Existing OIDC providers and login flows are fully backwards-compatible. Both new features are opt-in by default: require_email_verified=true / email_claim='email' preserves the previous behaviour; the Remember Me checkbox defaults to unchecked.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/maziggy/bambuddy/pull/1118 **Author:** [@netscout2001](https://github.com/netscout2001) **Created:** 4/24/2026 **Status:** ✅ Merged **Merged:** 4/25/2026 **Merged by:** [@maziggy](https://github.com/maziggy) **Base:** `dev` ← **Head:** `feature/2fa-oidc-authentication` --- ### 📝 Commits (10+) - [`a3e4b96`](https://github.com/maziggy/bambuddy/commit/a3e4b968a51bd461b07a09f1ce9df5a17d6425ba) feat(oidc): add Azure Entra ID support with configurable email claim resolution - [`50e9cd3`](https://github.com/maziggy/bambuddy/commit/50e9cd312d38815c597a92fc63f23b13dbea0686) fix(oidc): harden email claim resolution, guards, and test coverage - [`460818b`](https://github.com/maziggy/bambuddy/commit/460818ba15317a22c6a8990feab2289eedc67bb9) fix(oidc): apply maintainer review fixes for Azure Entra ID support - [`fad989d`](https://github.com/maziggy/bambuddy/commit/fad989d9b9bba98aeba1a0b07d27b7f02ab27c59) feat(auth): add Remember Me option to login page - [`0640101`](https://github.com/maziggy/bambuddy/commit/06401017473d98e31e6c8b34bb36c8d141ca414f) fix(db): swallow 'no such column' in _safe_execute for idempotent RENAME COLUMN migrations - [`190c268`](https://github.com/maziggy/bambuddy/commit/190c268fcf3779dc6d5df5f9d25b69dab7b27f52) fix(oidc): harden _safe_execute, extract email/autolink helpers, add test coverage - [`9c18b7f`](https://github.com/maziggy/bambuddy/commit/9c18b7f856806331ff5eaab8138c8032b525ab14) fix(db): restore 'no such column' and 'duplicate column name' to _safe_execute swallow list - [`d7fb0e6`](https://github.com/maziggy/bambuddy/commit/d7fb0e6a26430a42f3e6b96968dff105522318af) fix(db): swallow PostgreSQL 'does not exist' error for idempotent RENAME COLUMN migrations - [`216d16e`](https://github.com/maziggy/bambuddy/commit/216d16e7ff71dcb6670166e94e60aaf9a2bdf495) fix(db,auth): move DML backfills out of _safe_execute; split setAuthToken storage writes - [`c990775`](https://github.com/maziggy/bambuddy/commit/c99077572c3d0d9274c9c791ffe14d9f52de0ae9) fix(oidc,db): address PR #1118 maintainer review — migration safety, email shape checks, dialect branching ### 📊 Changes **22 files changed** (+2237 additions, -74 deletions) <details> <summary>View changed files</summary> 📝 `backend/app/api/routes/mfa.py` (+136 -18) 📝 `backend/app/core/database.py` (+132 -19) 📝 `backend/app/models/oidc_provider.py` (+24 -1) 📝 `backend/app/schemas/auth.py` (+56 -3) 📝 `backend/tests/integration/test_mfa_api.py` (+777 -0) 📝 `backend/tests/unit/test_db_dialect.py` (+313 -4) ➕ `docs/authentication/entra-id.md` (+82 -0) 📝 `frontend/src/__tests__/api/client.test.ts` (+56 -0) ➕ `frontend/src/__tests__/components/OIDCProviderSettings.test.tsx` (+125 -0) 📝 `frontend/src/__tests__/pages/LoginPage.test.tsx` (+328 -1) 📝 `frontend/src/api/client.ts` (+25 -11) 📝 `frontend/src/components/OIDCProviderSettings.tsx` (+50 -1) 📝 `frontend/src/contexts/AuthContext.tsx` (+9 -9) 📝 `frontend/src/i18n/locales/de.ts` (+8 -0) 📝 `frontend/src/i18n/locales/en.ts` (+8 -0) 📝 `frontend/src/i18n/locales/fr.ts` (+8 -0) 📝 `frontend/src/i18n/locales/it.ts` (+8 -0) 📝 `frontend/src/i18n/locales/ja.ts` (+8 -0) 📝 `frontend/src/i18n/locales/pt-BR.ts` (+8 -0) 📝 `frontend/src/i18n/locales/zh-CN.ts` (+8 -0) _...and 2 more files_ </details> ### 📄 Description ## Description This PR combines two improvements to the OIDC / authentication flow: ### 1 — Azure Entra ID support + hardening (maintainer review fixes) Adds Azure Entra ID support to the OIDC provider configuration by introducing two new fields: `email_claim` and `require_email_verified`. Azure Entra ID never includes the `email_verified` claim in its ID tokens. The previous code dropped the email whenever `email_verified` was absent (fail-closed), breaking Azure logins. This PR introduces three configurable email resolution paths: - **Case A** *(default)*: `email_claim="email"` + `require_email_verified=true` — standard behaviour, unchanged - **Case B**: `email_claim="email"` + `require_email_verified=false` — permissive mode, accepts email when `email_verified` is absent - **Case C**: `email_claim != "email"` — custom claim (`preferred_username`, `upn`), no `email_verified` check; recommended for Azure Entra ID `auto_link_existing_accounts` is blocked at schema level, route level, and DB level when either new field would make auto-linking unsafe (SEC-1/SEC-6). This PR also addresses the subsequent maintainer review feedback: - **Regex hardening**: `_EMAIL_SHAPE_RE` is now precompiled at module level and uses `fullmatch()` with a stricter pattern that requires a dot in the domain (`local@domain.tld`) — rejects `x@nodot`, `@domain`, `x@`, etc. - **PII removed from logs**: The raw email value is no longer logged when the shape check fails; only `provider_id`, `email_claim` name, and `sub` are emitted. - **PostgreSQL-compatible CheckConstraint**: Literals changed from `= 0` / `= 1` to `= FALSE` / `= TRUE` so the constraint compiles correctly on PostgreSQL. - **`_safe_execute` re-raises on real errors**: Non-idempotency errors are now logged with `logger.error` and re-raised so a failed migration aborts startup loudly instead of silently continuing. - **`_safe_execute` swallows PostgreSQL RENAME COLUMN idempotency errors**: `column "xyz" does not exist` (raised by PostgreSQL when re-running an already-applied `RENAME COLUMN` migration) is now swallowed correctly, fixing a startup failure on existing PostgreSQL installations. - **DML backfills moved out of `_safe_execute`**: The three data-backfill statements (`UPDATE api_keys`, `UPDATE users SET password_changed_at`, `DELETE FROM settings`) now use direct `conn.execute()` inside a savepoint instead of going through `_safe_execute`. This ensures any column-reference failure is always fatal and never silently swallowed — most importantly for the security-critical `password_changed_at` backfill. - **`provider_email` lowercasing migration**: Normalises existing mixed-case UPN values (common with Azure Entra ID) to lowercase for consistent matching. - **Azure Entra ID setup guide**: New `docs/authentication/entra-id.md` covering app registration, client secret, issuer URL, BamBuddy provider config, `preferred_username` vs `email` claim, and a troubleshooting table. - **New tests**: Real in-memory SQLite integration tests for `_safe_execute` reraise/swallow behaviour, `provider_email` lowercasing migration, PostgreSQL RENAME COLUMN idempotency, and `FALSE`/`TRUE` CheckConstraint enforcement. ### 2 — Remember Me persistent login - **"Remember Me" checkbox** added to the login form (unchecked by default). - When checked, the JWT is written to both `sessionStorage` **and** `localStorage`, so the session survives a tab close / browser restart. - Introduces `TokenPersistence = 'session' | 'persistent'` type, replacing the previous `persist?: boolean` parameter throughout `client.ts`, `AuthContext`, and `LoginPage`. - **OIDC round-trip support**: The preference is saved to `sessionStorage` as `auth_remember_me=1` before the provider redirect and consumed (read + removed atomically) in the OIDC callback `useEffect`, so the setting carries through the external redirect where React state is lost. - Persistence also threads through the **OIDC + 2FA** combined flow. - **Logout storage safety**: `sessionStorage` and `localStorage` writes/removals now happen in independent `try/catch` blocks so a failure on one storage (e.g. `localStorage` blocked in private mode) cannot prevent the other from being cleared — a previously possible scenario where a user could believe they were logged out while their `localStorage` token remained active. - All storage writes are wrapped in `try-catch` with `console.warn` to handle private browsing / quota errors gracefully (in-memory token still works for the current tab). - `rememberMe` i18n key added to all 8 locale files (en, de, fr, it, pt-BR, zh-CN, ja, zh-TW). - Full test coverage: persistence mode tests in `client.test.ts`; Remember Me checkbox, session/persistent storage, OIDC carry-through, OIDC+2FA carry-through, error cleanup, and negative cases in `LoginPage.test.tsx`. ## Related Issues Fixes #902 #1088 ## Documentation [https://github.com/maziggy/bambuddy-wiki/pull/19](https://github.com/maziggy/bambuddy-wiki/pull/19) ## Type of Change - [x] Bug fix (non-breaking change that fixes an issue) - [x] New feature (non-breaking change that adds functionality) - [x] Documentation update - [x] Test addition or update ## Changes Made **Backend** - `backend/app/api/routes/mfa.py` — precompiled `_EMAIL_SHAPE_RE`, PII-free log, three-case email resolution (Case A/B/C), `_is_valid_email_shaped` helper, Combined-State-Guard in PUT handler - `backend/app/models/oidc_provider.py` — `email_claim` and `require_email_verified` ORM fields, `CheckConstraint` uses `FALSE`/`TRUE` - `backend/app/core/database.py` — column migrations for new fields, PostgreSQL `ADD CONSTRAINT` migration, `_safe_execute` logs + re-raises non-idempotency errors + swallows PostgreSQL RENAME COLUMN idempotency errors, DML backfills moved to direct `conn.execute()`, `provider_email` lowercasing migration - `backend/app/schemas/auth.py` — new fields on `OIDCProviderCreate` / `OIDCProviderUpdate` / `OIDCProviderResponse`, `model_validator` for SEC-1/SEC-6 on create and update - `backend/tests/integration/test_mfa_api.py` — 19 new tests: 13-case parametrized matrix (Case A/B/C), SEC-1/SEC-6 guard tests, Combined-State-Guard tests, response field coverage, `no-dot-in-domain` matrix case - `backend/tests/unit/test_db_dialect.py` — real SQLite integration tests for `_safe_execute` and migrations, PostgreSQL RENAME COLUMN idempotency mock test - `docs/authentication/entra-id.md` — new Azure Entra ID setup guide **Frontend** - `frontend/src/api/client.ts` — `TokenPersistence` type, updated `setAuthToken` with split try/catch for independent storage failure handling, new fields on `OIDCProvider` and `OIDCProviderCreate` interfaces - `frontend/src/contexts/AuthContext.tsx` — `login`/`loginWithToken` accept `TokenPersistence` - `frontend/src/pages/LoginPage.tsx` — Remember Me checkbox, `consumeSavedRememberMe()`, `toPersistence()` helper, OIDC sessionStorage flag - `frontend/src/components/OIDCProviderSettings.tsx` — toggle for `require_email_verified`, input for `email_claim`, both shown in provider info view - `frontend/src/i18n/locales/*.ts` — `rememberMe` key + OIDC settings keys in all 8 locale files - `frontend/src/__tests__/api/client.test.ts` — 5 new persistence mode tests (including independent storage-failure paths on logout) - `frontend/src/__tests__/pages/LoginPage.test.tsx` — 9 new Remember Me / OIDC carry-through tests ## Screenshots <img width="1120" height="880" alt="image" src="https://github.com/user-attachments/assets/7aa12bfb-6ed7-4d2c-b803-06da6f01b217" /> <img width="460" height="716" alt="image" src="https://github.com/user-attachments/assets/68437d8f-887f-4841-9860-a69b7c2fcec1" /> ## Testing - [x] I have tested this on my local machine 149 backend tests pass (`pytest tests/integration/test_mfa_api.py tests/unit/test_db_dialect.py`). 1479 frontend tests pass (`vitest run`). No ruff or TypeScript warnings. ## Checklist - [x] My code follows the project's coding style - [x] My changes generate no new warnings - [x] I have tested my changes thoroughly ## Additional Notes Existing OIDC providers and login flows are fully backwards-compatible. Both new features are opt-in by default: `require_email_verified=true` / `email_claim='email'` preserves the previous behaviour; the Remember Me checkbox defaults to unchecked. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>