Add OIDC authentication support with multi-provider configuration #691
Conversation
- Add OIDC authentication endpoints (login, callback, config) - Implement flexible role mapping from OIDC groups/roles to Gramps roles - Add configurable OIDC scopes and role claim sources - Support environment-driven configuration with GRAMPSWEB_ prefix - Add fallback to guest role when no role claims found - Include option to disable local authentication when OIDC enabled - Add auto-redirect functionality for OIDC-only deployments - Update token endpoint to respect OIDC authentication settings - Add comprehensive test suite for OIDC functionality - Add Authlib dependency for OpenID Connect support
… management, security improvements - Add OIDC_USERNAME_CLAIM config for customizable username extraction - Implement optional role mapping: preserve existing roles when no OIDC_GROUP_* env vars set - Remove client_id exposure from public OIDC config endpoint - Upgrade Authlib dependency to 1.6.4 for security fixes and improvements
- Add support for Google, Microsoft, and GitHub built-in providers
- Auto-detect available providers from environment variables pattern OIDC_{PROVIDER}_CLIENT_ID
- Remove dependency on primary provider, make all providers truly optional
- Add provider-specific endpoints with validation and routing
- Implement username prefixing to prevent conflicts between providers
- Update configuration API to return list of available providers
- Handle GitHub OAuth 2.0 flow alongside standard OIDC providers
- Smart auto-redirect only when single provider is configured
The gunicorn package is not required for OIDC functionality. The project already uses waitress as its production WSGI server. The wsgi.py file supports gunicorn logging if available, but doesn't require it as a dependency.
- Add app parameter to get_available_oidc_providers() function - Add app parameter to get_provider_config() function - Use Flask app config instead of os.getenv() for configuration access - Update init_oidc() to pass app instance to provider functions - Fixes RuntimeError when accessing current_app during app initialization This resolves the 'Working outside of application context' errors and enables proper OIDC provider detection during Flask startup.
|
Thanks a lot! 🥳 Will review ASAP. Fixes #355 |
|
Noticed some |
- Allow setting custom display name via GRAMPSWEB_OIDC_NAME environment variable - Fallback to 'OIDC' if no custom name is provided - Add OIDC configuration documentation to README
|
Further investigation with OIDC Provider alternatives to keycloak revealed some issues. Authentik does not seem to want to cooperate in the token exchange with the current setup. Investigating. |
|
I'm halfway through with reviewing, overall looks really good. Eventually you'll also need to update |
|
Here are some general questions, partially due to my lack of deep knowledge about OIDC. Here are some architectural questions. User creation Users are identified purely via username. Doesn't this mean that, in principle, users could take over an existing account? By attaching the provider ID to the username, this is admittedly made unlikely, but it's not used for the custom provider, and it also makes the usernames quite ugly. In contrast, the e-mail is not used for account matching, although I would expect it to be much harder to compromise, as OIDC providers hopefully validate their users' email addresses. If I already have a "local" account on my Gramps Web server using a GMail address and decide to log in with Google instead, wouldn't it make sense to associate the account? (Admittedly, in the case of Google it's particularly complicated as it accepts several forms of the address, with or without dots, etc.). Somehow, I'm puzzled by the fact that we are not storing anything about the OIDC account association in the user database. Tokens Handing the tokens to the frontend is tricky, I'm aware. If I understand correctly, you chose a URL fragment instead of a query so it doesn't appear in the server log. Right? Isn't there any other way that avoids putting the tokens into the URL completely? Role mapping Finally, I don't understand the part about role mapping, can you please explain the idea? |
Removes OIDC configuration documentation from README.md as it should be documented in the main gramps-web-docs repository instead.
Adds OpenAPI documentation for the new OIDC authentication endpoints: - /oidc/config: Get OIDC configuration and available providers - /oidc/login/: Initiate OIDC login flow with provider - /oidc/callback/: Handle OIDC callback and create JWT tokens Includes schema definitions for OIDCConfig and OIDCProvider.
|
Awesome, thank you very much for your review! I'll address the obvious cases you mentioned above and get maybe get back to you in case I have questions. As to your general questions: username-based mapping and no OIDC account association stored in the user database.Your concerns are valid. I must admit I went with a quick implementation rather than a pretty one. From experience with other projects, a simple username mapping was usually the approach chosen. With self-hosted OIDC providers you can usually fix usernames, so impersonation would not be an issue in such cases. That's for example the approach I have chosen with my own Authentik setup as othe OIDC-enabled services would otherwise break. While email would be a quick fix, I think going with the I'll implement a fix which adds the OIDC provider_id and Auto email-assigned mapping of local users to OIDC ones would be cool as well. In the case of Google I think we could have a special handling case to normalize any stored emails to their "canonical" form before comparing against the OIDC-provided provided email for mapping, this should remove the issue you mentioned. In sumamary, the new auth flow would be changed to something like this:
TokensHahaha thanks for acknowledging that it's difficult, indeed I struggled a bit here. Role-mappingOIDC-provider usually have two common approaches to set permissions: groups and roles. Once logged in, we have access to a list of roles/groups the user has/is a part of. This implementation can handle both, where From this, we map this to the gramps roles, going by highest role possible. E.g. if a user is part of our designated admins group and users group, give him the Admin role in gramps. The explicit group/role names which we define in our OIDC-provider can be configured using the This does not work with Providers where you cannot define these groups/roles, so this is purely for custom OIDC providers. Could you clarify what your question here is? I've seen this approach to auto-provisioning of roles in other projects as well and it seems like the cleanest to me personally. Many thanks for your time in reviewing this, some things are obvious after you have pointed it out so thanks for bearing with me on them. |
- Fixed copyright headers to show only Alexander Bocken as author - Changed HTTP status codes from 404 to 405 for disabled OIDC endpoints - Updated error logging to use logger.exception() for better stack traces - Fixed module docstring from "blueprint" to "resources" - Moved current_app imports to top-level instead of local imports - Removed GitHub email fallback that generated fake noreply addresses - Updated log message to reference config options instead of env vars - Implemented FRONTEND_URL with BASE_URL fallback using get_config() - Added FRONTEND_URL to devcontainer environment for development - Added FRONTEND_URL to allowed database config keys - Removed default parameters from create_or_update_oidc_user function
|
Great work, thanks for your efforts, I think we are close. But I think I found one more security issue. If a user exists with email address A and tree X, they can sing up for tree Y with an OIDC account that has the same email address, and the code will update the existing user to use the new tree, but preserving the role. So, say, any tree owner on a given multi tree server could escalate to become tree owner of any other tree. Although we could close this whole with appropriate checks, I think we should remove the email matching feature from the scope of this PR to keep it simpler. Can you please remove it? We can always add it back later. Sorry for suggesting that initially. Apart from that, two questions:
|
Key changes: • Align OIDC user experience with local registration flow • Remove email matching feature to prevent privilege escalation across trees • Remove unnecessary last_login field from oidc_accounts table • Modernize Flask response handling (remove make_response wrapper) • Fix security vulnerability: ROLE_DISABLED users now get confirmation page instead of tokens • Improve code organization: move all imports to top-level • Extract development environment detection to utility function • Remove sensitive information from logging (no longer log all OIDC claims) • Use current_app.debug instead of environment variable checking • Simplify database schema by removing unused tracking fields Security improvements: • Prevent cross-tree privilege escalation via email matching • Default new OIDC users to ROLE_DISABLED requiring admin approval • Block token generation for disabled accounts • Remove potential PII exposure in debug logs Code quality improvements: • Consolidate duplicate environment detection logic • Organize imports consistently across OIDC modules • Remove unused functions and database fields • Modernize Flask patterns and response handling
|
Oh wow, good catch with the email matching privilege escalation! Apologies, I did not consider the multi-tree scenario when setting this up. Removed. Happy to create a PR in the future to re-introduce it in a cleaner way that prevents this issue.
Besides these points I have also just pushed a commit concerning your remaining points. |
tests/test_oidc_auth.py
Outdated
| self.assertEqual(kwargs["name"], case["expected_username"]) | ||
| self.assertEqual(kwargs["role"], case["expected_role"]) | ||
|
|
||
| if __name__ == "__main__": |
There was a problem hiding this comment.
We don't need this as unit tests are run with pytest
|
I think that was my last round of small comments 😊 But CI is failing with an import error. |
Co-authored-by: David Straub <[email protected]>
|
I have not kept pace with the unittests tbh. I'll see that I can cover the remaining topics until end of week:
|
Replace hardcoded "custom" string with PROVIDER_CUSTOM constant for better maintainability and consistency throughout the OIDC authentication module.
- Convert test_oidc_auth.py from unittest to pytest style - Remove if __name__ == "__main__" blocks (not needed for pytest) - Fix outdated function reference: get_role_from_groups -> get_role_from_claims - Add comprehensive test coverage for new OIDC features: * Multi-provider support (Google, Microsoft, GitHub, custom) * get_available_oidc_providers() function * get_provider_config() function * PROVIDER_CUSTOM constant usage * Custom and nested role claim support (e.g., realm_access.roles) * OIDC account table lookups via sub claim mapping * Username generation logic for different providers * Username conflict resolution with counter suffix * Role preservation when no mapping configured - Improve test organization with dedicated test classes per function - Add error handling tests for missing sub claim and invalid providers
- Create oidc_helpers.py to break circular import chain * Moved is_oidc_enabled() to separate module with no api imports * Updated token.py, user.py, and oidc.py to import from oidc_helpers - Use lazy imports in oidc.py for api module functions * Import run_task, send_email_new_user, get_tree_id inside function * Added comment explaining this intentional exception to top-level import standard - Fix pre-existing syntax errors in oidc.py callback validation * Added missing if-statement body for invalid tree check * Fixed invalid '!tree' syntax to 'not tree' - Update OIDC unit tests to work without Flask app context * Mock current_app with context managers instead of decorators * Mock get_tree_id and run_task to avoid database/task dependencies * Remove run_task patches (no longer at module level) * Fix test parameter ordering after removing current_app patches All 34 OIDC unit tests now passing successfully.
Change default from True to False to match expected behavior where users should explicitly opt-in to auto-redirecting to OIDC login.
This commit implements comprehensive OIDC logout functionality including frontend SSO logout and OpenID Connect Back-Channel Logout support. Backend changes: - Add token blocklist system for JWT revocation (token_blocklist.py) - Add /api/oidc/logout/ endpoint to retrieve provider logout URLs - Add /api/oidc/backchannel-logout/ endpoint for OIDC Back-Channel Logout - Include oidc_provider claim in JWT tokens to track auth method - Integrate token blocklist with JWT manager for revocation checks - Fix OIDC_AUTO_REDIRECT default to False API changes: - Update apispec.yaml with new logout endpoint documentation - Add proper error handling and graceful degradation Tests: - Add 17 comprehensive tests for logout functionality - Test OIDC logout endpoint (disabled, invalid provider, missing client) - Test backchannel logout validation (all OIDC spec requirements) - Test token blocklist operations and cleanup - Test OIDC provider claim in tokens - All tests passing (17/17)
Fixed two issues preventing OIDC authentication from working: 1. Fixed server_metadata_url construction: When OIDC_OPENID_CONFIG_URL is not set, it now properly falls back to constructing the URL from the issuer. The previous .get() with default was not working because None is a valid value that prevented the fallback. 2. Added explicit metadata loading at startup: Call load_server_metadata() immediately after registration to ensure the authorize_url and other endpoints are available when needed, preventing 'Missing authorize_url' runtime errors.
|
After a last review I think we should now be mergable. I have tested the setup with Authentik as well, found a nasty issue that was finally solved in c962faf. Feel free to have a look at https://github.com/AlexBocken/gramps-oidc-testing for the Keycloak and Authentik setups in action. The PRs in gramps-web and gramps-web-docs have been updated as well. |
|
Getting lots of unit test fails for the Python 3.9 build, even outside of my added tests. Should this be the case? |
- Import get_tokens from correct module (api.resources.token) - Patch is_oidc_enabled at usage locations, not definition - Use app.config instead of environment variables for runtime config - Fix expected status codes (405 for disabled OIDC endpoints) - Fix error response JSON structure assertions
|
I had the same problem and solved it by unpinning sentence transformers, it should work again after you've rebased on |
|
rebased, thanks for the hint! |
|
Can confirm that with the rebase the tests seem to run smoothly both for 3.9 and 3.13, at least locally. Let's hope the same also happens once you run them via Action 😄 |
Adds comprehensive OIDC authentication support to Gramps Web API with multi-provider configuration and automatic role mapping.
Features
Configuration
All OIDC settings use GRAMPSWEB_OIDC_* environment variables for consistency. Role mapping automatically assigns permissions based on group membership from OIDC tokens.
Testing
Complete testing environment available at: https://github.com/AlexBocken/gramps-oidc-testing
There are some unit-tests included but to be honest, these still need improving.
Misc
Happy to improve the codebase based on feedback, I tried to stick to utility functions and similar coding style to not waste effort.
Related: Requires companion frontend PR for full OIDC integration. (PR [gramps-web/#763)(https://github.com/gramps-project/gramps-web/pull/763)