Skip to content

Implement Client Configuration Endpoint #427

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

Closed
wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Sep 2, 2021

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 2, 2021
@ghost ghost marked this pull request as ready for review September 7, 2021 13:19
@ghost
Copy link
Author

ghost commented Sep 22, 2021

hI @jgrandja . After going again through the specification I think that we will also need to touch the client registration logic . The registration_access_token and the registration_client_uri should be generated when the client registration request is processed. (from specs: Authorization Server MUST provide the Client with the fully qualified URL and The Authorization Server MUST NOT expect the Client to construct or discover this URL on its own.)

Should I do it in this PR or would you prefer to raise a new PR/enhancement for this? (after this one is merged)

@jgrandja
Copy link
Collaborator

Yes, please enhance OidcClientRegistrationEndpointFilter to return the registration_access_token and registration_client_uri as part of the changes in this PR. Thanks.

@jgrandja jgrandja self-assigned this Sep 22, 2021
@jgrandja jgrandja added status: duplicate A duplicate of another issue type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 22, 2021
@jgrandja jgrandja added this to the 0.2.1 milestone Sep 22, 2021
Copy link
Collaborator

@jgrandja jgrandja left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @ovidiupopa91 ! Please see review comments.

@ghost ghost requested a review from jgrandja October 7, 2021 17:12
@ghost
Copy link
Author

ghost commented Oct 7, 2021

Thank you @jgrandja for the review! I've incorporated the changes based on your feedback (and squashed the last commit).

@jgrandja
Copy link
Collaborator

Thanks for the updates @ovidiupopa91 !

After a high-level review, I think we should merge the code from OidcClientConfigurationEndpointFilter into OidcClientRegistrationEndpointFilter, as well, merge OidcClientConfigurationAuthenticationProvider into OidcClientRegistrationAuthenticationProvider.

Most of the code in the 2x Filter's and AuthenticationProvider's is the same so let's combine and reuse.

Take a look at a similar implementation in OAuth2AuthorizationEndpointFilter and OAuth2AuthorizationCodeRequestAuthenticationProvider, which handles the Authorization Request (GET) and Authorization Consent (POST).

Thanks for your work!

@ghost
Copy link
Author

ghost commented Oct 18, 2021

Hi @jgrandja. I've merged the code into OidcClientRegistrationFilter and OidcClientRegistrationAuthenticationProvider.
The code is ready to be reviewed.

Copy link
Collaborator

@jgrandja jgrandja left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @ovidiupopa91 ! Please see review comments.

*
* @author Ovidiu Popa
*/
public class OidcClientConfigurationTests {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this and move tests into OidcClientRegistrationTests

@ghost
Copy link
Author

ghost commented Oct 22, 2021

@jgrandja I finished the changes based on your feedback. This should be ready for another review. Thank you!

@ghost ghost requested a review from jgrandja October 22, 2021 15:48
jgrandja added a commit that referenced this pull request Oct 27, 2021
@jgrandja
Copy link
Collaborator

Thanks for the updates @ovidiupopa91. This is now in main!

FYI, I added some polish in order to get this merged.
Please let me know if you have any questions with the updates.

Thanks again for all your work!

@jgrandja jgrandja closed this Oct 28, 2021
@ghost ghost deleted the gh-355 branch October 28, 2021 04:13
@ghost
Copy link
Author

ghost commented Oct 28, 2021

hi @jgrandja. No questions from my side.
Please let me know if there is anything else I can help with. Thank you!

@jgrandja
Copy link
Collaborator

Appreciate all your help @ovidiupopa91 !
I will get back to you next week with some options for next set of work.
Have a great weekend!

@jgrandja
Copy link
Collaborator

jgrandja commented Nov 4, 2021

Hi @ovidiupopa91. We've had a few issues logged for JdbcOAuth2AuthorizationService as it does not work out of the box with PostgreSQL. Although customization is possible, we should come up with a solution that will make it work by default.

I think we should change the schema from blob to varchar OR another standard SQL datatype that will work with PostgreSQL and most other databases. Would you be able to look into this?

On a side note, I've been working on gh-106 and the attributes varchar(4000) is not long enough when an OAuth2AuthenticationToken is saved under the Principal.class.getName() key. It needed at least 9290 chars so we'll need to fix this as well.

@ghost
Copy link
Author

ghost commented Nov 4, 2021

hi @jgrandja. Sure, I can look into this.

@ghost
Copy link
Author

ghost commented Nov 4, 2021

@jgrandja I've raised 480 to track this change.

jgrandja added a commit that referenced this pull request Nov 15, 2021
doba16 pushed a commit to doba16/spring-authorization-server that referenced this pull request Apr 21, 2023
doba16 pushed a commit to doba16/spring-authorization-server that referenced this pull request Apr 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Client Configuration Endpoint
3 participants