-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Add JdbcRelyingPartyRegistrationRepository #17077
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
base: main
Are you sure you want to change the base?
Conversation
Closes spring-projectsgh-16012 Signed-off-by: chao.wang <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @wapkch, thanks for this PR! Looking at #16012, I believe the ask is to be able to have separate places from which to draw information for the RP and AP metadata. As such, I think let's begin with implementing AssertingPartyMetadataRepository
(so JdbcAssertingPartyMetadataRepository
). You should be able to preserve most of what you've written, just staying focused on the asserting party. Are you able to make these changes?
/** | ||
* A JDBC implementation of {@link RelyingPartyRegistrationRepository}. Also implements | ||
* {@link Iterable} to simplify the default login page. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will you please add @since 7.0
in this class-level JavaDoc? You may also want to add yourself as the @author
.
* A JDBC implementation of {@link RelyingPartyRegistrationRepository}. Also implements | ||
* {@link Iterable} to simplify the default login page. | ||
*/ | ||
public class JdbcRelyingPartyRegistrationRepository implements IterableRelyingPartyRegistrationRepository { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modern Spring Security components favor composition over inheritance. As such, please make this final
for now.
* @param relyingPartyRegistrationRowMapper the {@link RowMapper} used for mapping the | ||
* current row in {@code java.sql.ResultSet} to {@link RelyingPartyRegistration} | ||
*/ | ||
public final void setAuthorizedClientRowMapper( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the class will be final
this can remain non-final
. This is specifically helpful if when we do open the class for extension, folks can have a setter that extends and calls this one.
* @param relyingPartyRegistrationRowMapper the {@link RowMapper} used for mapping the | ||
* current row in {@code java.sql.ResultSet} to {@link RelyingPartyRegistration} | ||
*/ | ||
public final void setAuthorizedClientRowMapper( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also call it setAssertingPartyMetadataRowMapper
instead.
|
||
protected RowMapper<RelyingPartyRegistration> relyingPartyRegistrationRowMapper; | ||
|
||
protected final LobHandler lobHandler; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GIven that LobHandler is deprecated by the Spring Framework, let's please work with Blob's and byte array's directly. You can see JdbcUserCredentialRepsitory for an example.
|
||
} | ||
|
||
public static class Certificate { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By moving to Java serialization, I believe this class becomes unnecessary.
* The default {@link RowMapper} that maps the current row in | ||
* {@code java.sql.ResultSet} to {@link RelyingPartyRegistration}. | ||
*/ | ||
public static class RelyingPartyRegistrationRowMapper implements RowMapper<RelyingPartyRegistration> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's please remove the lob handler and as such also make this class private
and final
for the time being.
|
||
private RelyingPartyRegistration.Builder createBuilderUsingMetadata(String assertingPartyEntityId, | ||
String assertingPartyMetadataUri) { | ||
Collection<RelyingPartyRegistration.Builder> candidates = RelyingPartyRegistrations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's please keep the implementations separate for now. That is, we should build it only from the database record in the JDBC implementation.
builder.signingX509Credentials((credentials) -> credentials.add(signingCredential)); | ||
} | ||
catch (Exception ex) { | ||
this.logger.error(LogMessage.format("Signing credentials of %s must have a valid certificate.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please log at the DEBUG level in all cases in this class
*/ | ||
public class JdbcRelyingPartyRegistrationRepository implements IterableRelyingPartyRegistrationRepository { | ||
|
||
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though Jackson is a common dependency, we don't want to require it in order to use the class. Let's use Java serialization instead to write the credentials. In the future, we can expose a setter to configure how the credentials get read and written.
Closes gh-16012