Conversation
|
Before going further with this PR and working on tests/documentation, could please someone comment about the general structure of the implementation? My main concerns:
|
|
|
36a433e to
4aeac81
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1573 +/- ##
==========================================
- Coverage 97.38% 96.06% -1.32%
==========================================
Files 34 35 +1
Lines 2214 2315 +101
==========================================
+ Hits 2156 2224 +68
- Misses 58 91 +33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4aeac81 to
8750834
Compare
|
b89d426 to
d3c2684
Compare
|
@lullis it looks like the next step is expanding test coverage right now the patch is just shy of 70%, our target is 100% coverage on patches going forward. At the very least we aim to match the current total coverage. |
|
@dopry this coverage report seems to be stale and based on a previous commit. How can I re-run the coverage report? |
|
It seems something has changed with the jazzband Codecov account and our codecov uploads are getting rate limited. I don't have the necessary permissions to add a the codecov repository upload token to the repo secrets and update our CI to use it. We're working on transferring out of jazzband right now to get away from those types of restrictions. In the meantime I'll keep trying to rerun the build to get coverage uploaded. |
|
I will take a look to see if I can run the report of the diff locally. Running coverage for the whole report showed that all files touched were at least at 99% coverage. |
|
Hi @dopry, is there any update? |
|
No @jezdez still hasn't transferred the repo. |
|
@lullis org is transferred. Can you rebase this PR on the latest master? |
73f43ce to
4e37550
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@lullis it looks like we still have some pretty big gaps in test code coverage for the backchannel logout functionality. Will you have time to work on that? Is there another of your PRs that you would want to prioritize? |
a58ff7f to
780a113
Compare
|
@dopry , I've increased the test coverage as much as I could, but codecov is hung up on one line on application/models.py not being covered. Any tips on the best approach to test that exceptions are being caught in the middle of a function, or is it okay as it is? |
780a113 to
81d83ec
Compare
|
@dopry please take a look again. I did not squash this commit because in case I did not understand your description of the problem correct, As an aside, can you check why the tests would be failing only for the specific combination of python 3.14 and django 5.2? I could not reproduce the issue locally. |
95c5ce9 to
1812980
Compare
|
@lullis this looks good. Are there going to be cases where a user has an access token that is not tied to a refresh token? Do we need to query access tokens to find any of those? I wouldn't want to miss a session that didn't have a refresh token, but still needed a notification, but I'm not sure if that happens in practice offhand. |
1812980 to
acc64af
Compare
|
I believe so. This needs to be rebased so I can test. Unfortunately, the mushroomlabs repo is restricted such that I cannot rebase it or make any minor fixes that might be blocking merging... |
There was a problem hiding this comment.
Pull request overview
Adds OpenID Connect Backchannel Logout support to django-oauth-toolkit by introducing an Application-level backchannel logout URI, advertising support via OIDC discovery metadata, and wiring logout signal handling to send logout tokens to RPs.
Changes:
- Add
backchannel_logout_uritoApplication(model + migration) and expose it in the application create/update UI. - Add OIDC discovery metadata fields for backchannel logout when enabled.
- Add signal handler + checks + documentation and tests for backchannel logout behavior.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
oauth2_provider/handlers.py |
Implements logout token creation and signal-driven dispatch to RP backchannel logout endpoints. |
oauth2_provider/checks.py |
Adds Django system checks for backchannel logout configuration. |
oauth2_provider/exceptions.py |
Adds a dedicated exception type for backchannel logout request failures. |
oauth2_provider/models.py |
Adds backchannel_logout_uri field on Application. |
oauth2_provider/migrations/0015_application_backchannel_logout_uri.py |
Migrates DB schema to include the new Application field. |
oauth2_provider/views/oidc.py |
Publishes backchannel logout capability in OIDC discovery when enabled. |
oauth2_provider/views/application.py |
Adds conditional form field for editing backchannel_logout_uri based on settings. |
oauth2_provider/templates/oauth2_provider/application_detail.html |
Displays the configured backchannel logout URI on the application detail page. |
oauth2_provider/settings.py |
Introduces backchannel logout settings and import-string handling. |
oauth2_provider/apps.py |
Ensures handlers are imported so signal receivers are registered. |
tests/test_backchannel_logout.py |
Adds tests for handler invocation, request sending, and application form integration. |
tests/test_django_checks.py |
Adds checks tests for invalid handler / missing issuer endpoint. |
tests/test_oidc_views.py |
Adds discovery metadata tests for backchannel logout flags. |
tests/presets.py |
Adds OIDC settings presets for backchannel logout test configuration. |
docs/settings.rst |
Documents new settings for backchannel logout. |
docs/oidc.rst |
Documents how to enable/use backchannel logout. |
CHANGELOG.md |
Notes addition of backchannel logout support. |
AUTHORS |
Adds contributor. |
e229dac to
3c05cbe
Compare
|
@dopry , rebased again and implemented the suggested changes given by automated review |
386cd5a to
9031ea4
Compare
There was a problem hiding this comment.
Pull request overview
Adds OpenID Connect Back-Channel Logout support to django-oauth-toolkit by introducing a per-Application backchannel logout URI, advertising capability via OIDC discovery, and wiring logout signal handling to send logout tokens.
Changes:
- Add
backchannel_logout_urito Applications (model + migration) and expose it in application create/update UI and detail template. - Add backchannel logout metadata to OIDC discovery when enabled, plus Django system checks and new handler module for signal-driven logout token delivery.
- Add unit tests and documentation/changelog updates for the new feature.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_oidc_views.py | Tests discovery document advertising backchannel logout support. |
| tests/test_django_checks.py | Tests new Django system checks for backchannel logout configuration. |
| tests/test_backchannel_logout.py | New end-to-end/unit tests for signal handling and logout token sending. |
| tests/presets.py | Adds an OIDC settings preset enabling backchannel logout for tests. |
| oauth2_provider/views/oidc.py | Adds discovery metadata fields for backchannel logout when enabled. |
| oauth2_provider/views/application.py | Conditionally adds backchannel_logout_uri to application editor forms. |
| oauth2_provider/templates/oauth2_provider/application_detail.html | Displays backchannel_logout_uri in the application detail page. |
| oauth2_provider/settings.py | Introduces OIDC_BACKCHANNEL_LOGOUT_* settings (default + import-string support). |
| oauth2_provider/models.py | Adds backchannel_logout_uri field to the Application model. |
| oauth2_provider/migrations/0015_application_backchannel_logout_uri.py | Migration adding the new Application field. |
| oauth2_provider/handlers.py | New signal receiver + default logout request sender implementation. |
| oauth2_provider/exceptions.py | Adds BackchannelLogoutRequestError. |
| oauth2_provider/checks.py | Adds system check validation for backchannel logout configuration. |
| oauth2_provider/apps.py | Ensures handlers module is imported so signal receivers register. |
| docs/settings.rst | Documents new settings. |
| docs/oidc.rst | Documents backchannel logout feature usage. |
| CHANGELOG.md | Notes feature addition in unreleased changelog. |
| AUTHORS | Adds contributor. |
e8e256e to
449540d
Compare
| logger = logging.getLogger(__name__) | ||
|
|
||
| BACKCHANNEL_LOGOUT_TIMEOUT = getattr(oauth2_settings, "OIDC_BACKCHANNEL_LOGOUT_TIMEOUT", 5) | ||
|
|
There was a problem hiding this comment.
BACKCHANNEL_LOGOUT_TIMEOUT is read via getattr(oauth2_settings, "OIDC_BACKCHANNEL_LOGOUT_TIMEOUT", 5), but OIDC_BACKCHANNEL_LOGOUT_TIMEOUT is not defined in oauth2_provider.settings.DEFAULTS. Because oauth2_settings.__getattr__ rejects unknown keys, any user-supplied OAUTH2_PROVIDER["OIDC_BACKCHANNEL_LOGOUT_TIMEOUT"] will be ignored and this will always fall back to 5. Add OIDC_BACKCHANNEL_LOGOUT_TIMEOUT to DEFAULTS (and docs if intended as public), and consider reading it inside send_backchannel_logout_request() instead of at import time so overrides/tests take effect.
| ) | ||
| response.raise_for_status() | ||
| except requests.RequestException as exc: | ||
| raise BackchannelLogoutRequestError(str(exc)) |
There was a problem hiding this comment.
send_backchannel_logout_request re-raises BackchannelLogoutRequestError(str(exc)) without exception chaining, which discards the original traceback and makes debugging/observability harder. Re-raise using raise ... from exc so the original request failure context is preserved.
| raise BackchannelLogoutRequestError(str(exc)) | |
| raise BackchannelLogoutRequestError(str(exc)) from exc |
| default="", | ||
| ) | ||
| backchannel_logout_uri = models.URLField( | ||
| blank=True, null=True, help_text=_("Backchannel Logout URI where logout tokens will be sent") |
There was a problem hiding this comment.
backchannel_logout_uri is stored as a plain URLField but isn’t validated consistently with other URI-like Application fields (e.g., redirect_uris / allowed_origins use AllowedURIValidator + ALLOWED_*SCHEMES in AbstractApplication.clean). Since this value is later used for server-side requests.post, it should be constrained to supported schemes (at least http/https) and validated similarly to avoid storing unsupported/unsafe URIs.
| blank=True, null=True, help_text=_("Backchannel Logout URI where logout tokens will be sent") | |
| blank=True, | |
| null=True, | |
| help_text=_("Backchannel Logout URI where logout tokens will be sent"), | |
| validators=[AllowedURIValidator(oauth2_settings.ALLOWED_REDIRECT_URI_SCHEMES)], |
There was a problem hiding this comment.
We should validate to the spec, but not with oauth2_settings.ALLOWED_REDIRECT_URI_SCHEMES as they're for redirect uris. Also keep in mind that the spec says
backchannel_logout_uri
OPTIONAL. RP URL that will cause the RP to log itself out when
sent a Logout Token by the OP. This URL SHOULD use the "https"
scheme and MAY contain port, path, and query parameter components;
however, it MAY use the "http" scheme, provided that the Client
Type is "confidential", as defined in Section 2.1 of OAuth 2.0
[RFC6749], and provided the OP allows the use of "http" RP URIs.
and we are permissive with http for non-confidential clients on localhost for development and testing scenarios where certs aren't always easy to use. see: docs/settings.rst ALLOWED_SCHEMES for some examples of the language we've used when allowing this in the past and the implementations for how we warn and communicate about these risks at run time.
|
|
||
| <li> | ||
| <p><b>{% trans "Backchannel Logout URI" %}</b></p> | ||
| <input class="input-block-level" type="text" value="{{ application.backchannel_logout_uri }}" readonly> |
There was a problem hiding this comment.
When application.backchannel_logout_uri is None, this renders as the literal string "None" in the input’s value attribute. Use |default_if_none:"" (and/or conditional rendering) so the field displays as empty when unset.
| <input class="input-block-level" type="text" value="{{ application.backchannel_logout_uri }}" readonly> | |
| <input class="input-block-level" type="text" value="{{ application.backchannel_logout_uri|default_if_none:"" }}" readonly> |
|
@lullis thank you for attending to the requests of our pedenatic overlords. There is another round. I wish I could push to your branch here becuase I'd be happy to address these and put a bow on it... unfortunately I don't seem to have permissions to do that with org forks as I do with individual contributors. |
|
Hi @dopry. I understand it might help you to deal with some of the maintenance load, but to be 100% honest dealing with Copilot is quite demotivating, At least for me, it makes me less interested in becoming a more active contributor to the project. Anyway, I gave you write permission to my repo. Let me know if this helps us make quicker progress. |
|
@lullis thanks for the permissions. I've confirmed they're working with a rebase. I'll support where I can and notify you when I make/push changes. I hear you regarding copilot. I'd love to hear more about what you find demotivating. I started a "Copilot, thoughts?" discussion topic for the conversation. |
- Add migration to add backchannel logout support - Add parameters related to backchannel logout on OIDC Discovery View - Change application creation and update form - Change template that renders information about the application
449540d to
ce5bde7
Compare
Fixes #1545
Description of the Change
Checklist
CHANGELOG.mdupdated (only for user relevant changes)AUTHORS