fix(ui_sso): resolve highest privilege Entra app role, not first in claim - #36728
Conversation
…laim A user assigned more than one Entra app role — commonly by belonging to several assigned groups — arrives at the Microsoft SSO callback with every role in the id_token `roles` claim. LiteLLM stores a single role per user, and get_microsoft_callback_response collapsed the list by taking the first value that resolved to a LitellmUserRoles and breaking. Entra does not guarantee the ordering of the `roles` claim, so which role won was effectively arbitrary: a user in one group mapped to internal_user and another mapped to proxy_admin_viewer could be silently demoted to internal_user, and proxy_admin could lose to either. The generic/Okta path already resolves this correctly via determine_role_from_groups, which walks a documented privilege hierarchy. Hoist that hierarchy into LITELLM_USER_ROLE_HIERARCHY and reuse it, so app-role logins and group-mapping logins agree. Extract the selection into MicrosoftSSOHandler.get_user_role_from_app_roles so it is directly testable — the existing tests re-implemented the loop inline, which is why the ordering bug was not caught. Behaviour is unchanged for single-role claims, unrecognised values, and empty claims. Roles the hierarchy does not rank (org_admin, team, customer) are resolved deterministically rather than by claim order.
Greptile SummaryThe PR makes Microsoft SSO app-role resolution independent of Entra claim ordering by selecting the highest recognized role through the existing hierarchy.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| litellm/proxy/management_endpoints/ui_sso.py | Centralizes the role hierarchy and uses it to resolve Microsoft SSO app roles deterministically; the previously reported commentary issue is resolved. |
| tests/test_litellm/proxy/management_endpoints/test_entraid_app_roles.py | Expands focused unit coverage for app-role extraction and deterministic privilege resolution. |
Reviews (2): Last reviewed commit: "Merge branch 'BerriAI:litellm_internal_s..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…ions Addresses review feedback on the app role selection helper. Drop the explanatory comments and the Args/Returns docstring boilerplate that restated the control flow, keeping only the part a reader cannot infer from the code: that Entra does not guarantee claim ordering, and how unranked roles resolve. Type the parameter as Sequence[str] rather than list[str] and build the resolved set as a frozenset, so the helper stops adding an LIT001 mutable-collection annotation. Make LITELLM_USER_ROLE_HIERARCHY a tuple for the same reason. No behaviour change: the ordering regression tests still fail against the previous first-match-wins logic and pass here.
|
The one remaining red check, Nothing in this PR touches the interactions surface or the OpenAPI spec, so I've left it alone rather than papering over it here |
…les-highest-privilege
|
@greptileai review latest head |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 2002fee. Configure here.
02dcc4d
into
BerriAI:litellm_internal_staging
|
Thanks for the contribution. LGTM |
TLDR
Problem this solves:
rolesclaim orderingproxy_admincan silently lose tointernal_userHow it solves it:
User Flow
Before: a platform admin who is also a member of a regular team group signs in and lands with read-only access, unable to administer the proxy
proxy_adminandinternal_user, and assigns one group to eachhttps://litellm-domain/uiand completes the Microsoft sign-inproxy_admininstead, because the role that wins depends on the order Entra listed the roles inAfter: the same user signs in and lands with the higher privilege role, keeping their team membership
https://litellm-domain/uiand completes the Microsoft sign-inRelevant issues
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Screenshots / Proof of Fix
Before, against a live proxy on v1.95.0
This reproduces on a real tenant, not a constructed claim. The user below is a member of two Entra groups assigned to this app, one mapped to the
internal_userapp role and one mapped toproxy_admin_viewer. Hostname, tenant, app id and email are redacted; the role values are verbatim.Minting a real Entra access token for the app and decoding its
rolesclaim shows Entra listinginternal_userfirst:The running proxy resolved that user to the lower privilege role, dropping
proxy_admin_viewer:That is the bug end to end: the user holds
proxy_admin_viewerin Entra, the token carries it, and the proxy storedinternal_userpurely because Entra happened to list it first.After, same claim through the patched resolver
At f7d1524, the real claim above resolves the other way:
Remaining half, the interactive sign-in
The sign-in itself needs a browser, so I cannot capture it headlessly. Steps to reproduce both halves against a local proxy, if you want the UI screenshots:
http://localhost:4000/sso/callbackto the app registration's redirect URIsinternal_userandproxy_admin_viewer, via two groups or directlyMICROSOFT_CLIENT_ID,MICROSOFT_CLIENT_SECRET,MICROSOFT_TENANTandPROXY_BASE_URL=http://localhost:4000python litellm/proxy/proxy_cli.py --config litellm/proxy/dev_config.yaml --detailed_debug --reload 2>&1 | tee litellm.logcurl -sS localhost:4000/user/info -H "Authorization: Bearer $TOKEN"reportsinternal_user/user/inforeportingproxy_admin_viewerType
🐛 Bug Fix
Caveats (if any)
org_admin,team,customerare unranked by the existing hierarchyorg_adminshould be rankedFinal Attestation
Note
Medium Risk
Changes how Microsoft SSO persists user roles at login, which directly affects admin UI access; behavior is narrowed and well-tested but any mis-ranking of unlisted roles like org_admin could surprise tenants.
Overview
Fixes Microsoft SSO assigning the first mapped Entra app role in the token instead of the highest privilege role when a user has several app roles—Entra does not guarantee
roles/app_rolesclaim order, so admins could land asinternal_userdespite also holdingproxy_adminorproxy_admin_viewer.LITELLM_USER_ROLE_HIERARCHYis introduced as a shared ordering (proxy admin → admin viewer → internal user → internal viewer) and wired intodetermine_role_from_groupsso group-based SSO uses the same list. Microsoft callback logic now callsMicrosoftSSOHandler.get_user_role_from_app_roles, which maps all claim values viaget_litellm_user_role, picks the top ranked role in that hierarchy, and falls back to a deterministic choice by role name for unranked roles (org_admin, etc.). Unmapped roles are ignored; empty or unresolvable claims still returnNoneso existing defaults apply.Tests in
test_entraid_app_roles.pycover single roles, claim order permutations, unknown roles, and the id_token → role path (including therolesclaim).Reviewed by Cursor Bugbot for commit 2002fee. Bugbot is set up for automated code reviews on this repo. Configure here.