Skip to content
This repository was archived by the owner on Aug 29, 2023. It is now read-only.

Conversation

@ryanluker
Copy link

@ryanluker ryanluker commented Nov 17, 2020

https://app.clubhouse.io/greenspace/story/26294/properly-add-request-meta-to-authorization-response

Description of the Change

Before we couldn't do custom issuer domains because of an issue where the request was being sanitized and loosing the request headers. This PR uses the request.META before the sanitation occurs to persist the headers into the lower areas of the code.

This module leverages the oauthlib to construct this response so you will need to look here if you wish to see how it was moving the headers around.
https://oauthlib.readthedocs.io/en/latest/_modules/oauthlib/oauth2/rfc6749/endpoints/authorization.html#AuthorizationEndpoint.create_authorization_response

Note:
Reminder that this is a library and not the greenspace project, we dont expect to verify acceptance style tests in this repo. I will create a release for 1.4.1 and then update the greenspace requirements to include these changes.

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • CHANGELOG.md updated (only for user relevant changes) No api breaking changes on this bug fix
  • author name in AUTHORS N/A

Examples

Failing test

Screenshot from 2020-11-17 13-02-54

QA Notes (no CICD in this repo)

How to run all the tests

tox -e py36-django22

How to run the single test

After following the setup guide in the readme you can run the single new test with the command below.

tox -e py36-django22 -- tests/test_authorization_code.py::TestAuthorizationCodeTokenView::test_id_token_public_oidc_capable

Add the request headers to authorization response to allow
for dynamic issuer support.
@ryanluker ryanluker requested review from aemitos and enosan November 17, 2020 21:08
@ryanluker ryanluker changed the title Send in the request meta for headers usage Send in the request meta for headers when creating the authorization response Nov 17, 2020
Copy link

@alexpdraper alexpdraper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks super good and professional. The source repo owners are going to love it!


# Turn back on the OIDC specific endpoints
oauth2_settings.OIDC_ISS_ENDPOINT = "http://localhost"
oauth2_settings.OIDC_USERINFO_ENDPOINT = "http://localhost/userinfo/"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to move this logic into before/after functions. If the test fails before this then these lines won’t be called and the settings won’t get restored.

The other thing I’d consider doing is storing the initial values in variables and then setting to those variables after instead of hardcoding the settings here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yah I was pondering along similar lines but I wanted to keep with the same style that are in the other tests inside test_oidc_views.

class TestConnectDiscoveryInfoView(TestCase):
    def test_get_connect_discovery_info(self):
        expected_response = {
            "issuer": "http://localhost",
            "authorization_endpoint": "http://localhost/o/authorize/",
            "token_endpoint": "http://localhost/o/token/",
            "userinfo_endpoint": "http://localhost/userinfo/",
            "jwks_uri": "http://localhost/o/jwks/",
            "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"]
        }
        response = self.client.get(reverse("oauth2_provider:oidc-connect-discovery-info"))
        self.assertEqual(response.status_code, 200)
        assert response.json() == expected_response

    def test_get_connect_discovery_info_without_issuer_url(self):
        oauth2_settings.OIDC_ISS_ENDPOINT = None
        oauth2_settings.OIDC_USERINFO_ENDPOINT = None
        expected_response = {
            "issuer": "http://testserver/o",
            "authorization_endpoint": "http://testserver/o/authorize/",
            "token_endpoint": "http://testserver/o/token/",
            "userinfo_endpoint": "http://testserver/o/userinfo/",
            "jwks_uri": "http://testserver/o/jwks/",
            "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"]
        }
        response = self.client.get(reverse("oauth2_provider:oidc-connect-discovery-info"))
        self.assertEqual(response.status_code, 200)
        assert response.json() == expected_response
        oauth2_settings.OIDC_ISS_ENDPOINT = "http://localhost"
        oauth2_settings.OIDC_USERINFO_ENDPOINT = "http://localhost/userinfo/"


class TestJwksInfoView(TestCase):
    def test_get_jwks_info(self):
        expected_response = {
            "keys": [{
                "alg": "RS256",
                "use": "sig",
                "kid": "s4a1o8mFEd1tATAIH96caMlu4hOxzBUaI2QTqbYNBHs",
                "e": "AQAB",
                "kty": "RSA",
                "n": "mwmIeYdjZkLgalTuhvvwjvnB5vVQc7G9DHgOm20Hw524bLVTk49IXJ2Scw42HOmowWWX-oMVT_ca3ZvVIeffVSN1-TxVy2zB65s0wDMwhiMoPv35z9IKHGMZgl9vlyso_2b7daVF_FQDdgIayUn8TQylBxEU1RFfW0QSYOBdAt8"  # noqa
            }]
        }
        response = self.client.get(reverse("oauth2_provider:jwks-info"))
        self.assertEqual(response.status_code, 200)
        assert response.json() == expected_response

Copy link

@enosan enosan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ryanluker Nice and elegant! LGTM.

@ryanluker ryanluker merged commit d719ed9 into master Nov 17, 2020
@ryanluker ryanluker deleted the 26294-meta-headers-support branch November 17, 2020 23:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants