Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed RFC7592 Dynamic Client Registration Management Protocol #505

Merged
merged 10 commits into from
Nov 5, 2022

Conversation

azmeuk
Copy link
Collaborator

@azmeuk azmeuk commented Oct 30, 2022

This PR fixes/implements the RFC7592 spec. It brings those changes

  • The Flask integration keep the request data when the request type is PUT
  • rfc7592/endpoint.py implements the RFC7592 specification
  • The endpoint is tested by tests/flask/test_oauth2/test_client_configuration_endpoint.py
  • rfc7592/endpoint.py has a 100% coverage
  • The implementation is documented by docs/specs/rfc7592.rst

I read RFC7592 and did my best to implement this, but I am not 100% confident to have understood or implemented things right. I think a second pair of eyes might help. I tried to make unit tests as explicit and short as I could, and insert quotes from the spec to illustrate the tests.

The extract_client_metadata method has been copied from RFC7591 (and I removed the software_statement part). Maybe this should be factorized? If so, how should this be? I was thinking of extracting the method in a ClientRegistrationEndpointMixin from which would inherit ClientRegistrationEndpoint and ClientConfigurationEndpoint.

There are breaking changes on some method signatures, but this spec was not documented anyways:

  • introspect_client MUST be implemented
  • save_client is renamed update_client and takes an additional client_metadata parameter
  • introspect_metadata is not required anymore
  • revoke_access_token takes a token parameter

I think the documentation could make clearer that a registration access token can be a different thing than a regular access_token, but I guess this is enough like this.

Fixes #499

  • You consent that the copyright of your pull request source code belongs to Authlib's author.

- The Flask integration keep the request data when the request type is
  `PUT`
- rfc7592/endpoint.py implements the RFC7592 specification
- The endpoint is tested by tests/flask/test_oauth2/test_client_configuration_endpoint.py
- rfc7592/endpoint.py has a 100% coverage
- The implementation is documented by docs/specs/rfc7592.rst
@lepture
Copy link
Owner

lepture commented Oct 31, 2022

  1. I tried to remove sqlachemy from authlib.
  2. are you using some code formatting tool? most of the Authlib code are using ' instead of ", please don't change them.

@lepture
Copy link
Owner

lepture commented Oct 31, 2022

Thanks for your contribution, I'll check the specification implementation later.

@azmeuk
Copy link
Collaborator Author

azmeuk commented Oct 31, 2022

I have added commits for simple quotes and framework independent examples. I added commits to the PR so you can review the differences, but if you prefer (and for future fixes) I can squash the commit and force push.

are you using some code formatting tool?

Yep. I was using black :)

What do you think about using a pre-commit configuration to check and enforce the code style for authlib? We could take inspiration from the wtforms pre-commit configuration. I can open a PR if you wish.

@lepture
Copy link
Owner

lepture commented Oct 31, 2022

@azmeuk I don't like the black code style. Let's keep it the current way.

For testing, I'm using self.assertEqual and other self.assertXXX from unittest module. These method would print better error stack than the raw assert.

No worries, I'll squash to merge the PR. You can add as many commits as you like.

docs/changelog.rst Outdated Show resolved Hide resolved
authlib/oauth2/rfc7592/endpoint.py Show resolved Hide resolved
@lepture lepture added this to the Version 1.2 milestone Nov 1, 2022
@lepture
Copy link
Owner

lepture commented Nov 2, 2022

We are almost done. But there should be a get_server_metadata method implementation in the rfc documentation.

Thanks for your hard work.

@azmeuk
Copy link
Collaborator Author

azmeuk commented Nov 2, 2022

This is done, is this implementation example enough?

@lepture lepture merged commit a7ac27a into lepture:master Nov 5, 2022
@lepture
Copy link
Owner

lepture commented Nov 5, 2022

Thanks.

@azmeuk azmeuk deleted the rfc7592 branch November 5, 2022 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC7592 Dynamic Client Registration Management Protocol
2 participants