PIMS-1718: Keycloak not updating role #2434
Conversation
LawrenceLau2020
requested review from
Sharala-Perumal,
dbarkowsky,
TaylorFries,
GrahamS-Quartech and
ManishSihag
as code owners
|
🚀 Deployment Information The Express API Image has been built with the tag: |
|
Code Climate has analyzed commit 5c48d2f and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 94.4%. View more on Code Climate. |
| @@ -272,6 +273,7 @@ const updateUser = async (user: DeepPartial<User>) => { | |||
| ...user, | |||
| DisplayName: `${user.LastName}, ${user.FirstName}`, | |||
| }); | |||
| await KeycloakService.updateKeycloakUserRoles(resource.Username, [roleName]); | |||
There was a problem hiding this comment.
We need to check if a role was even included. If this is called with any area other than the status update section, the role is not included, and that user's roles are removed from Keycloak (but not PIMS).
There was a problem hiding this comment.
good catch!
| // Ensure existingRolesResponse is an array of roles | ||
| let existingRoles: IKeycloakRole[] = []; | ||
| if (Array.isArray(existingRolesResponse)) { | ||
| existingRoles = existingRolesResponse; | ||
| } else if (existingRolesResponse && typeof existingRolesResponse === 'object') { | ||
| const response = existingRolesResponse as { data?: IKeycloakRole[] }; // Type assertion | ||
| existingRoles = response.data || []; | ||
| } else { | ||
| throw new Error('Unexpected structure of existingRolesResponse'); | ||
| } |
There was a problem hiding this comment.
I think we could avoid all of these checks if we just wrapped most of this function in a try/catch block and handled the case if the getKeycloakUserRoles threw returned an error.
There was a problem hiding this comment.
Then we also wouldn't have to change the loops below.
There was a problem hiding this comment.
Made changes, ready for another review.
…eycloakUserRoles function
There was a problem hiding this comment.
Working well for me. Tried experimenting with not waiting for the Keycloak update to finish in an attempt to make it snappier. Unfortunately, we can't throw an error if we do that. Seemed more important to keep the error message visible.
Edited the error message thrown when a Keycloak user isn't found. Hopefully it's clearer now for frontend users.
🎯 Summary
PIMS-1718:
When changing the user’s role in the frontend, the role in the Keycloak SSO was not reflecting the change to the role in the database.
Updating the keycloak and user related services to allow for updating the user's role in the Keycloak SSO app.
This may need more work to handle errors when trying to save changes to users who don't exist in the Keycloak instance which will trigger this error message:
Submission failed with code 500. Message: Error: keycloakService.updateKeycloakUserRoles: keycloakService.getKeycloakUserRoles: undefined🔰 Checklist