Added authenticated dynamic client registration #123

infohash · 2022-01-30T12:32:21Z

Identity Providers support two ways how new clients can be registered through Client Registration Service.

Authenticated requests - Request to register new client must contain either Initial Access Token or Bearer Token.
Anonymous requests - Request to register new client doesn’t need to contain any token at all.

Features:

Support for authenticated requests by using Initial Access Token.
Register multiple redirect URIs when the client is dynamically created.

Refer to Client Registration and Keycloak's Client Registration Service for more info.

Identity Providers support two ways how new clients can be registered through Client Registration Service. Authenticated requests - Request to register new client must contain either Initial Access Token or Bearer Token. Anonymous requests - Request to register new client doesn’t need to contain any token at all. This feature adds support for Authenticated requests.

…_config is passed from OIDCAuthentication

This reverts commit 84d0419.

…rect_uri_config is passed from OIDCAuthentication" This reverts commit 3ae93d4.

infohash · 2022-01-31T08:32:58Z

Flask-pyoidc/src/flask_pyoidc/pyoidc_facade.py

Lines 72 to 73 in 40b1163

    
           def register(self, extra_registration_params=None): 
        
               client_metadata = self._provider_configuration.register_client([self._redirect_uri], extra_registration_params)

When the client is dynamically registered, the user may want to register multiple redirect_uris but in this line we are adding only one here.

Flask-pyoidc/src/flask_pyoidc/provider_configuration.py

Line 174 in 40b1163

registration_request['redirect_uris'] = redirect_uris

Then this line adds only one redirect_uri in the registration request. This is also causing redirect to logout view to fail because post_logout_redirect_uri is not in the list of Identity Provider's Valid Redirect URIs.

User can add custom redirect uris for dynamic client registration through ClientRegistrationInfo class. This feature also ensures atleast one redirect uri is always registered with IdP.

infohash · 2022-02-01T20:37:28Z

This feature can dynamically register all kinds of redirect URIs through Dynamic Client Registration so to register custom post logout redirect URIs in #124 dynamically, #123 is required.

Comply with 4.4.2. Access Token Request of Client Credentials Grant in RFC 6749

zamzterz · 2022-02-07T19:33:38Z

Yet another great contribution! 🙌

I've just had a brief look at the changes, but to get a better overview (and make it easier to review and track the changes) could you please split it into separate PRs?
There's at least 3 different features/changes in this single PR which makes it hard to untangle what/how the changes are related:

Support initial access token for registration
Support configuring multiple redirect_uris when using dynamic registration
Adding support for scope parameter when using client credentials grant method.

infohash · 2022-02-07T19:55:23Z

This will break lot of code if I split this. 1 & 2 are related to each other. I can write a review on my code to reference which are 1 & 2. Perhaps if you view from Files changed section, it'll be much easier to review changes.

src/flask_pyoidc/provider_configuration.py

infohash · 2022-02-07T20:09:33Z

src/flask_pyoidc/pyoidc_facade.py

        """
        client_credentials_payload = {
            'grant_type': 'client_credentials'
        }
+        if scope:
+            client_credentials_payload['scope'] = ' '.join(scope)


A minor change, client credentials flow as per ouath specs can also take scope as an argument which takes string as a parameter delimited by spaces to separate multiple scopes. E.g. scope='read write'. Just to make it easier for the user, I'm taking it as a list and convert it into the string for the payload.

zamzterz · 2022-02-07T20:54:43Z

While your comments help, I'd strongly prefer it to be split to follow the principle of "1 PR per change".
This PR is quite big (11 changed files) and only some of those actually relate to the title issue of this PR (include initial access token with client registration request).

It will help with the reviewing process (making it go faster) to have smaller chunks of changes to review.

infohash · 2022-02-07T21:10:06Z

I'm not sure if 1 & 2 are separable. Even their test cases are common. They may look different but they are part of the same flow that collects all redirect uris from the user code & then send them in the registration request. I implemented the 1st one but during testing I found it incomplete because it was registering only one redirect uri which lead to invalid_redirect_uri error. Then I worked on 2nd to complete the flow.

I should rather update PR description and title so it doesn't look like 2 different independent features.

I can separate 3rd one, it's completely exclusive.

zamzterz · 2022-02-08T08:32:59Z

I might be misunderstanding something, but I don't see how 1 & 2 are related 🙂
Yes, they are both part of client registration but

Adding support for including an access token in the request is only the addition of registration_token in ClientRegistrationInfo (which is then picked up by underlying pyoidc lib), while
Which redirect URIs are to be registered with the provider is independent of whether the request is authenticated (via token) or not.

Is it something specific for your use case/setup (which I gather is using Keycloak)?

infohash · 2022-02-08T09:54:38Z

I think about new features and raise pull request. Keycloak is happen to be the one I'm using to test my features.

It's not worth the time to rework 1 & 2 separately because their test cases also have to be reverted and reworked. It broke existing test cases as well which I had to address them before writing new ones. It will duplicate my work to fix test cases again if I separate them.

I can draft a pull request for review purpose only. For merge, this pull request is the accurate one.

zamzterz · 2022-02-09T12:35:43Z

Ok, leave it be for now. 🙂

I'm having a pretty busy week, but I'll try to review it later this week. 👍

zamzterz

Great addition, thanks for taking the time to put this together! 🎉

I think there's some unnecessary inconsistencies with how the redirect URIs are handled, both in implementation and behavior:

specifying redirect_uris in client registration info should be passed unmodified to the provider
post_logout_redirect_uris should not be mixed with redirect_uris
determining whether redirect URIs have been specified should happen in OIDCAuthentication._register_client

Other than that it's mostly minor questions 🙂

.gitignore

docs/configuration.md

docs/quickstart.md

src/flask_pyoidc/flask_pyoidc.py

src/flask_pyoidc/provider_configuration.py

tests/test_provider_configuration.py

zamzterz · 2022-02-12T14:46:04Z

tests/test_provider_configuration.py

@@ -102,3 +218,9 @@ def test_copy_should_overwrite_existing_value(self):
        data = OIDCData(abc='xyz')
        copy_data = data.copy(qwe='rty', abc='123')
        assert copy_data == {'abc': '123', 'qwe': 'rty'}
+
+    def test_del_and_len(self):


I don't think this test is needed since those methods are directly delegated to the underlying storage.
(That's why there's only the case for OIDCData.__str__ above - it's the only method except copy which has some custom behavior.)

It has increased the coverage.

Same as above, it's unrelated to this PR and I really don't think it's necessary to test builtin dict-behavior.

zamzterz · 2022-02-12T14:48:02Z

tests/test_redirect_uri_config.py

@@ -31,6 +31,7 @@ def test_redirect_uri_config(self):
        redirect_uri_config = RedirectUriConfig.from_config(config)
        assert redirect_uri_config.full_uri == 'https://myexample.com:6000/callback'
        assert redirect_uri_config.endpoint == 'callback'
+        assert repr(redirect_uri_config) == f'({redirect_uri_config.full_uri}, {redirect_uri_config.endpoint})'


Unrelated to this PR and I'm not sure it's needed to test the string representation for this class (which isn't intended to be used for anything).

To increase the test coverage. It's at 98.86% now.

While I appreciate your effort, striving for 100 % coverage is not really necessary. 🙂
I think it's pushing against the limits of the trade-off of having more test code to maintain.

tests/test_pyoidc_facade.py

zamzterz · 2022-02-12T14:48:59Z

tests/test_pyoidc_facade.py

+            'registration_client_uri': 'https://op.example.com/register/client1',
+            'registration_access_token': 'registration_access_token1'
+        }
+        responses.add(responses.POST, registration_endpoint, json=client_registration_response)


Why this change?

To mock client registration response.

What was the difference needed for mocking the response like that instead of using the existing CLIENT_METADATA?

The response must have redirect_uris.

The response is not used/verified in this test.
So this would be an unrelated change that is not needed.

The response still has to be mocked to initialize ClientMetadata by using the parameters taken from the response. Then only it can be considered that assert facade.is_registered() is True is actually true.

Then only it can be considered that assert facade.is_registered() is True is actually true.

Not sure I'm following - the tests passed before this change and the actual values in the response does not matter.
What makes this change necessary for this tests?

…ed-dynamic-client-registration

1. Improved provider_configuration's register_client 2. Added and moved test cases.

infohash · 2022-02-18T22:02:40Z

Requested changes are done.

zamzterz

This is getting real close now!

The only main thing left is the mixing of redirect_uris and post_logout_redirect_uris (both in registration request and response).
I think this extension should opt for standards compliance so that special handling should not be included (and specifically Keycloak has an open issue for fixing this in their provider implementation).

src/flask_pyoidc/provider_configuration.py

zamzterz · 2022-02-26T20:11:49Z

tests/test_pyoidc_facade.py

+            'registration_client_uri': 'https://op.example.com/register/client1',
+            'registration_access_token': 'registration_access_token1'
+        }
+        responses.add(responses.POST, registration_endpoint, json=client_registration_response)


The response is not used/verified in this test.
So this would be an unrelated change that is not needed.

src/flask_pyoidc/flask_pyoidc.py

To comply with OIDC specs

src/flask_pyoidc/provider_configuration.py

infohash · 2022-03-04T16:30:31Z

Have you abandoned this project?

zamzterz · 2022-03-06T15:28:08Z

Have you abandoned this project?

Sorry for the long turnaround time on these PRs! 😞
I have not abandoned this project, but it is more in a state of maintenance than active development.
And the time I have to spend on it is unfortunately very limited right now (mainly some small windows during weekends as you might have noticed).

zamzterz

Thanks for sticking with it! 👏

This looks ready to go with the minor fix of reverting the use of requests_session in ProviderConfiguration.register_client

src/flask_pyoidc/provider_configuration.py

infohash · 2022-03-08T09:51:24Z

Until keycloak approves the feature to register post logout redirect uris keycloak/keycloak#9950, there's a workaround to register post_logout_redirect_uris with ClientMetadata. For anyone who ends up here with the issue:

...
auth = OIDCAuthentication({'default': provider_config})
client = auth.clients['default']
with app.app_context():
    auth._register_client(client)
client._provider_configuration._client_metadata[
    'post_logout_redirect_uris'] = ['https://client.example.com/logout', 'https://client.example.com/logout2']
client.register()

infohash added 7 commits January 30, 2022 17:56

added test, updated docs and changed kwargs

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

f0e20c0

fixed test cases relevant to client registration

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

db64bd7

fixed bug _redirect_uri_config has no attribute endpoint redirect_uri…

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

3ae93d4

…_config is passed from OIDCAuthentication

fixed test cases for redirect_uri_config

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

84d0419

Revert "fixed test cases for redirect_uri_config"

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

6565ab6

This reverts commit 84d0419.

infohash added 5 commits February 2, 2022 03:21

fixed test cases of provider configuration

Unverified

This user has not yet uploaded their public signing key.

GPG key ID: 2B899287C9C18D6D

Learn about vigilant mode

34c9515

fixed test cases of pyoidc_facade

5bd2abd

added scope in client credentials grant

ac44dca

Comply with 4.4.2. Access Token Request of Client Credentials Grant in RFC 6749

improved test coverage

013fc12

added test case for _register_client

fe59d07

infohash commented Feb 7, 2022

View reviewed changes

src/flask_pyoidc/provider_configuration.py Show resolved Hide resolved

infohash commented Feb 7, 2022

View reviewed changes

moved changes to client credentials grant to new PR

5724eb8

zamzterz requested changes Feb 12, 2022

View reviewed changes

infohash added 2 commits February 13, 2022 00:50

Merge remote-tracking branch 'upstream/main' into feature/authenticat…

17fee78

…ed-dynamic-client-registration

requested changes done

8a4a3e6

1. Improved provider_configuration's register_client 2. Added and moved test cases.

added assertion to check initial access token in the request header

93ef3c0

zamzterz requested changes Feb 26, 2022

View reviewed changes

separated post_logout_redirect_uris from redirect_uris

949b6e4

To comply with OIDC specs

zamzterz reviewed Feb 27, 2022

View reviewed changes

src/flask_pyoidc/provider_configuration.py Outdated Show resolved Hide resolved

removed post_logout_redirect_uris keyword argument

ee79997

removed overriding of post_logout_redirect_uris

6f463a3

zamzterz reviewed Mar 7, 2022

View reviewed changes

src/flask_pyoidc/provider_configuration.py Show resolved Hide resolved

zamzterz added 2 commits March 7, 2022 22:33

Security fix: don't log registration response (contains client_secret).

01443ab

Align formatting of configuration docs and tiny typos.

22380df

zamzterz approved these changes Mar 7, 2022

View reviewed changes

src/flask_pyoidc/provider_configuration.py Show resolved Hide resolved

zamzterz merged commit cb6a487 into zamzterz:main Mar 7, 2022

infohash deleted the feature/authenticated-dynamic-client-registration branch March 7, 2022 21:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added authenticated dynamic client registration #123

Added authenticated dynamic client registration #123

infohash commented Jan 30, 2022 •

edited

Loading

infohash commented Jan 31, 2022 •

edited

Loading

infohash commented Feb 1, 2022 •

edited

Loading

zamzterz commented Feb 7, 2022

infohash commented Feb 7, 2022

infohash Feb 7, 2022

zamzterz commented Feb 7, 2022

infohash commented Feb 7, 2022 •

edited

Loading

zamzterz commented Feb 8, 2022

infohash commented Feb 8, 2022

zamzterz commented Feb 9, 2022

zamzterz left a comment

zamzterz Feb 12, 2022

infohash Feb 12, 2022

zamzterz Feb 13, 2022

zamzterz Feb 12, 2022

infohash Feb 12, 2022

zamzterz Feb 13, 2022

zamzterz Feb 12, 2022

infohash Feb 12, 2022

zamzterz Feb 13, 2022

infohash Feb 18, 2022

zamzterz Feb 26, 2022

infohash Feb 27, 2022

zamzterz Feb 27, 2022

infohash commented Feb 18, 2022

zamzterz left a comment

zamzterz Feb 26, 2022

infohash commented Mar 4, 2022

zamzterz commented Mar 6, 2022

zamzterz left a comment

infohash commented Mar 8, 2022

Added authenticated dynamic client registration #123

Added authenticated dynamic client registration #123

Conversation

infohash commented Jan 30, 2022 • edited Loading

infohash commented Jan 31, 2022 • edited Loading

infohash commented Feb 1, 2022 • edited Loading

zamzterz commented Feb 7, 2022

infohash commented Feb 7, 2022

Choose a reason for hiding this comment

zamzterz commented Feb 7, 2022

infohash commented Feb 7, 2022 • edited Loading

zamzterz commented Feb 8, 2022

infohash commented Feb 8, 2022

zamzterz commented Feb 9, 2022

zamzterz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infohash commented Feb 18, 2022

zamzterz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infohash commented Mar 4, 2022

zamzterz commented Mar 6, 2022

zamzterz left a comment

Choose a reason for hiding this comment

infohash commented Mar 8, 2022

infohash commented Jan 30, 2022 •

edited

Loading

infohash commented Jan 31, 2022 •

edited

Loading

infohash commented Feb 1, 2022 •

edited

Loading

infohash commented Feb 7, 2022 •

edited

Loading