Make new dashboard client credentials configurable on bootstrap#1185
Make new dashboard client credentials configurable on bootstrap#1185
Conversation
iam-login-service/src/main/java/it/infn/mw/iam/config/IamProperties.java
Outdated
Show resolved
Hide resolved
iam-login-service/src/main/java/it/infn/mw/iam/core/util/StartupRunner.java
Outdated
Show resolved
Hide resolved
iam-login-service/src/main/java/it/infn/mw/iam/dashboard/DashboardConfigService.java
Outdated
Show resolved
Hide resolved
iam-login-service/src/main/java/it/infn/mw/iam/dashboard/DashboardConfigService.java
Outdated
Show resolved
Hide resolved
iam-login-service/src/main/java/it/infn/mw/iam/dashboard/DashboardConfigService.java
Outdated
Show resolved
Hide resolved
iam-login-service/src/main/java/it/infn/mw/iam/dashboard/DashboardConfigService.java
Outdated
Show resolved
Hide resolved
iam-login-service/src/main/java/it/infn/mw/iam/dashboard/DashboardConfigService.java
Outdated
Show resolved
Hide resolved
enricovianello
added
status/changes-requested
priority/high
and removed
status/in-progress
component/dashboard
labels
|
enricovianello
changed the title
enricovianello
left a comment
enricovianello
left a comment
There was a problem hiding this comment.
Probably some safer comparisons could be done and a missing ClientUpdatedEvent to be fixed. Also minor things about log messages.
But from the point of view of the logic it's ok.
Theres's also a question for @giacomini about supporting or not a configurable list of redirect URIs in order to allow different hostnames (different from the issuer).
| } | ||
|
|
||
| boolean isValid = dashboardConfigService.init(); | ||
| if (!isValid) { |
There was a problem hiding this comment.
checking the code of the service, I see false is returned after an Exception is thrown on db entry creation. I think we can raise another error here saying something like "Error during dashboard client initialization. Startup failed. Please check the logged error."
| public void run(ApplicationArguments args) { | ||
| if (!dashboardConfigService.isEnabled()) { | ||
| LOG.info( | ||
| "Dashboard client is disabled, skipping checks for the dashboard client properties and the presence of the record for the dashboard client"); |
There was a problem hiding this comment.
I'd simplify this message into "Skipping dashboard client initialization." The perfect message should include something like "Read more info about here: " + URL of an updated IAM documentation.
iam-login-service/src/main/java/it/infn/mw/iam/dashboard/DashboardConfigService.java
Show resolved
Hide resolved
| Optional<ClientDetailsEntity> dashboardRecord = clientRepository.findByClientId(clientId); | ||
|
|
||
| if (!dashboardRecord.isPresent()) { | ||
| LOG.info("The client record for dashboard does not exist. Creating record with default configuration..."); |
There was a problem hiding this comment.
"Dashboard client does not exist and it will be created."
| ClientDetailsEntity client = dashboardRecord.get(); | ||
| boolean isValid = checkRecordConfiguration(client, clientSecret, url); | ||
| if (!isValid) { | ||
| LOG.warn("The record is not properly configured. Updating Dashboard client."); |
There was a problem hiding this comment.
"Changes on default dashboard client configuration found: restoring expected configuration."
I know it's a database record but I'd like to communicate an higher level concept to the final admins :)
| } | ||
|
|
||
| private boolean usesClientSecretBasicAuth(ClientDetailsEntity client) { | ||
| return client.getTokenEndpointAuthMethod().equals(AuthMethod.SECRET_BASIC); |
| } | ||
|
|
||
| private boolean usesPKCES256(ClientDetailsEntity client) { | ||
| return client.getCodeChallengeMethod().getName().equals(PKCEAlgorithm.S256.toString()); |
| } | ||
|
|
||
| private boolean hasValidClientSecret(ClientDetailsEntity client, String clientSecret) { | ||
| return client.getClientSecret().equals(clientSecret); |
| } | ||
|
|
||
| private boolean hasAllRequiredScopes(ClientDetailsEntity client) { | ||
| return client.getScope().containsAll(DASHBOARD_SCOPES); |
There was a problem hiding this comment.
I'd add a safer:
client.getScope() != null && before
to protect from null pointer exceptions
| client.setRedirectUris(Set.of(url)); | ||
| client.setTokenEndpointAuthMethod(AuthMethod.SECRET_BASIC); | ||
|
|
||
| clientRepository.save(client); |
There was a problem hiding this comment.
We're not using client service so we need to add a ClientUpdatedEvent here
There was a problem hiding this comment.
Or simply use clientService.updateClient() which also evicts cached values
5 participants
No description provided.