feat(auth): add ADFS SSO authorization provider#3798
Conversation
d5523e6 to
dbe9a25
Compare
|
make pylint Your code has been rated at 10.00/10 (previous run: 10.00/10, +0.00) |
|
Unit test: pytest tests/unit/mcpgateway/services/test_sso_service.py added |
|
Thanks @calculus-ask. ADFS SSO is useful for enterprise deployments. Please rebase onto main to resolve the merge conflicts. |
09b619f to
fc7f0cb
Compare
rakdutta
left a comment
There was a problem hiding this comment.
✅ APPROVED - Ready to Merge
📊 Overview
Title: feat(auth): add ADFS SSO authorization provider
Author: @calculus-ask (Santhana Krishnan)
Changes: +2,274 / -368 lines across 16 files
Related Issue: #3532
Quality Rating: ⭐⭐⭐⭐⭐ Excellent
🎯 What It Does
Adds comprehensive ADFS (Active Directory Federation Services) SSO authentication with:
- Full ADFS OAuth/OIDC integration - Extracts user info from ID token (ADFS doesn't support GET on userinfo endpoint)
- Smart email normalization - Handles Windows domain formats: user@domain.com, DOMAIN\username, plain username
- Enhanced OAuth error handling - RFC 6749 compliant with user-friendly error messages
- Provider lifecycle management - Optional auto-disable for unconfigured providers (backward compatible)
- Comprehensive testing - 97% coverage with 49 new test functions
✅ Strengths
- Security: SecretStr usage, log sanitization, JWT verification, secrets baseline updated correctly
- Code Quality: Excellent type hints, docstrings, clear separation of concerns, follows all project standards
- Testing: Integration tests (866 lines), unit tests, edge cases covered, doctests included
- Documentation: Complete tutorial with curl examples, configuration guide, troubleshooting section
- Backward Compatibility: Feature flags default to safe values, no breaking changes, no database migrations needed
- UX: Graceful OAuth error handling, clear error messages, resolves "two buttons" UI issue
- Coverage config change (parallel = true) - should mention in PR description
- Placeholder userinfo_url - ADFS doesn't use it but DB requires it (acceptable, documented)
- Verbose logging - 27 new log statements (acceptable for new feature debugging)
🔍 Technical Assessment
┌─────────────────┬────────────┬───────────────────────────────────────────┐
│ Area │ Rating │ Notes │
├─────────────────┼────────────┼───────────────────────────────────────────┤
│ Code Quality │ ⭐⭐⭐⭐⭐ │ Professional-grade implementation │
├─────────────────┼────────────┼───────────────────────────────────────────┤
│ Security │ ⭐⭐⭐⭐⭐ │ Strong, no vulnerabilities identified │
├─────────────────┼────────────┼───────────────────────────────────────────┤
│ Testing │ ⭐⭐⭐⭐⭐ │ 97% coverage, comprehensive scenarios │
├─────────────────┼────────────┼───────────────────────────────────────────┤
│ Documentation │ ⭐⭐⭐⭐⭐ │ Complete, copy-paste ready examples │
├─────────────────┼────────────┼───────────────────────────────────────────┤
│ Maintainability │ ⭐⭐⭐⭐ │ Well-isolated ADFS logic, clear structure │
└─────────────────┴────────────┴───────────────────────────────────────────┘
🚀 Recommendation
✅ APPROVE - Ready for Production
Risk Level: Low
Confidence: High
This PR is exceptionally well-executed with professional-grade implementation, comprehensive testing, and strong security practices. The ADFS email normalization logic is particularly well-designed for handling various Windows domain formats.
Once pre-commit error resolve . Ready to merge
bb873a0 to
98bb4c7
Compare
crivetimihai
left a comment
There was a problem hiding this comment.
Review: feat(auth): add ADFS SSO authorization provider
Rebased, reviewed, and cleaned up. The original 59 commits (including 9 merge commits with unrelated changes) have been squashed into a single clean commit on top of current main.
Changes made during review
Bug fixes:
- ADFS callback was unreachable in production — The OIDC id_token verification block in
handle_oauth_callback_with_tokenshard-failed for all OIDC providers when_verify_oidc_id_tokenreturnedNone. Since ADFS'sprovider_typeis"oidc", on-prem ADFS without a discoverable JWKS endpoint would fail before the ADFS-specific_get_user_infopath was ever reached. Fixed by excluding ADFS from the OIDC verification gate. - OAuth error callbacks returned 422 —
statewas a required query parameter, so if a provider returned an error withoutstate, FastAPI rejected the request before the error handler could translate it into a login redirect. Madestateoptional with explicit validation after error handling. - Bootstrap allowed incomplete ADFS config — Provider was created when only
enabled,client_id, andauthorization_urlwere set, buttoken_urlcould beNone, leaving a broken login button. Addedtoken_urlto the guard condition.
Consistency improvements:
- ADFS normalization now uses the shared
_extract_groups_and_roles()utility and_build_normalized_user_info()helper, consistent with all other SSO providers. Previously it had inline manual extraction that didn't handle string-valued groups or therolesclaim. - ADFS path in
_get_user_infonow prefers already-verified id_token claims when available (e.g., ADFS with discoverable JWKS), falling back to unverified decode only for on-prem ADFS without JWKS.
Documentation fixes (5):
- Wrong admin API path (
/admin/sso/providers→/auth/sso/admin/providers) - Login endpoint returns JSON, not a redirect — rewrote curl examples
- Response format corrected from wrapped object to bare list
- Claim priority order in troubleshooting section corrected to match code (
email > preferred_username > upn > unique_name) - Non-existent log line replaced with actual log messages
Test additions:
- 4 new OAuth error callback tests (access_denied mapping, server_error mapping, unknown error fallback, missing state rejection)
- Fixed 2 test expectations to match corrected behavior (set-based group comparison, string groups handled by shared utility)
Verification
- 327 unit tests pass
- 15 integration tests (skipped without
--with-integration) - flake8, black, isort, ruff all clean
- Coverage: sso_service.py 99%, sso.py 98%, sso_bootstrap.py 97%
9fe48da to
1ed2fde
Compare
1ed2fde to
30d857a
Compare
30d857a to
8c9f987
Compare
Add ADFS (Active Directory Federation Services) as a new SSO provider, enabling enterprise SSO login for organizations using on-prem or federated ADFS deployments. Core changes: - ADFS provider config (sso_adfs_*), bootstrap, and ID token-based user info extraction (ADFS doesn't support GET on userinfo endpoint) - UPN normalization for DOMAIN\user, plain username, and email formats - OAuth error callback handling with RFC 6749 error code mapping - Email validation in admin privilege evaluation to prevent bypass - Auto-disable feature flag for unconfigured SSO providers - ADFS tutorial documentation and comprehensive test coverage Co-authored-by: Santhana Krishnan <a.santhana.k@gmail.com> Closes IBM#3532 Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
Add ADFS (Active Directory Federation Services) as a new SSO provider, enabling enterprise SSO login for organizations using on-prem or federated ADFS deployments. Core changes: - ADFS provider config (sso_adfs_*), bootstrap, and ID token-based user info extraction (ADFS doesn't support GET on userinfo endpoint) - UPN normalization for DOMAIN\user, plain username, and email formats - OAuth error callback handling with RFC 6749 error code mapping - Email validation in admin privilege evaluation to prevent bypass - Auto-disable feature flag for unconfigured SSO providers - ADFS tutorial documentation and comprehensive test coverage Closes #3532 Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>

🔗 Related Issue
Closes #
#3532
📝 Summary
Added ADFS Authorization
Now UI Login page displays one login button "ADFS Login". Two buttons issue resolved
Doc Link
/docs/docs/sso-adfs-testing.md
🏷️ Type of Change
✅ Checklist
make black isort pre-commit)📓 Notes (optional)