Conversation
enricovianello
added
the
component/db
...c/main/java/it/infn/mw/iam/api/client/management/service/DefaultClientManagementService.java
Outdated
Show resolved
Hide resolved
iam-login-service/src/main/java/it/infn/mw/iam/config/security/MitreSecurityConfig.java
Outdated
Show resolved
Hide resolved
iam-login-service/src/main/java/it/infn/mw/iam/config/security/MitreSecurityConfig.java
Outdated
Show resolved
Hide resolved
...in/java/it/infn/mw/iam/api/client/registration/service/DefaultClientRegistrationService.java
Outdated
Show resolved
Hide resolved
iam-login-service/src/main/java/it/infn/mw/iam/config/AuthorizationServerConfig.java
Outdated
Show resolved
Hide resolved
iam-login-service/src/main/java/it/infn/mw/iam/config/security/IamApiSecurityConfig.java
Outdated
Show resolved
Hide resolved
| } else if (isNull(client.getClientSecret())) { | ||
| client.setClientSecret(defaultsService.generateClientSecret()); | ||
| } else { | ||
| newClient.setClientSecret(defaultsService.generateClientSecret()); |
There was a problem hiding this comment.
In quali casi entriamo in questo else?
There was a problem hiding this comment.
è da rimuovere, altrimenti genera un nuovo secret ogni volta che modifichiamo il client
There was a problem hiding this comment.
In some cases, when updating client data from the dashboard, the dashboard doesn't know the new ClientSecret hash, so it's updated with the old value. We don't want the user to change the secret or secret hash from the client update, but only from the new secret request.
There was a problem hiding this comment.
here's definitely a better way to write this check, but I discovered it after a bug search and it gave me bad credentials. I'll finish it in the next few days, or if anyone wants to fix it...
There was a problem hiding this comment.
I would preferably identify the "some cases" and properly address them with specific checks rather than a generic else, if there is no a valid reason to do it.
...ogin-service/src/main/java/it/infn/mw/iam/config/security/IamTokenEndointSecurityConfig.java
Outdated
Show resolved
Hide resolved
...service/src/main/java/it/infn/mw/iam/core/expression/IamMethodSecurityExpressionHandler.java
Show resolved
Hide resolved
...login-service/src/main/java/it/infn/mw/iam/core/expression/IamSecurityExpressionMethods.java
Show resolved
Hide resolved
...c/main/java/it/infn/mw/iam/api/client/management/service/DefaultClientManagementService.java
Show resolved
Hide resolved
iam-login-service/src/main/java/it/infn/mw/iam/api/client/service/DefaultClientService.java
Outdated
Show resolved
Hide resolved
iam-login-service/src/main/java/it/infn/mw/iam/config/AuthorizationServerConfig.java
Outdated
Show resolved
Hide resolved
iam-persistence/src/main/java/db/migration/mysql/V109__HashClientSecret.java
Outdated
Show resolved
Hide resolved
| }); | ||
| return res; | ||
|
|
||
| var res = () => { |
There was a problem hiding this comment.
| var res = () => { | |
| const res = () => { |
| return res; | ||
|
|
||
| var res = () => { | ||
| var modalInstance = $uibModal.open({ |
There was a problem hiding this comment.
| var modalInstance = $uibModal.open({ | |
| const modalInstance = $uibModal.open({ |
| modalInstance.result.then(function (selectedItem) { | ||
| $ctrl.selected = selectedItem; | ||
| }, function () { | ||
| console.log('Dialog dismissed at: ' + new Date()); |
There was a problem hiding this comment.
Why this log on the browser console?
| }, function () { | ||
| console.log('Dialog dismissed at: ' + new Date()); | ||
| }); | ||
| return res; |
There was a problem hiding this comment.
You are returning lambda function, is it correct?
| return res; | ||
| } | ||
|
|
||
| var modalSecret = $uibModal.open({ |
There was a problem hiding this comment.
| var modalSecret = $uibModal.open({ | |
| const modalSecret = $uibModal.open({ |
| assertEquals(client.getIdTokenValiditySeconds(), clientDB.get().getIdTokenValiditySeconds()); | ||
| assertEquals(client.getDeviceCodeValiditySeconds(), clientDB.get().getDeviceCodeValiditySeconds()); | ||
| assertEquals(client.getDeviceCodeValiditySeconds(), | ||
| clientDB.get().getDeviceCodeValiditySeconds()); |
There was a problem hiding this comment.
You are calling clientDB.get() at least four times.
Call it only once a store the result in a variable
| clientDB = clientRepo.findByClientId(client.getClientId()); | ||
| assertEquals(client.getAccessTokenValiditySeconds(), clientDB.get().getAccessTokenValiditySeconds()); | ||
| assertEquals(client.getRefreshTokenValiditySeconds(), clientDB.get().getRefreshTokenValiditySeconds()); | ||
| assertEquals(client.getAccessTokenValiditySeconds(), |
|
|
||
| String responseJson = mvc | ||
| .perform(post(ClientRegistrationApiController.ENDPOINT).contentType(APPLICATION_JSON) | ||
| .content(clientJson)) |
There was a problem hiding this comment.
| .content(clientJson)) | |
| .content(clientJson)) |
| RegisteredClientDTO client = mapper.readValue(responseJson, RegisteredClientDTO.class); | ||
| String clientSecret = client.getClientSecret(); | ||
|
|
||
| final String url = |
There was a problem hiding this comment.
I think that final is unnecessary in this context
|
|
||
| String responseJson = mvc | ||
| .perform(post(ClientManagementAPIController.ENDPOINT).contentType(APPLICATION_JSON) | ||
| .content(clientJson)) |
There was a problem hiding this comment.
| .content(clientJson)) | |
| .content(clientJson)) |
(don't copy/past to much)
| RegisteredClientDTO client = mapper.readValue(responseJson, RegisteredClientDTO.class); | ||
| String clientSecret = client.getClientSecret(); | ||
|
|
||
| final String url = |
| ClientDetailsEntity client = | ||
| clientRepo.findByClientId(clientId).orElseThrow(ClientSuppliers.clientNotFound(clientId)); | ||
|
|
||
| final String url = |
| @WithMockUser(username = "test", roles = {"USER"}) | ||
| void secretRotationFailsForOtherUsers() throws Exception { | ||
|
|
||
| assertSecretRotationForbidden("client"); |
There was a problem hiding this comment.
Maybe we can use a better client id rather than "client"
| @WithMockUser(username = "non-existent-user", roles = {"USER"}) | ||
| void secretRotationFailsWhenUserNotFound() throws Exception { | ||
|
|
||
| assertSecretRotationForbidden("client-cred"); |
| .as(RegisteredClientDTO.class); | ||
|
|
||
| assertThat(getResponse.getClientSecret(), is(registerResponse.getClientSecret())); | ||
| assertThat(new IamClientSecretEncoder().matches(registerResponse.getClientSecret(), getResponse.getClientSecret()), is(true)); |
There was a problem hiding this comment.
Can we split this line in multiple lines? Is very hard to read
| repo.deleteAll(); | ||
| clientService.unlinkClientFromAccount(clientDetailsService.loadClientByClientId(TEST_CLIENT_ID), | ||
| accountRepo.findByUsername(TEST_ADMIN).get()); | ||
| clientService.unlinkClientFromAccount(clientDetailsService.loadClientByClientId(TEST_CLIENT_ID), |
There was a problem hiding this comment.
Please call clientDetailsService.loadClientByClientId(TEST_CLIENT_ID) only once and store the result.
This very long calls are really hard to read
| @Test | ||
| @WithMockUser(roles = {"ADMIN", "USER"}, username = TEST_ADMIN) | ||
| public void testIsClientOwnerIsAdmin() { | ||
| clientService.linkClientToAccount(clientDetailsService.loadClientByClientId(TEST_CLIENT_ID), |
| @WithMockUser(roles = {"ADMIN", "USER"}, username = "test_200") | ||
| public void testIsClientOwnerIsUser() { | ||
| String owner = "test_200"; | ||
| clientService.linkClientToAccount(clientDetailsService.loadClientByClientId(TEST_CLIENT_ID), |
There was a problem hiding this comment.
I do not repeat again, check also the next lines
| throws JOSEException { | ||
|
|
||
| JWSSigner signer = new MACSigner(CLIENT_ID_SECRET_JWT_SECRET); | ||
| ClientDetailsEntity client = clientRepo.findByClientId(CLIENT_ID_SECRET_JWT).orElseThrow(); |
There was a problem hiding this comment.
Is the .orElseThrow() call necessary? On the next line, if the client does not exist (is null) the client.getClientSecret() will throw a NullPointerException
| client.setScope(Sets.newHashSet("openid", "profile", "email")); | ||
| clientRepo.save(client); | ||
|
|
||
| try { |
There was a problem hiding this comment.
Cannot understand this try without the catch.
If we have an error here, this will disappear in the cosmic galactic void without any evidence
iam-persistence/src/main/java/db/migration/mysql/V110__HashClientSecret.java
Show resolved
Hide resolved
|
|
||
| if (NONE.equals(newClient.getTokenEndpointAuthMethod())) { | ||
| newClient.setClientSecret(null); | ||
| } else if (isNull(request.getClientSecret()) && isNull(oldClient.getClientSecret())) { |
There was a problem hiding this comment.
Just curiosity: what is the difference between NONE.equals and isNull? Can we use the same check to be consistent?
| } else if (isNull(request.getClientSecret()) && isNull(oldClient.getClientSecret())) { | ||
| newClient.setClientSecret(defaultsService.generateClientSecret()); | ||
| } else { | ||
| // Direct updates are disabled. Changes must be made via secret reset process |
There was a problem hiding this comment.
What is a secret reset process? Do you mean calling the reset secret API endpoint?
And what do you mean with "direct updates?" Do you mean passing the secret in the PUT request to edit the client?
If I'm getting it right, we should add a test that checks that the secret is ignored from the PUT request or a bad request error is thrown
| doReturn(Optional.of(clientTest)).when(clientRepo).findByClientId(TEST_CLIENT_ID); | ||
| Optional<IamAccount> account = accountRepo.findByUsername(TEST_ADMIN); | ||
| ClientDetailsEntity clientTestUpdate = clientService.linkClientToAccount(clientTest, account.get()); | ||
|
|
| doReturn(Optional.of(clientTest)).when(clientRepo).findByClientId(TEST_CLIENT_ID); | ||
| Optional<IamAccount> account = accountRepo.findByUsername(owner); | ||
| ClientDetailsEntity clientTestUpdate = clientService.linkClientToAccount(clientTest, account.get()); | ||
|
|
| doReturn(Optional.of(clientTest)).when(clientRepo).findByClientId(TEST_CLIENT_ID); | ||
| Optional<IamAccount> account = accountRepo.findByUsername(owner); | ||
| ClientDetailsEntity clientTestUpdate = clientService.linkClientToAccount(clientTest, account.get()); | ||
|
|
6 participants
No description provided.