feat(spp_oauth): prepare spp_oauth for stable status with RS256 bridge module#114
feat(spp_oauth): prepare spp_oauth for stable status with RS256 bridge module#114gonzalesedwin1123 wants to merge 9 commits into
Conversation
…rage Empty default config parameter values so get_private_key()/get_public_key() raise clear OpenSPPOAuthJWTException when keys are not configured, instead of failing with a cryptic PyJWT error on the placeholder strings. Also: - Export get_private_key/get_public_key from tools __init__ - Remove unused MOCK_PRIVATE_KEY constant and deprecated default_backend() - Move inline imports to top of test_rsa_encode_decode.py - Add test_res_config_settings.py covering settings model - Add test_exception_logs_error asserting _logger.error is called - Add QA testing guide (USAGE.md) with 8 test scenarios - Update DESCRIPTION.md with missing functions and cryptography dep
Bridge module that auto-installs when both spp_api_v2 and spp_oauth are present, enabling RS256 JWT authentication for the API alongside the existing HS256 flow. - Override FastAPI auth dependency to route by JWT header alg field - Add /oauth/token/rs256 endpoint for RS256 token generation - Explicit algorithm allowlist (RS256/HS256 only) to prevent confusion attacks - 22 tests covering RS256 auth, HS256 regression, and token generation
…indings Rename abbreviated field/param names to full forms per naming conventions: - oauth_priv_key → oauth_private_key - oauth_pub_key → oauth_public_key across models, config params, views, data, tests, and docs. spp_oauth changes: - Category: "OpenSPP" → "OpenSPP/Integration" - ACL: restrict to base.group_system with perm_create=1 - Remove ERROR logging from exception constructor (let callers decide) - Rename model class RegistryConfig → OAuthConfig - Add wrong-key verification test - Improve test assertions (assertIsNotNone) and remove numbered prefixes - Fix stale doc references to removed logging behavior spp_api_v2_oauth changes: - Remove empty ir.model.access.csv (no new models) - Add RS256 endpoint to API auth allowlist - Fix nosemgrep annotation for multiline sudo() - Remove Python <3.11 compat guard (Odoo 19 requires 3.11+) - Import constants from ..constants instead of redeclaring - Replace silent if-guards with skipTest() for endpoint tests - Safe int() cast for token_lifetime_hours config - asyncio.get_event_loop() → asyncio.run() - Manual key restore → addCleanup for test safety - SimpleNamespace for mock credentials
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the OpenSPP API's authentication capabilities by introducing support for RS256 JSON Web Tokens (JWTs) through a new bridge module. It also refines the existing OAuth module, improving its maintainability, security, and user experience. The changes aim to provide more robust and flexible authentication options, particularly beneficial for distributed and zero-trust architectures, while ensuring backward compatibility for existing HS256 clients. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements by adding RS256 JWT authentication support through a new spp_api_v2_oauth bridge module, while also improving the existing spp_oauth module. The changes are well-structured, with comprehensive test coverage for both new functionality and regression testing for existing HS256 authentication. Key improvements include stricter access controls on OAuth key configuration, clearer naming conventions, and better error handling. The documentation has also been thoroughly updated. My review focuses on a couple of opportunities to reduce code duplication to further enhance long-term maintainability. Overall, this is an excellent contribution that strengthens the platform's authentication capabilities.
I am having trouble creating individual review comments. Click here to see my feedback.
spp_api_v2_oauth/constants.py (7-8)
The constants JWT_AUDIENCE and JWT_ISSUER are also used in the spp_api_v2 module, where they are currently hardcoded. While defining them as constants here is an improvement, it introduces duplication between modules. To maintain a single source of truth, consider defining these constants in a shared location, perhaps within the spp_api_v2 module itself (since spp_api_v2_oauth depends on it), and importing them here. This would make future updates to these values safer and more maintainable.
spp_api_v2_oauth/routers/oauth_rs256.py (73-81)
The logic to retrieve and parse the token_lifetime_hours configuration parameter, including the try-except block and default value handling, is duplicated from the original HS256 token endpoint in spp_api_v2. To improve maintainability and avoid potential inconsistencies, this logic could be extracted into a shared utility function that both token endpoints can call.
spp_oauth/tools/oauth_exception.py (1-9)
Removing the automatic error logging from the exception's constructor is a good design choice. It correctly places the responsibility of logging on the caller, allowing for more context-specific log levels and messages. However, the exception message itself could be more informative for the developer who needs to debug it. Consider including the original error message when wrapping other exceptions.
class OpenSPPOAuthJWTException(Exception):
"""Raised when an OAuth JWT operation fails (missing keys, invalid tokens, etc.)."""
pass
spp_oauth/tools/rsa_encode_decode.py (48-49)
The variable name privkey is an abbreviation. For consistency with the other changes in this PR (e.g., oauth_private_key), it would be clearer to use the full name private_key.
private_key = get_private_key(env)
return jwt.encode(headers=header, payload=payload, key=private_key, algorithm=JWT_ALGORITHM)
spp_oauth/tools/rsa_encode_decode.py (61-63)
The variable name pubkey is an abbreviation. For consistency with other variable names and to improve clarity, it's better to use the full name public_key.
public_key = get_public_key(env)
try:
return jwt.decode(access_token, key=public_key, algorithms=[JWT_ALGORITHM])
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 19.0 #114 +/- ##
==========================================
+ Coverage 71.70% 71.81% +0.10%
==========================================
Files 942 953 +11
Lines 55563 55852 +289
==========================================
+ Hits 39844 40112 +268
- Misses 15719 15740 +21
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…l-disclosure warnings Semgrep's python-logger-credential-disclosure rule flags log strings containing "token" as potential secret leaks. Reword messages to avoid the keyword while preserving log clarity.
Response to Gemini Code ReviewThanks for the review. Here's the status of each finding: ✅ Already FixedFinding #4 ( ℹ️ No Action NeededFinding #3 (exception class 📋 Valid But Out of ScopeFinding #1 (duplicated Fixing this properly requires modifying These would be good follow-up items for a |
…nal issuers Adds the spp.oauth.issuer model and refactors the bridge to route RS256 tokens by the `iss` claim. Tokens whose iss matches the internal "openspp-api-v2" value keep using the spp_oauth key (unchanged behavior); tokens from any registered active issuer record are verified against that record's static PEM or JWKS endpoint and resolved to a spp.api.client via the issuer's configurable client_claim. JWKS responses are cached in process memory per issuer record via PyJWKClient; the cache is invalidated on record write/unlink.
|
Updating PR description to reflect changes introduced in 05670eb |
Resolves conflicts in spp_oauth/{__manifest__.py,README.rst,static/description/index.html}.
Manifest: takes 19.0's standardized values (category "OpenSPP", version
19.0.2.0.0, development_status Production/Stable) — aligned with the
branch's stable-prep intent.
README.rst and static/description/index.html: regenerated from the
already-auto-merged readme/*.md fragments via oca-gen-addon-readme.
Reflects the post-19.0.2.0.0 changes on fix/spp-oauth-stable-prep — config parameter renames, model class rename, ACL tightening, new tools exports, and the placeholder-default fix. Minor (not patch) because the work is broader than a single bug fix; not major because the module was still pre-production and the renames are part of the stabilization.
The unverified jwt.decode() that reads the iss claim only to pick the verification key still ran the default verify_exp / verify_nbf / verify_iat checks. For expired tokens this raised ExpiredSignatureError outside the routing function's narrow DecodeError catch, surfacing as the generic "Authentication failed" instead of "Token expired". All claim checks are now explicitly disabled at the routing step so the verifying jwt.decode() inside _validate_internal_rs256 / _validate_external_rs256 is the single source of truth for both verification and error messages. Strengthens the external static-PEM and JWKS expiry tests to assert the "expired" detail message.
The bridge module is moving to Production/Stable, so its trust model needs to be tight. Add spp.api.client.oauth_issuer_id (Many2one, ondelete=restrict) and scope the spp.api.client lookup by the resolved issuer record: internal HS256/RS256 tokens only resolve to clients with no issuer link; external-IdP tokens only resolve to clients linked to their issuer. Without this scoping, an IdP that emitted a claim value colliding with an internal client_id would authenticate as that internal client. Also add a 30-second leeway on RS256 verification (exp/nbf/iat) to absorb normal NTP drift between OpenSPP and external IdPs, remove an unused _validate_rs256_token wrapper, and update test call sites to use _validate_rs256_token_with_issuer directly. spp_oauth docs now recommend RSA-2048 (NIST-approved through 2030, ~5x faster on sign/verify than RSA-4096) and mention ES256 as a faster alternative; the Settings block title drops the "4096 bits" qualifier.
3 participants
Summary
Prepares
spp_oauthfor Production/Stable status and ships a new auto-installing bridge modulespp_api_v2_oauththat enables RS256 JWT authentication for API V2, including from external Identity Providers (e.g. Keycloak) via configurable trusted-issuer records.Changes
spp_oauth— stabilization (now19.0.2.1.0, Production/Stable)oauth_priv_key→oauth_private_key,oauth_pub_key→oauth_public_key) and model class (RegistryConfig→OAuthConfig) across models, config params, views, data, tests, and docs.get_private_key()/get_public_key()raise a clearOpenSPPOAuthJWTExceptionwhen keys are not configured, instead of failing later with a cryptic PyJWT error on the placeholder strings.base.group_systemwithperm_create=1.get_private_keyandget_public_keyfromspp_oauth.tools.OpenSPPOAuthJWTException's constructor — callers decide log level.19.0.2.1.0.spp_api_v2_oauth— new bridge module (now19.0.2.0.0, Beta)Auto-installs when both
spp_api_v2andspp_oauthare present. Accepts both HS256 (unchanged) and RS256 JWTs, with RS256 dispatched per-issuer so external Identity Providers can be trusted without modifying core code.get_authenticated_clientvia FastAPIdependency_overrides. Routes by JWT headeralg:HS256→ delegated to originalspp_api_v2verification (unchanged).RS256→ routed by theissclaim:iss == "openspp-api-v2"→ verified with thespp_oauthpublic key (existing behavior).issmatches an activespp.oauth.issuerrecord → verified via that record's JWKS endpoint or static PEM, with the record's configuredclient_claimmapped tospp.api.client.client_id.POST /oauth/token/rs256with the same client-auth flow, rate limiting, and payload structure as the HS256 endpoint.spp.oauth.issuer(admin-only) —name,issuer,audience,key_source(JWKS URI or static PEM),jwks_uri,public_key,algorithms(whitelist),client_claim,jwks_cache_ttl_seconds,http_timeout_seconds,active. Constraints reject HMAC algorithms,none, plain HTTP for non-loopback URLs, and invalid/private-key PEMs.PyJWKClientcache keyed by issuer record id; invalidated on record write/unlink.iss-routing decode so the verifyingjwt.decode()is the single source of truth for both verification and error messages.noneare blocked.https://(or loopback for dev IdPs).cryptography.hazmat.primitives.serialization.load_pem_public_keyat write time.readme/USAGE.mdincludes a worked Keycloak example.PyJWKClient), iss dispatch, inactive/unknown-issuer rejection,client_claimmapping, and isolation of the internal path from external issuers.Merge from
19.019.0.2.0.0baseline forspp_oauth(Production/Stable, categoryOpenSPP).19.0.2.1.0for the post-19.0.2.0.0 work on this branch (rename + new exports + placeholder fix + ACL tightening).Test plan
./scripts/test_single_module.sh spp_oauth— green./scripts/test_single_module.sh spp_api_v2_oauth— 74/74 passingpre-commit run --all-files— all hooks green (ruff, semgrep, bandit, pylint, OpenSPP custom checks)POST /oauth/token/rs256returns a valid RS256 token when keys are configured.spp.oauth.issuer(JWKS or static PEM) authenticates against anapi_v2endpoint; an expired one returns "Token expired"; an unknownissreturns "Untrusted issuer".🤖 Generated with Claude Code