starred/bambuddy

Fork 0

mirror of https://github.com/maziggy/bambuddy.git synced 2026-05-09 05:35:30 +02:00

[PR #1176] [MERGED] fix(oidc): use preferred_username/name claim for auto-created username (#1173) #1166

New issue

Closed

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

BreizhHardware commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/maziggy/bambuddy/pull/1176
Author: @netscout2001
Created: 4/30/2026
Status: ✅ Merged
Merged: 5/2/2026
Merged by: @maziggy

Base: dev ← Head: fix/oidc-autocreate-username-readable-claim

📝 Commits (7)

c09f4a2 fix(oidc): use preferred_username/name claim for auto-created username
fc7ec64 Merge branch 'dev' into fix/oidc-autocreate-username-readable-claim
85ffc45 Merge remote-tracking branch 'upstream/dev' into fix/oidc-autocreate-username-readable-claim
f5560d0 feat(oidc): configurable default group for auto-created users
7ec80c2 Merge remote-tracking branch 'origin/fix/oidc-autocreate-username-readable-claim' into fix/oidc-autocreate-username-readable-claim
e8cd605 docs: add CHANGELOG entry for OIDC username derivation and default group
d949d52 Merge branch 'dev' into fix/oidc-autocreate-username-readable-claim

📊 Changes

17 files changed (+807 additions, -10 deletions)

View changed files

📝 CHANGELOG.md (+2 -0)
📝 backend/app/api/routes/mfa.py (+47 -9)
📝 backend/app/core/database.py (+8 -0)
📝 backend/app/models/oidc_provider.py (+6 -0)
📝 backend/app/schemas/auth.py (+3 -0)
📝 backend/tests/integration/test_mfa_api.py (+603 -0)
📝 frontend/src/__tests__/components/OIDCProviderSettings.test.tsx (+77 -0)
📝 frontend/src/api/client.ts (+2 -0)
📝 frontend/src/components/OIDCProviderSettings.tsx (+35 -1)
📝 frontend/src/i18n/locales/de.ts (+3 -0)
📝 frontend/src/i18n/locales/en.ts (+3 -0)
📝 frontend/src/i18n/locales/fr.ts (+3 -0)
📝 frontend/src/i18n/locales/it.ts (+3 -0)
📝 frontend/src/i18n/locales/ja.ts (+3 -0)
📝 frontend/src/i18n/locales/pt-BR.ts (+3 -0)
📝 frontend/src/i18n/locales/zh-CN.ts (+3 -0)
📝 frontend/src/i18n/locales/zh-TW.ts (+3 -0)

📄 Description

Description

Addresses two issues from #1173:

Thread 1 — Readable usernames for OIDC auto-created users

When OIDC auto-creates a new user and no valid email claim is present (e.g. Fall C with a custom email_claim), the username was derived from the opaque provider_sub[:30] — producing unreadable usernames like 2f8a91bc04d7e3c0a512. This PR fixes the derivation chain to prefer human-readable IdP claims.

New derivation order (no email available):

preferred_username claim → sanitized
name claim → sanitized (fallback if preferred_username is absent or sanitizes to empty)
provider_sub[:30] → sanitized (last resort)

isinstance guards protect against non-string claim values (e.g. a list), and sanitization is applied per candidate so that a value that strips to empty (e.g. "!!!") correctly falls through to the next option instead of silently producing "oidcuser".

Thread 2 — Configurable default group for OIDC auto-created users

Each OIDC provider gains a default_group_id field. When set, auto-created users are placed in that group instead of always landing in Viewers. The resolution chain is: configured group → Viewers fallback → no group.

Details:

Nullable FK column on oidc_providers with ON DELETE SET NULL; migration ordered after _migrate_update_auto_link_constraint to survive the SQLite table-recreation that function performs on stale-formula databases
SQLite does not enforce FK constraints at the DB level here — a runtime lookup returning None falls through to Viewers, keeping the flow safe if the configured group is later deleted
Provider create/update validate the FK exists (422 on a non-existent group ID)
Exposed in the OIDC settings form as a group dropdown; detail view resolves the group name or shows the Viewers fallback label
i18n: 3 new keys (defaultGroup, defaultGroupDesc, defaultGroupViewersFallback) in all 8 locales

Limitation: to clear a configured default group, delete the group or select a different one — explicit reset-to-null is not currently supported (consistent with how icon_url and other nullable fields behave).

Neither the maintainer nor the contributor can reproduce this on current dev. The UsersPage edit modal disables password-reset/email controls but not groups or permissions, and email is optional in the schema. Dropped from this PR's scope — reduced to "needs reporter repro" in #1173 for a separate follow-up if concrete steps surface.

Fixes #1173 (Threads 1 & 2)

Documentation

No docs update required for Thread 1 — internal behaviour change only
User-visible change for Thread 2 — new default_group_id field on OIDC provider (form dropdown + CHANGELOG entry added)

Type of Change

Bug fix (Thread 1 — non-breaking, fixes opaque username derivation)
New feature (Thread 2 — configurable default group, backward-compatible)
Test addition or update

Changes Made

Thread 1

backend/app/api/routes/mfa.py: extend username derivation in the auto-create block (preferred_username → name → provider_sub) with per-candidate sanitization and isinstance guards
backend/tests/integration/test_mfa_api.py: add TestOIDCAutoCreateUsername with 7 integration tests

Thread 2

backend/app/models/oidc_provider.py: nullable default_group_id FK column
backend/app/schemas/auth.py: default_group_id on OIDCProviderCreate, OIDCProviderUpdate, OIDCProviderResponse
backend/app/core/database.py: idempotent ALTER TABLE migration
backend/app/api/routes/mfa.py: FK validation on create/update; resolution chain in auto-create block
frontend/src/api/client.ts: default_group_id on OIDCProvider and OIDCProviderCreate types
frontend/src/components/OIDCProviderSettings.tsx: group dropdown + detail view
frontend/src/i18n/locales/*.ts: 3 new keys in all 8 locales
backend/tests/integration/test_mfa_api.py: 5 new TestOIDCProviders cases + TestOIDCAutoCreateDefaultGroup with 4 cases
frontend/src/__tests__/components/OIDCProviderSettings.test.tsx: 6 new tests
CHANGELOG.md: entry for both improvements including limitation note

Screenshots

N/A — backend-only for Thread 1; Thread 2 adds a group dropdown to the existing OIDC provider form (admin-only settings page)

Testing

Thread 1 — 7 integration tests in TestOIDCAutoCreateUsername:

Test	Claims	Expected username
`test_preferred_username_used_when_no_email`	`preferred_username='johndoe'`	`johndoe`
`test_preferred_username_spaces_sanitized`	`preferred_username='John Doe'`	`JohnDoe`
`test_name_claim_used_when_no_preferred_username`	`name='Jane Smith'`	`JaneSmith`
`test_provider_sub_fallback_when_no_claims`	sub only	`abc123xyz`
`test_non_string_preferred_username_falls_through_to_name`	`preferred_username=["listval"]`, `name='BobJones'`	`BobJones`
`test_preferred_username_sanitizes_to_empty_falls_through_to_name`	`preferred_username='!!!'`, `name='bob'`	`bob`
`test_username_collision_appends_counter`	pre-existing `collider` user	`collider1`

Thread 2 — 9 backend + 6 frontend tests:

Test	Scenario
`test_create_provider_with_default_group_id`	valid group ID accepted, returned in response
`test_create_provider_invalid_default_group_id_returns_422`	non-existent group ID → 422
`test_create_provider_omit_default_group_id_stores_null`	omitting field → `null`
`test_update_provider_default_group_id`	PATCH updates the field
`test_default_group_id_in_public_and_admin_list`	field present in both list endpoints
`test_configured_group_assigned_to_auto_created_user`	auto-created user lands in configured group
`test_null_default_group_id_falls_back_to_viewers`	`null` → Viewers fallback
`test_dangling_default_group_id_falls_back_to_viewers`	deleted group → Viewers fallback (SQLite FK not enforced)
`test_administrators_group_can_be_set_as_default`	Administrators group accepted

python3 -m pytest tests/integration/test_mfa_api.py::TestOIDCAutoCreateUsername \
                  tests/integration/test_mfa_api.py::TestOIDCProviders \
                  tests/integration/test_mfa_api.py::TestOIDCAutoCreateDefaultGroup -v
# 16 passed

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

Checklist

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

Additional Notes

Thread 1 fix is limited to the auto-create path (provider.auto_create_users = True). The auto-link path (matching existing users by provider_sub or email) is unaffected.

Thread 2: default_group_id follows the existing exclude_none=True PATCH semantics — a PATCH that omits the field leaves the current value unchanged. This is consistent with icon_url and the other nullable provider fields.

_{🔄 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/1176 **Author:** [@netscout2001](https://github.com/netscout2001) **Created:** 4/30/2026 **Status:** ✅ Merged **Merged:** 5/2/2026 **Merged by:** [@maziggy](https://github.com/maziggy) **Base:** `dev` ← **Head:** `fix/oidc-autocreate-username-readable-claim` --- ### 📝 Commits (7) - [`c09f4a2`](https://github.com/maziggy/bambuddy/commit/c09f4a27bef85fe71839d9adf06796562476ea2b) fix(oidc): use preferred_username/name claim for auto-created username - [`fc7ec64`](https://github.com/maziggy/bambuddy/commit/fc7ec648365427481cb08fe3dcf4b5bb8e07dc17) Merge branch 'dev' into fix/oidc-autocreate-username-readable-claim - [`85ffc45`](https://github.com/maziggy/bambuddy/commit/85ffc450b56c1849a95e53dd9bcd1f379283d37f) Merge remote-tracking branch 'upstream/dev' into fix/oidc-autocreate-username-readable-claim - [`f5560d0`](https://github.com/maziggy/bambuddy/commit/f5560d0dcb5aef1f59e9cbcfa6c146ec4776f072) feat(oidc): configurable default group for auto-created users - [`7ec80c2`](https://github.com/maziggy/bambuddy/commit/7ec80c2830649eccdf6e94c3199786cab04ea7f7) Merge remote-tracking branch 'origin/fix/oidc-autocreate-username-readable-claim' into fix/oidc-autocreate-username-readable-claim - [`e8cd605`](https://github.com/maziggy/bambuddy/commit/e8cd6059866952707fbe3c6489c92c26ddfbb2cd) docs: add CHANGELOG entry for OIDC username derivation and default group - [`d949d52`](https://github.com/maziggy/bambuddy/commit/d949d52dff645433e45bd68ed6c4c470c3f13f29) Merge branch 'dev' into fix/oidc-autocreate-username-readable-claim ### 📊 Changes **17 files changed** (+807 additions, -10 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+2 -0) 📝 `backend/app/api/routes/mfa.py` (+47 -9) 📝 `backend/app/core/database.py` (+8 -0) 📝 `backend/app/models/oidc_provider.py` (+6 -0) 📝 `backend/app/schemas/auth.py` (+3 -0) 📝 `backend/tests/integration/test_mfa_api.py` (+603 -0) 📝 `frontend/src/__tests__/components/OIDCProviderSettings.test.tsx` (+77 -0) 📝 `frontend/src/api/client.ts` (+2 -0) 📝 `frontend/src/components/OIDCProviderSettings.tsx` (+35 -1) 📝 `frontend/src/i18n/locales/de.ts` (+3 -0) 📝 `frontend/src/i18n/locales/en.ts` (+3 -0) 📝 `frontend/src/i18n/locales/fr.ts` (+3 -0) 📝 `frontend/src/i18n/locales/it.ts` (+3 -0) 📝 `frontend/src/i18n/locales/ja.ts` (+3 -0) 📝 `frontend/src/i18n/locales/pt-BR.ts` (+3 -0) 📝 `frontend/src/i18n/locales/zh-CN.ts` (+3 -0) 📝 `frontend/src/i18n/locales/zh-TW.ts` (+3 -0) </details> ### 📄 Description ## Description Addresses two issues from #1173: ### Thread 1 — Readable usernames for OIDC auto-created users When OIDC auto-creates a new user and no valid email claim is present (e.g. Fall C with a custom `email_claim`), the username was derived from the opaque `provider_sub[:30]` — producing unreadable usernames like `2f8a91bc04d7e3c0a512`. This PR fixes the derivation chain to prefer human-readable IdP claims. **New derivation order (no email available):** 1. `preferred_username` claim → sanitized 2. `name` claim → sanitized (fallback if `preferred_username` is absent or sanitizes to empty) 3. `provider_sub[:30]` → sanitized (last resort) `isinstance` guards protect against non-string claim values (e.g. a list), and sanitization is applied per candidate so that a value that strips to empty (e.g. `"!!!"`) correctly falls through to the next option instead of silently producing `"oidcuser"`. ### Thread 2 — Configurable default group for OIDC auto-created users Each OIDC provider gains a `default_group_id` field. When set, auto-created users are placed in that group instead of always landing in Viewers. The resolution chain is: configured group → Viewers fallback → no group. **Details:** - Nullable FK column on `oidc_providers` with `ON DELETE SET NULL`; migration ordered after `_migrate_update_auto_link_constraint` to survive the SQLite table-recreation that function performs on stale-formula databases - SQLite does not enforce FK constraints at the DB level here — a runtime lookup returning `None` falls through to Viewers, keeping the flow safe if the configured group is later deleted - Provider create/update validate the FK exists (422 on a non-existent group ID) - Exposed in the OIDC settings form as a group dropdown; detail view resolves the group name or shows the Viewers fallback label - i18n: 3 new keys (`defaultGroup`, `defaultGroupDesc`, `defaultGroupViewersFallback`) in all 8 locales **Limitation:** to clear a configured default group, delete the group or select a different one — explicit reset-to-null is not currently supported (consistent with how `icon_url` and other nullable fields behave). ### Thread 3 — Email-gating in UsersPage edit modal Neither the maintainer nor the contributor can reproduce this on current `dev`. The UsersPage edit modal disables password-reset/email controls but not groups or permissions, and email is optional in the schema. **Dropped from this PR's scope** — reduced to "needs reporter repro" in #1173 for a separate follow-up if concrete steps surface. ## Related Issue Fixes #1173 (Threads 1 & 2) ## Documentation - [x] No docs update required for Thread 1 — internal behaviour change only - [x] User-visible change for Thread 2 — new `default_group_id` field on OIDC provider (form dropdown + CHANGELOG entry added) ## Type of Change - [x] Bug fix (Thread 1 — non-breaking, fixes opaque username derivation) - [x] New feature (Thread 2 — configurable default group, backward-compatible) - [x] Test addition or update ## Changes Made **Thread 1** - `backend/app/api/routes/mfa.py`: extend username derivation in the auto-create block (`preferred_username` → `name` → `provider_sub`) with per-candidate sanitization and `isinstance` guards - `backend/tests/integration/test_mfa_api.py`: add `TestOIDCAutoCreateUsername` with 7 integration tests **Thread 2** - `backend/app/models/oidc_provider.py`: nullable `default_group_id` FK column - `backend/app/schemas/auth.py`: `default_group_id` on `OIDCProviderCreate`, `OIDCProviderUpdate`, `OIDCProviderResponse` - `backend/app/core/database.py`: idempotent `ALTER TABLE` migration - `backend/app/api/routes/mfa.py`: FK validation on create/update; resolution chain in auto-create block - `frontend/src/api/client.ts`: `default_group_id` on `OIDCProvider` and `OIDCProviderCreate` types - `frontend/src/components/OIDCProviderSettings.tsx`: group dropdown + detail view - `frontend/src/i18n/locales/*.ts`: 3 new keys in all 8 locales - `backend/tests/integration/test_mfa_api.py`: 5 new `TestOIDCProviders` cases + `TestOIDCAutoCreateDefaultGroup` with 4 cases - `frontend/src/__tests__/components/OIDCProviderSettings.test.tsx`: 6 new tests - `CHANGELOG.md`: entry for both improvements including limitation note ## Screenshots N/A — backend-only for Thread 1; Thread 2 adds a group dropdown to the existing OIDC provider form (admin-only settings page) ## Testing **Thread 1 — 7 integration tests in `TestOIDCAutoCreateUsername`:** | Test | Claims | Expected username | |---|---|---| | `test_preferred_username_used_when_no_email` | `preferred_username='johndoe'` | `johndoe` | | `test_preferred_username_spaces_sanitized` | `preferred_username='John Doe'` | `JohnDoe` | | `test_name_claim_used_when_no_preferred_username` | `name='Jane Smith'` | `JaneSmith` | | `test_provider_sub_fallback_when_no_claims` | sub only | `abc123xyz` | | `test_non_string_preferred_username_falls_through_to_name` | `preferred_username=["listval"]`, `name='BobJones'` | `BobJones` | | `test_preferred_username_sanitizes_to_empty_falls_through_to_name` | `preferred_username='!!!'`, `name='bob'` | `bob` | | `test_username_collision_appends_counter` | pre-existing `collider` user | `collider1` | **Thread 2 — 9 backend + 6 frontend tests:** | Test | Scenario | |---|---| | `test_create_provider_with_default_group_id` | valid group ID accepted, returned in response | | `test_create_provider_invalid_default_group_id_returns_422` | non-existent group ID → 422 | | `test_create_provider_omit_default_group_id_stores_null` | omitting field → `null` | | `test_update_provider_default_group_id` | PATCH updates the field | | `test_default_group_id_in_public_and_admin_list` | field present in both list endpoints | | `test_configured_group_assigned_to_auto_created_user` | auto-created user lands in configured group | | `test_null_default_group_id_falls_back_to_viewers` | `null` → Viewers fallback | | `test_dangling_default_group_id_falls_back_to_viewers` | deleted group → Viewers fallback (SQLite FK not enforced) | | `test_administrators_group_can_be_set_as_default` | Administrators group accepted | ```bash python3 -m pytest tests/integration/test_mfa_api.py::TestOIDCAutoCreateUsername \ tests/integration/test_mfa_api.py::TestOIDCProviders \ tests/integration/test_mfa_api.py::TestOIDCAutoCreateDefaultGroup -v # 16 passed ``` - [x] I have tested this on my local machine - [ ] I have tested with my printer model:  ## 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 Thread 1 fix is limited to the auto-create path (`provider.auto_create_users = True`). The auto-link path (matching existing users by `provider_sub` or email) is unaffected. Thread 2: `default_group_id` follows the existing `exclude_none=True` PATCH semantics — a PATCH that omits the field leaves the current value unchanged. This is consistent with `icon_url` and the other nullable provider fields. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>