Add support for RP-Initiated Registration#1571
Add support for RP-Initiated Registration#1571lullis wants to merge 1 commit intodjango-oauth:masterfrom
Conversation
fb2e149 to
f7f1e97
Compare
|
@lullis could you please rebase this PR? |
f7f1e97 to
1047e95
Compare
|
@dopry sorry, I haven't seen the notification for this message. I rebased it now, let me know if you need anything else. |
|
@lullis at a glance this looks pretty good and follows my understanding of prompt=create. I'll try to give it a proper review next week. ping me if you don't hear from me. If anyone else is interested in this feature in the meantime, please feel free to test it and provide feedback. |
1047e95 to
b8c3264
Compare
|
@dopry you asked me to ping you about this. Did you get a chance to look at it? |
|
yep and it's still on my radar. As soon as we get the repo transferred out of jazzband i'm going to get codecov fixed merge the other PRs in our backlog and get to this one. |
|
@lullis could you resolve the conflicts and rebase this one when you get a chance? |
e85b5a4 to
83aad34
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
83aad34 to
8841c0b
Compare
|
We need to get codecov passing before we can merge this. |
7d56eb6 to
07b3742
Compare
|
@dopry , coverage issues fixed for this one as well... let me know if you'd like any changes on this. |
|
@dopry Would be great if we could get forward with this feature, too. |
|
Ping @dopry ! |
|
current release is gonna go out, then I'll work on this, |
07b3742 to
96616ed
Compare
|
@dopry, anything you need from me here? |
|
@lullis could your resolve conflicts. I tried this morning, but I can't push back to your fork. |
There was a problem hiding this comment.
Pull request overview
This pull request implements support for RP-Initiated Registration according to the OpenID Connect Prompt Create 1.0 specification. When a prompt=create parameter is added to an authorization request, anonymous users are redirected to a registration page. After successful registration, users are redirected back to the authorization flow with prompt=login to complete the OIDC flow.
Changes:
- Added new settings
OIDC_RP_INITIATED_REGISTRATION_ENABLEDandOIDC_RP_INITIATED_REGISTRATION_VIEW_NAMEto control registration functionality - Implemented
handle_prompt_create()method inAuthorizationViewto handle registration redirects - Updated OIDC discovery endpoint to include "create" in
prompt_values_supportedwhen feature is enabled - Added comprehensive test coverage for various prompt=create scenarios
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| oauth2_provider/views/base.py | Implements the core handle_prompt_create() logic to redirect users to registration and handle various error conditions |
| oauth2_provider/views/oidc.py | Updates OIDC discovery endpoint to advertise "create" in prompt_values_supported when the feature is enabled |
| oauth2_provider/settings.py | Adds configuration settings for enabling RP-initiated registration and specifying the registration view name |
| tests/test_oidc_views.py | Adds comprehensive test suite covering registration redirects, logged user validation, state echoing, and error handling |
| tests/presets.py | Defines test preset configuration for RP-initiated registration tests |
| docs/settings.rst | Documents the new configuration settings with descriptions and default values |
| CHANGELOG.md | Records the new feature addition |
| AUTHORS | Adds contributor credit |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2a69acb to
91e59b5
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| redirect_uri = self.request.GET.get("redirect_uri") | ||
| if redirect_uri: | ||
| response_parameters = {"error": str(exc)} | ||
| state = self.request.GET.get("state") | ||
| if state: | ||
| response_parameters["state"] = state | ||
|
|
||
| separator = "&" if "?" in redirect_uri else "?" | ||
| redirect_to = redirect_uri + separator + urlencode(response_parameters) | ||
| return self.redirect(redirect_to, application=None) | ||
| else: | ||
| return HttpResponseBadRequest(str(exc)) |
There was a problem hiding this comment.
Security vulnerability: The redirect_uri is obtained from request.GET without validation when accessed through handle_no_permission (line 351). While validate_authorization_request has been called when this method is invoked from line 164 in the get method, it has NOT been called when invoked from handle_no_permission. This means an attacker could supply an arbitrary redirect_uri that hasn't been validated against the application's registered redirect URIs. The redirect_uri should only be used after validation, or the method should be restructured to not use redirect_uri for error responses when called from handle_no_permission.
There was a problem hiding this comment.
I agree with regard to the potential security issue here. I believe we need to ensure anytime we redirect someone it is to an allowed redirect_uri. Does that seem correct?
There was a problem hiding this comment.
@dopry You are right, redirect_uri should be validated. As it is now this is an open redirect
| try: | ||
| assert not self.request.user.is_authenticated, "account_selection_required" | ||
| path = self.request.build_absolute_uri() | ||
|
|
||
| views_to_attempt = [oauth2_settings.OIDC_RP_INITIATED_REGISTRATION_VIEW_NAME, "account_signup"] | ||
|
|
||
| registration_url = None | ||
| for view_name in views_to_attempt: | ||
| if not view_name: | ||
| continue | ||
| try: | ||
| registration_url = reverse(view_name) | ||
| break | ||
| except NoReverseMatch: | ||
| # If the URL pattern is not defined for this view name, try the next option. | ||
| pass | ||
|
|
||
| # Parse the current URL and remove the prompt parameter | ||
| parsed = urlparse(path) | ||
| parsed_query = dict(parse_qsl(parsed.query)) | ||
| parsed_query.pop("prompt", None) | ||
|
|
||
| # Create the next parameter to redirect back to the authorization endpoint | ||
| next_url = parsed._replace(query=urlencode(parsed_query)).geturl() | ||
|
|
||
| assert oauth2_settings.OIDC_RP_INITIATED_REGISTRATION_ENABLED, "access_denied" | ||
| assert registration_url is not None, "access_denied" |
There was a problem hiding this comment.
The assertion order is incorrect. The code checks if the user is authenticated and attempts to resolve the registration URL before checking if the feature is enabled (OIDC_RP_INITIATED_REGISTRATION_ENABLED). This means unnecessary work (including potential database queries or URL resolution) happens even when the feature is disabled. The feature enablement check at line 300 should be moved to the beginning of the try block, right after line 275.
There was a problem hiding this comment.
I agree with the assertion here. We should check whether 'create' is supported at the beginning to avoid unnecessary work. In addition, https://openid.net/specs/openid-connect-prompt-create-1_0.html states
If the OpenID Provider receives a prompt value that it does not support (not declared in the prompt_values_supported metadata field) the OP SHOULD respond with an HTTP 400 (Bad Request) status code and an error value of invalid_request. It is RECOMMENDED that the OP return an error_description value identifying the invalid parameter value
access_denied feels incorrect in this assertion.
| @pytest.mark.usefixtures("oauth2_settings") | ||
| @pytest.mark.oauth2_settings(presets.OIDC_SETTINGS_RP_REGISTRATION) | ||
| class TestRPInitiatedRegistration(TestCase): | ||
| def setUp(self): | ||
| Application = get_application_model() | ||
| self.application = Application.objects.create( | ||
| name="Test Application", | ||
| redirect_uris="http://localhost http://example.com", | ||
| client_type=Application.CLIENT_CONFIDENTIAL, | ||
| authorization_grant_type=Application.GRANT_AUTHORIZATION_CODE, | ||
| ) | ||
| User = get_user_model() | ||
| self.test_user = User.objects.create_user("test_user", "test@example.com", "123456") | ||
|
|
||
| def _build_authorization_request(self, query_params, user=None): | ||
| auth_url = reverse("oauth2_provider:authorize") | ||
| query_string = "&".join(f"{k}={v}" for k, v in query_params.items()) | ||
| full_auth_url = f"{auth_url}?{query_string}" | ||
| request = RequestFactory().get(full_auth_url) | ||
| request.user = user or AnonymousUser() | ||
| return request | ||
|
|
||
| def test_connect_discovery_info_has_create(self): | ||
| expected_response = { | ||
| "issuer": "http://localhost/o", | ||
| "authorization_endpoint": "http://localhost/o/authorize/", | ||
| "token_endpoint": "http://localhost/o/token/", | ||
| "userinfo_endpoint": "http://localhost/o/userinfo/", | ||
| "jwks_uri": "http://localhost/o/.well-known/jwks.json", | ||
| "scopes_supported": ["read", "write", "openid"], | ||
| "response_types_supported": [ | ||
| "code", | ||
| "token", | ||
| "id_token", | ||
| "id_token token", | ||
| "code token", | ||
| "code id_token", | ||
| "code id_token token", | ||
| ], | ||
| "subject_types_supported": ["public"], | ||
| "id_token_signing_alg_values_supported": ["RS256", "HS256"], | ||
| "token_endpoint_auth_methods_supported": ["client_secret_post", "client_secret_basic"], | ||
| "code_challenge_methods_supported": ["plain", "S256"], | ||
| "claims_supported": ["sub"], | ||
| "prompt_values_supported": ["none", "login", "create"], | ||
| } | ||
| response = self.client.get("/o/.well-known/openid-configuration") | ||
| self.assertEqual(response.status_code, 200) | ||
| assert response.json() == expected_response | ||
|
|
||
| def test_prompt_create_redirects_to_registration_view(self): | ||
| request = self._build_authorization_request( | ||
| query_params={ | ||
| "response_type": "code", | ||
| "client_id": self.application.client_id, | ||
| "redirect_uri": "http://localhost", | ||
| "scope": "openid", | ||
| "prompt": "create", | ||
| } | ||
| ) | ||
| view = AuthorizationView() | ||
| view.setup(request) | ||
|
|
||
| with patch("oauth2_provider.views.base.reverse") as patched_reverse: | ||
| patched_reverse.return_value = "/register-test/" | ||
| response = view.get(request) | ||
|
|
||
| self.assertEqual(response.status_code, 302) | ||
| redirect_url = response.url | ||
| parsed_url = urlparse(redirect_url) | ||
|
|
||
| # Verify it's the registration URL | ||
| self.assertEqual(parsed_url.path, "/register-test/") | ||
|
|
||
| # Verify the query parameters | ||
| query = parse_qs(parsed_url.query) | ||
| self.assertIn("next", query) | ||
|
|
||
| # Verify the next parameter doesn't contain prompt=create | ||
| next_url = query["next"][0] | ||
| self.assertNotIn("prompt=create", next_url) | ||
|
|
||
| # But it should contain the other original parameters | ||
| self.assertIn("response_type=code", next_url) | ||
| self.assertIn(f"client_id={self.application.client_id}", next_url) | ||
|
|
||
| def test_logged_users_can_not_prompt_create(self): | ||
| view = AuthorizationView() | ||
| request = self._build_authorization_request( | ||
| query_params={ | ||
| "response_type": "code", | ||
| "client_id": self.application.client_id, | ||
| "redirect_uri": "http://localhost", | ||
| "scope": "openid", | ||
| "prompt": "create", | ||
| }, | ||
| user=self.test_user, | ||
| ) | ||
| view.setup(request) | ||
| response = view.get(request) | ||
| self.assertEqual(response.status_code, 302) | ||
| self.assertIn("account_selection_required", response.url) | ||
|
|
||
| def test_state_is_echoed_on_bad_requests(self): | ||
| state_query_params = { | ||
| "response_type": "code", | ||
| "client_id": self.application.client_id, | ||
| "redirect_uri": "http://localhost", | ||
| "scope": "openid", | ||
| "prompt": "create", | ||
| "state": "testing_state", | ||
| } | ||
| view = AuthorizationView() | ||
| request = self._build_authorization_request(query_params=state_query_params, user=self.test_user) | ||
| view.setup(request) | ||
| response = view.get(request) | ||
| self.assertIn("state=testing_state", response.url) | ||
|
|
||
| def test_bad_request_if_missing_redirect_uri(self): | ||
| view = AuthorizationView() | ||
| request = self._build_authorization_request( | ||
| query_params={ | ||
| "response_type": "code", | ||
| "client_id": self.application.client_id, | ||
| "scope": "openid", | ||
| "prompt": "create", | ||
| }, | ||
| user=self.test_user, | ||
| ) | ||
| view.setup(request) | ||
| response = view.handle_prompt_create() | ||
| self.assertEqual(response.status_code, 400) | ||
|
|
||
| def test_redirect_on_handle_no_permission(self): | ||
| view = AuthorizationView() | ||
| request = self._build_authorization_request( | ||
| query_params={ | ||
| "response_type": "code", | ||
| "redirect_uri": "http://localhost", | ||
| "client_id": self.application.client_id, | ||
| "scope": "openid", | ||
| "prompt": "create", | ||
| } | ||
| ) | ||
| view.setup(request) | ||
| response = view.handle_no_permission() | ||
| self.assertEqual(response.status_code, 302) | ||
|
|
||
| def test_bad_request_for_unresolvable_view_name(self): | ||
| view = AuthorizationView() | ||
| request = self._build_authorization_request( | ||
| query_params={ | ||
| "response_type": "code", | ||
| "client_id": self.application.client_id, | ||
| "redirect_uri": "http://localhost", | ||
| "scope": "openid", | ||
| "prompt": "create", | ||
| } | ||
| ) | ||
|
|
||
| with patch("oauth2_provider.views.base.reverse") as patched_reverse: | ||
| patched_reverse.side_effect = NoReverseMatch() | ||
| view.setup(request) | ||
| response = view.get(request) | ||
| self.assertEqual(response.status_code, 302) | ||
| self.assertIn("access_denied", response.url) | ||
|
|
||
|
|
There was a problem hiding this comment.
Missing test coverage: There is no test case for when OIDC_RP_INITIATED_REGISTRATION_ENABLED is False but prompt=create is used. This scenario should be tested to ensure the correct error response is returned. According to the code logic at line 300, when the feature is disabled, an "access_denied" error should be returned.
There was a problem hiding this comment.
We do need to add test for this case., but I think the response should be bad request.
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
The documentation for OIDC_RP_INITIATED_REGISTRATION_VIEW_NAME should include additional information: 1) The fallback behavior to "account_signup" if the configured view name is None or cannot be resolved, 2) The requirement that the registration view must accept a 'next' query parameter for redirecting back to the authorization flow, and 3) What happens when neither the configured view name nor "account_signup" can be resolved (returns access_denied error to the client).
| If this setting is ``None`` or the configured view name cannot be resolved, | |
| the provider will fall back to using the ``"account_signup"`` view. The | |
| registration view (whether configured explicitly or using the fallback) must | |
| accept a ``next`` query parameter, which is used to redirect the user back | |
| to the authorization flow after completing registration. | |
| If neither the configured view name nor ``"account_signup"`` can be resolved, | |
| the authorization request fails and an ``access_denied`` error is returned | |
| to the client. |
There was a problem hiding this comment.
The additonal documentation would be helpful for consumers implementing custom signup views for their IDP. I'm on the fence as to whether these details can be easily inferred.
dopry
left a comment
There was a problem hiding this comment.
I have some q's regarding how we're handling some of the error cases...
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
The additonal documentation would be helpful for consumers implementing custom signup views for their IDP. I'm on the fence as to whether these details can be easily inferred.
| try: | ||
| assert not self.request.user.is_authenticated, "account_selection_required" | ||
| path = self.request.build_absolute_uri() | ||
|
|
||
| views_to_attempt = [oauth2_settings.OIDC_RP_INITIATED_REGISTRATION_VIEW_NAME, "account_signup"] | ||
|
|
||
| registration_url = None | ||
| for view_name in views_to_attempt: | ||
| if not view_name: | ||
| continue | ||
| try: | ||
| registration_url = reverse(view_name) | ||
| break | ||
| except NoReverseMatch: | ||
| # If the URL pattern is not defined for this view name, try the next option. | ||
| pass | ||
|
|
||
| # Parse the current URL and remove the prompt parameter | ||
| parsed = urlparse(path) | ||
| parsed_query = dict(parse_qsl(parsed.query)) | ||
| parsed_query.pop("prompt", None) | ||
|
|
||
| # Create the next parameter to redirect back to the authorization endpoint | ||
| next_url = parsed._replace(query=urlencode(parsed_query)).geturl() | ||
|
|
||
| assert oauth2_settings.OIDC_RP_INITIATED_REGISTRATION_ENABLED, "access_denied" | ||
| assert registration_url is not None, "access_denied" |
There was a problem hiding this comment.
I agree with the assertion here. We should check whether 'create' is supported at the beginning to avoid unnecessary work. In addition, https://openid.net/specs/openid-connect-prompt-create-1_0.html states
If the OpenID Provider receives a prompt value that it does not support (not declared in the prompt_values_supported metadata field) the OP SHOULD respond with an HTTP 400 (Bad Request) status code and an error value of invalid_request. It is RECOMMENDED that the OP return an error_description value identifying the invalid parameter value
access_denied feels incorrect in this assertion.
| next_url = parsed._replace(query=urlencode(parsed_query)).geturl() | ||
|
|
||
| assert oauth2_settings.OIDC_RP_INITIATED_REGISTRATION_ENABLED, "access_denied" | ||
| assert registration_url is not None, "access_denied" |
There was a problem hiding this comment.
I'm also wondering about the handling of a missing registration_url here. access_denied also feels incorrect. This feels more like a misconfigure server that should be kicking out a 500. How is misconfiguration handled else where?
There was a problem hiding this comment.
Django has the ImproperlyConfigured error, and DOT uses it in places: https://github.com/search?q=repo%3Adjango-oauth%2Fdjango-oauth-toolkit+ImproperlyConfigured
| redirect_uri = self.request.GET.get("redirect_uri") | ||
| if redirect_uri: | ||
| response_parameters = {"error": str(exc)} | ||
| state = self.request.GET.get("state") | ||
| if state: | ||
| response_parameters["state"] = state | ||
|
|
||
| separator = "&" if "?" in redirect_uri else "?" | ||
| redirect_to = redirect_uri + separator + urlencode(response_parameters) | ||
| return self.redirect(redirect_to, application=None) | ||
| else: | ||
| return HttpResponseBadRequest(str(exc)) |
There was a problem hiding this comment.
I agree with regard to the potential security issue here. I believe we need to ensure anytime we redirect someone it is to an allowed redirect_uri. Does that seem correct?
| self.assertIn("response_type=code", next_url) | ||
| self.assertIn(f"client_id={self.application.client_id}", next_url) | ||
|
|
||
| def test_logged_users_can_not_prompt_create(self): |
There was a problem hiding this comment.
Are we sure that logged users should not be able to prompt=create?
| view.setup(request) | ||
| response = view.get(request) | ||
| self.assertEqual(response.status_code, 302) | ||
| self.assertIn("access_denied", response.url) |
There was a problem hiding this comment.
shouldn't this be a 400 bad request?
| @pytest.mark.usefixtures("oauth2_settings") | ||
| @pytest.mark.oauth2_settings(presets.OIDC_SETTINGS_RP_REGISTRATION) | ||
| class TestRPInitiatedRegistration(TestCase): | ||
| def setUp(self): | ||
| Application = get_application_model() | ||
| self.application = Application.objects.create( | ||
| name="Test Application", | ||
| redirect_uris="http://localhost http://example.com", | ||
| client_type=Application.CLIENT_CONFIDENTIAL, | ||
| authorization_grant_type=Application.GRANT_AUTHORIZATION_CODE, | ||
| ) | ||
| User = get_user_model() | ||
| self.test_user = User.objects.create_user("test_user", "test@example.com", "123456") | ||
|
|
||
| def _build_authorization_request(self, query_params, user=None): | ||
| auth_url = reverse("oauth2_provider:authorize") | ||
| query_string = "&".join(f"{k}={v}" for k, v in query_params.items()) | ||
| full_auth_url = f"{auth_url}?{query_string}" | ||
| request = RequestFactory().get(full_auth_url) | ||
| request.user = user or AnonymousUser() | ||
| return request | ||
|
|
||
| def test_connect_discovery_info_has_create(self): | ||
| expected_response = { | ||
| "issuer": "http://localhost/o", | ||
| "authorization_endpoint": "http://localhost/o/authorize/", | ||
| "token_endpoint": "http://localhost/o/token/", | ||
| "userinfo_endpoint": "http://localhost/o/userinfo/", | ||
| "jwks_uri": "http://localhost/o/.well-known/jwks.json", | ||
| "scopes_supported": ["read", "write", "openid"], | ||
| "response_types_supported": [ | ||
| "code", | ||
| "token", | ||
| "id_token", | ||
| "id_token token", | ||
| "code token", | ||
| "code id_token", | ||
| "code id_token token", | ||
| ], | ||
| "subject_types_supported": ["public"], | ||
| "id_token_signing_alg_values_supported": ["RS256", "HS256"], | ||
| "token_endpoint_auth_methods_supported": ["client_secret_post", "client_secret_basic"], | ||
| "code_challenge_methods_supported": ["plain", "S256"], | ||
| "claims_supported": ["sub"], | ||
| "prompt_values_supported": ["none", "login", "create"], | ||
| } | ||
| response = self.client.get("/o/.well-known/openid-configuration") | ||
| self.assertEqual(response.status_code, 200) | ||
| assert response.json() == expected_response | ||
|
|
||
| def test_prompt_create_redirects_to_registration_view(self): | ||
| request = self._build_authorization_request( | ||
| query_params={ | ||
| "response_type": "code", | ||
| "client_id": self.application.client_id, | ||
| "redirect_uri": "http://localhost", | ||
| "scope": "openid", | ||
| "prompt": "create", | ||
| } | ||
| ) | ||
| view = AuthorizationView() | ||
| view.setup(request) | ||
|
|
||
| with patch("oauth2_provider.views.base.reverse") as patched_reverse: | ||
| patched_reverse.return_value = "/register-test/" | ||
| response = view.get(request) | ||
|
|
||
| self.assertEqual(response.status_code, 302) | ||
| redirect_url = response.url | ||
| parsed_url = urlparse(redirect_url) | ||
|
|
||
| # Verify it's the registration URL | ||
| self.assertEqual(parsed_url.path, "/register-test/") | ||
|
|
||
| # Verify the query parameters | ||
| query = parse_qs(parsed_url.query) | ||
| self.assertIn("next", query) | ||
|
|
||
| # Verify the next parameter doesn't contain prompt=create | ||
| next_url = query["next"][0] | ||
| self.assertNotIn("prompt=create", next_url) | ||
|
|
||
| # But it should contain the other original parameters | ||
| self.assertIn("response_type=code", next_url) | ||
| self.assertIn(f"client_id={self.application.client_id}", next_url) | ||
|
|
||
| def test_logged_users_can_not_prompt_create(self): | ||
| view = AuthorizationView() | ||
| request = self._build_authorization_request( | ||
| query_params={ | ||
| "response_type": "code", | ||
| "client_id": self.application.client_id, | ||
| "redirect_uri": "http://localhost", | ||
| "scope": "openid", | ||
| "prompt": "create", | ||
| }, | ||
| user=self.test_user, | ||
| ) | ||
| view.setup(request) | ||
| response = view.get(request) | ||
| self.assertEqual(response.status_code, 302) | ||
| self.assertIn("account_selection_required", response.url) | ||
|
|
||
| def test_state_is_echoed_on_bad_requests(self): | ||
| state_query_params = { | ||
| "response_type": "code", | ||
| "client_id": self.application.client_id, | ||
| "redirect_uri": "http://localhost", | ||
| "scope": "openid", | ||
| "prompt": "create", | ||
| "state": "testing_state", | ||
| } | ||
| view = AuthorizationView() | ||
| request = self._build_authorization_request(query_params=state_query_params, user=self.test_user) | ||
| view.setup(request) | ||
| response = view.get(request) | ||
| self.assertIn("state=testing_state", response.url) | ||
|
|
||
| def test_bad_request_if_missing_redirect_uri(self): | ||
| view = AuthorizationView() | ||
| request = self._build_authorization_request( | ||
| query_params={ | ||
| "response_type": "code", | ||
| "client_id": self.application.client_id, | ||
| "scope": "openid", | ||
| "prompt": "create", | ||
| }, | ||
| user=self.test_user, | ||
| ) | ||
| view.setup(request) | ||
| response = view.handle_prompt_create() | ||
| self.assertEqual(response.status_code, 400) | ||
|
|
||
| def test_redirect_on_handle_no_permission(self): | ||
| view = AuthorizationView() | ||
| request = self._build_authorization_request( | ||
| query_params={ | ||
| "response_type": "code", | ||
| "redirect_uri": "http://localhost", | ||
| "client_id": self.application.client_id, | ||
| "scope": "openid", | ||
| "prompt": "create", | ||
| } | ||
| ) | ||
| view.setup(request) | ||
| response = view.handle_no_permission() | ||
| self.assertEqual(response.status_code, 302) | ||
|
|
||
| def test_bad_request_for_unresolvable_view_name(self): | ||
| view = AuthorizationView() | ||
| request = self._build_authorization_request( | ||
| query_params={ | ||
| "response_type": "code", | ||
| "client_id": self.application.client_id, | ||
| "redirect_uri": "http://localhost", | ||
| "scope": "openid", | ||
| "prompt": "create", | ||
| } | ||
| ) | ||
|
|
||
| with patch("oauth2_provider.views.base.reverse") as patched_reverse: | ||
| patched_reverse.side_effect = NoReverseMatch() | ||
| view.setup(request) | ||
| response = view.get(request) | ||
| self.assertEqual(response.status_code, 302) | ||
| self.assertIn("access_denied", response.url) | ||
|
|
||
|
|
There was a problem hiding this comment.
We do need to add test for this case., but I think the response should be bad request.
| next_url = parsed._replace(query=urlencode(parsed_query)).geturl() | ||
|
|
||
| assert oauth2_settings.OIDC_RP_INITIATED_REGISTRATION_ENABLED, "access_denied" | ||
| assert registration_url is not None, "access_denied" |
There was a problem hiding this comment.
Django has the ImproperlyConfigured error, and DOT uses it in places: https://github.com/search?q=repo%3Adjango-oauth%2Fdjango-oauth-toolkit+ImproperlyConfigured
|
|
||
| """ | ||
| try: | ||
| assert not self.request.user.is_authenticated, "account_selection_required" |
There was a problem hiding this comment.
Using assert for control flow can be risky, as assertions are not executed when Python is run with the -O optimization flag. It’s safer to use explicit exceptions or restructure these conditionals so they don’t rely on assert.
| assert not self.request.user.is_authenticated, "account_selection_required" | ||
| path = self.request.build_absolute_uri() | ||
|
|
||
| views_to_attempt = [oauth2_settings.OIDC_RP_INITIATED_REGISTRATION_VIEW_NAME, "account_signup"] |
There was a problem hiding this comment.
Is account_signup an arbitrary choice or based on something that is usually available?
| redirect_uri = self.request.GET.get("redirect_uri") | ||
| if redirect_uri: | ||
| response_parameters = {"error": str(exc)} | ||
| state = self.request.GET.get("state") | ||
| if state: | ||
| response_parameters["state"] = state | ||
|
|
||
| separator = "&" if "?" in redirect_uri else "?" | ||
| redirect_to = redirect_uri + separator + urlencode(response_parameters) | ||
| return self.redirect(redirect_to, application=None) | ||
| else: | ||
| return HttpResponseBadRequest(str(exc)) |
There was a problem hiding this comment.
@dopry You are right, redirect_uri should be validated. As it is now this is an open redirect
|
@lullis Can you please reply to these change requests and/or act on them? |
|
@lullis could you rebase this PR? |
|
Could you please disable the Copilot bullshit for our merge requests? |
|
@Natureshadow, no. Please mind the profanity. It's not acceptable here. |
By adding a `prompt=create` parameter to the authorization request, the user is redirected to the OP's registration point where they can create an account, and on successful registration the user is then redirected back to the authorization view with prompt=login Closes django-oauth#1546
91e59b5 to
737f8c2
Compare
Fixes #1546
Description of the Change
By adding a
prompt=createparameter to the authorization request, the user is redirected to the OP's registration point where they can create an account, and on successful registration the user is then redirected back to the authorization view with prompt=loginChecklist
CHANGELOG.mdupdated (only for user relevant changes)AUTHORS