[PR #1142] [MERGED] fix(oidc): Allow `auto_link_existing_accounts` with custom email claims (Azure Entra ID) #1170

New issue

Closed

opened 2026-05-07 00:16:26 +02:00 by BreizhHardware · 0 comments

BreizhHardware commented

2026-05-07 00:16:26 +02:00

Owner

📋 Pull Request Information

Original PR: https://github.com/maziggy/bambuddy/pull/1142
Author: @netscout2001
Created: 4/27/2026
Status: ✅ Merged
Merged: 4/28/2026
Merged by: @maziggy

Base: dev ← Head: fix/oidc-allow-auto-link-with-custom-claim

📝 Commits (10+)

50f482c chore(i18n): extend parity gate to all locales with strict/info tiers
ce88bd6 Bumped version
339cd40 Added new update.sh and update docs
eff65ba fix(oidc): allow auto_link_existing_accounts with custom email claims
585eb47 fix(oidc): prevent require_email_verified toggle from jumping layout
606f9ea Merge branch 'dev' into fix/oidc-allow-auto-link-with-custom-claim
d8d733b Merge branch 'dev' into fix/oidc-allow-auto-link-with-custom-claim
21a3200 fix(oidc): address PR #1142 review feedback
86715ba Merge branch 'dev' into fix/oidc-allow-auto-link-with-custom-claim
65a26fd Merge branch 'dev' into fix/oidc-allow-auto-link-with-custom-claim

📊 Changes

19 files changed (+628 additions, -87 deletions)

View changed files

📝 CHANGELOG.md (+2 -0)
📝 UPDATING.md (+10 -9)
📝 backend/app/api/routes/mfa.py (+6 -7)
📝 backend/app/core/database.py (+104 -11)
📝 backend/app/models/oidc_provider.py (+4 -4)
📝 backend/app/schemas/auth.py (+12 -9)
📝 backend/tests/integration/test_mfa_api.py (+213 -13)
📝 backend/tests/unit/test_db_dialect.py (+235 -17)
📝 frontend/scripts/check-i18n-parity.mjs (+1 -14)
📝 frontend/src/__tests__/components/OIDCProviderSettings.test.tsx (+28 -1)
📝 frontend/src/components/OIDCProviderSettings.tsx (+5 -2)
📝 frontend/src/i18n/locales/de.ts (+1 -0)
📝 frontend/src/i18n/locales/en.ts (+1 -0)
📝 frontend/src/i18n/locales/fr.ts (+1 -0)
📝 frontend/src/i18n/locales/it.ts (+1 -0)
📝 frontend/src/i18n/locales/ja.ts (+1 -0)
📝 frontend/src/i18n/locales/pt-BR.ts (+1 -0)
📝 frontend/src/i18n/locales/zh-CN.ts (+1 -0)
📝 frontend/src/i18n/locales/zh-TW.ts (+1 -0)

📄 Description

Description

Relaxes the SEC-1 guard for auto_link_existing_accounts so that providers using a custom email_claim (e.g. preferred_username, upn) — the recommended Azure Entra ID configuration — can safely enable automatic account linking.

Previously the guard blocked all combinations except email_claim='email' + require_email_verified=True. This also blocked Azure Entra ID configurations that use a custom claim, even though those are not subject to the email-verification bypass attack.

The new guard only blocks the genuinely unsafe combination (Fall B): email_claim='email' + require_email_verified=False. Custom-claim paths (Fall C) never perform an email_verified check, so an attacker-controlled IdP cannot exploit email matching — auto-linking is safe there.

Docs ->

Fixes maziggy/bambuddy#1088

Documentation

Docs PR(s) linked above: infos in CHANGELOG
No docs update required — reason: internal security guard change with no new config keys or user-visible behavior change beyond unblocking a previously rejected configuration

Type of Change

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation update
Code refactoring
Performance improvement
Test addition or update

Changes Made

models/oidc_provider.py: Updated CheckConstraint formula from require_email_verified = TRUE AND email_claim = 'email' to email_claim != 'email' OR require_email_verified = TRUE (only blocks Fall B)
schemas/auth.py: Updated OIDCProviderCreate and OIDCProviderUpdate model validators and error message to match the new guard condition
api/routes/mfa.py: Updated _enforce_auto_link_safety() combined-state guard to the same Fall-B-only condition
core/database.py: Added _migrate_update_auto_link_constraint() to update the DB constraint on existing installations — SQLite via table recreation (shadow table pattern), PostgreSQL via DROP CONSTRAINT IF EXISTS + ADD CONSTRAINT; narrowed backfill SQL to only reset unsafe Fall B rows
tests/integration/test_mfa_api.py: Updated 4 tests and added 2 new tests covering the newly allowed Fall C scenarios and continued blocking of Fall B
tests/unit/test_db_dialect.py: Updated 2 unit tests and added 4 new tests for the new constraint formula and the SQLite migration function

Testing

I have tested this on my local machine
I have tested with my printer model:

163 relevant tests pass (tests/integration/test_mfa_api.py + tests/unit/test_db_dialect.py). Full test suite: no regressions.

Key scenarios verified:

Scenario	email_claim	require_email_verified	auto_link	Result
Case A (standard)	`email`	`True`	`True`	✅ Allowed
Case B (unsafe)	`email`	`False`	`True`	❌ Blocked (422)
Case C (Azure)	`preferred_username`	any	`True`	✅ Allowed
Case C (Azure)	`upn`	`False`	`True`	✅ Allowed

Additional Notes

Migration behavior:

New installations: The updated CHECK constraint is applied at schema creation time — no action needed.
Existing SQLite installations: _migrate_update_auto_link_constraint() recreates the oidc_providers table with the new constraint (SQLite does not support ALTER TABLE DROP/ADD CONSTRAINT). The old constraint is detected via sqlite_master; the migration is a no-op if already on the new formula.
Existing PostgreSQL installations: The function issues DROP CONSTRAINT IF EXISTS + ADD CONSTRAINT — fully idempotent.
Only Fall B rows (email_claim='email' + require_email_verified=False + auto_link=True) are reset by the backfill; Fall C rows are left unchanged.

_{🔄 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/1142 **Author:** [@netscout2001](https://github.com/netscout2001) **Created:** 4/27/2026 **Status:** ✅ Merged **Merged:** 4/28/2026 **Merged by:** [@maziggy](https://github.com/maziggy) **Base:** `dev` ← **Head:** `fix/oidc-allow-auto-link-with-custom-claim` --- ### 📝 Commits (10+) - [`50f482c`](https://github.com/maziggy/bambuddy/commit/50f482c945b25e7b02e387987f4f27502753f2c6) chore(i18n): extend parity gate to all locales with strict/info tiers - [`ce88bd6`](https://github.com/maziggy/bambuddy/commit/ce88bd6a796912ee5b7cc35bddba134da6ec9869) Bumped version - [`339cd40`](https://github.com/maziggy/bambuddy/commit/339cd405ad40c9f6eb6c13819db5446d89b0f62d) Added new update.sh and update docs - [`eff65ba`](https://github.com/maziggy/bambuddy/commit/eff65ba7617d7932d7555d16277af68fe07f5e5a) fix(oidc): allow auto_link_existing_accounts with custom email claims - [`585eb47`](https://github.com/maziggy/bambuddy/commit/585eb477f5d58af55feea04c91f16d5158a0d624) fix(oidc): prevent require_email_verified toggle from jumping layout - [`606f9ea`](https://github.com/maziggy/bambuddy/commit/606f9eae7934d4518a1d2f4939a75fa8d455d943) Merge branch 'dev' into fix/oidc-allow-auto-link-with-custom-claim - [`d8d733b`](https://github.com/maziggy/bambuddy/commit/d8d733b805d1ddbb762bf3c6a177ba67f9f1e4ac) Merge branch 'dev' into fix/oidc-allow-auto-link-with-custom-claim - [`21a3200`](https://github.com/maziggy/bambuddy/commit/21a320048b05535984b32fcb0bbd073f280379ac) fix(oidc): address PR #1142 review feedback - [`86715ba`](https://github.com/maziggy/bambuddy/commit/86715ba4a43e6bc0bfe5381e49adb3bd071c8d76) Merge branch 'dev' into fix/oidc-allow-auto-link-with-custom-claim - [`65a26fd`](https://github.com/maziggy/bambuddy/commit/65a26fd58338cc91707b9d9b331258d41383df0a) Merge branch 'dev' into fix/oidc-allow-auto-link-with-custom-claim ### 📊 Changes **19 files changed** (+628 additions, -87 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+2 -0) 📝 `UPDATING.md` (+10 -9) 📝 `backend/app/api/routes/mfa.py` (+6 -7) 📝 `backend/app/core/database.py` (+104 -11) 📝 `backend/app/models/oidc_provider.py` (+4 -4) 📝 `backend/app/schemas/auth.py` (+12 -9) 📝 `backend/tests/integration/test_mfa_api.py` (+213 -13) 📝 `backend/tests/unit/test_db_dialect.py` (+235 -17) 📝 `frontend/scripts/check-i18n-parity.mjs` (+1 -14) 📝 `frontend/src/__tests__/components/OIDCProviderSettings.test.tsx` (+28 -1) 📝 `frontend/src/components/OIDCProviderSettings.tsx` (+5 -2) 📝 `frontend/src/i18n/locales/de.ts` (+1 -0) 📝 `frontend/src/i18n/locales/en.ts` (+1 -0) 📝 `frontend/src/i18n/locales/fr.ts` (+1 -0) 📝 `frontend/src/i18n/locales/it.ts` (+1 -0) 📝 `frontend/src/i18n/locales/ja.ts` (+1 -0) 📝 `frontend/src/i18n/locales/pt-BR.ts` (+1 -0) 📝 `frontend/src/i18n/locales/zh-CN.ts` (+1 -0) 📝 `frontend/src/i18n/locales/zh-TW.ts` (+1 -0) </details> ### 📄 Description ## Description Relaxes the SEC-1 guard for `auto_link_existing_accounts` so that providers using a custom `email_claim` (e.g. `preferred_username`, `upn`) — the recommended Azure Entra ID configuration — can safely enable automatic account linking. Previously the guard blocked **all** combinations except `email_claim='email'` + `require_email_verified=True`. This also blocked Azure Entra ID configurations that use a custom claim, even though those are not subject to the email-verification bypass attack. The new guard only blocks the genuinely unsafe combination (Fall B): `email_claim='email'` + `require_email_verified=False`. Custom-claim paths (Fall C) never perform an `email_verified` check, so an attacker-controlled IdP cannot exploit email matching — auto-linking is safe there. Docs -> ## Related Issue Fixes maziggy/bambuddy#1088 ## Documentation - [x] Docs PR(s) linked above: infos in CHANGELOG - [ ] No docs update required — reason: internal security guard change with no new config keys or user-visible behavior change beyond unblocking a previously rejected configuration ## Type of Change - [x] Bug fix (non-breaking change that fixes an issue) - [ ] New feature (non-breaking change that adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update - [ ] Code refactoring - [ ] Performance improvement - [x] Test addition or update ## Changes Made - **`models/oidc_provider.py`**: Updated `CheckConstraint` formula from `require_email_verified = TRUE AND email_claim = 'email'` to `email_claim != 'email' OR require_email_verified = TRUE` (only blocks Fall B) - **`schemas/auth.py`**: Updated `OIDCProviderCreate` and `OIDCProviderUpdate` model validators and error message to match the new guard condition - **`api/routes/mfa.py`**: Updated `_enforce_auto_link_safety()` combined-state guard to the same Fall-B-only condition - **`core/database.py`**: Added `_migrate_update_auto_link_constraint()` to update the DB constraint on existing installations — SQLite via table recreation (shadow table pattern), PostgreSQL via `DROP CONSTRAINT IF EXISTS` + `ADD CONSTRAINT`; narrowed backfill SQL to only reset unsafe Fall B rows - **`tests/integration/test_mfa_api.py`**: Updated 4 tests and added 2 new tests covering the newly allowed Fall C scenarios and continued blocking of Fall B - **`tests/unit/test_db_dialect.py`**: Updated 2 unit tests and added 4 new tests for the new constraint formula and the SQLite migration function ## Testing - [x] I have tested this on my local machine - [ ] I have tested with my printer model:  163 relevant tests pass (`tests/integration/test_mfa_api.py` + `tests/unit/test_db_dialect.py`). Full test suite: no regressions. **Key scenarios verified:** | Scenario | email_claim | require_email_verified | auto_link | Result | |---|---|---|---|---| | Case A (standard) | `email` | `True` | `True` | ✅ Allowed | | Case B (unsafe) | `email` | `False` | `True` | ❌ Blocked (422) | | Case C (Azure) | `preferred_username` | any | `True` | ✅ Allowed | | Case C (Azure) | `upn` | `False` | `True` | ✅ Allowed | ## Additional Notes **Migration behavior:** - *New installations*: The updated `CHECK` constraint is applied at schema creation time — no action needed. - *Existing SQLite installations*: `_migrate_update_auto_link_constraint()` recreates the `oidc_providers` table with the new constraint (SQLite does not support `ALTER TABLE DROP/ADD CONSTRAINT`). The old constraint is detected via `sqlite_master`; the migration is a no-op if already on the new formula. - *Existing PostgreSQL installations*: The function issues `DROP CONSTRAINT IF EXISTS` + `ADD CONSTRAINT` — fully idempotent. - Only Fall B rows (`email_claim='email'` + `require_email_verified=False` + `auto_link=True`) are reset by the backfill; Fall C rows are left unchanged. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>