-
Notifications
You must be signed in to change notification settings - Fork 2
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
Migration to role group #71
base: main
Are you sure you want to change the base?
Conversation
748fb42
to
65fd7fb
Compare
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.
Some of the comments i need to explain, som lets have call about it.
src/main/java/cz/cvut/kbss/study/security/SecurityConstants.java
Outdated
Show resolved
Hide resolved
src/main/java/cz/cvut/kbss/study/security/CustomSwitchUserFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/cz/cvut/kbss/study/security/SecurityConstants.java
Outdated
Show resolved
Hide resolved
if (!currentUser.getTypes().contains(Vocabulary.s_c_administrator) | ||
&& (!instance.getTypes().equals(currentUser.getTypes()) || (instance.getInstitution() != null | ||
if (!currentUser.isAdmin() | ||
&& (!instance.getRoleGroup().getRoles().equals(currentUser.getRoleGroup().getRoles()) || (instance.getInstitution() != null |
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.
Wow, not sure if I understand role of this condition now or before, but I am little worried that we need compare roleGroups here instead. Otherwise we would persist different roleGroups in internal-auth profile.
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.
? (it seems to me that this code is used for both keycloak-auth and internal-auth)
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.
src/test/java/cz/cvut/kbss/study/persistence/dao/PatientRecordDaoTest.java
Outdated
Show resolved
Hide resolved
6beabca
to
3d722f7
Compare
@blcham |
3d722f7
to
f736a00
Compare
0f248d0
to
5d9f9aa
Compare
While implementing the logic to support the Keycloak profile, I encountered a problem. When the application extracts roles from the Keycloak claims, it creates a new instance of RoleGroup and adds these roles to this empty group. The issue is that this group does not exist in the database, and a cascade strategy for this role group is not provided because we don’t need it. As a result, when the current user updates or creates something, this empty RoleGroup with the roles is in the transactional context, but the database does not know how to manage it since this RoleGroup does not exist and no cascade strategy is provided, which leads to an error. I have one idea on how to solve this: retrieve the RoleGroup from the token claim(not roles), attempt to find it in the database, and then add it to the user. What do you think? Do you have any other ideas? |
Not sure if I understand. Isn't it possible just to ignore persistence of RoleGroup in keycloak profile? E.g. in preUpdate method? |
@blcham |
631a0b3
to
349001c
Compare
8335db5
to
467b0f7
Compare
… exception if the provided role does not exist
http-nio-8080-exec-7] WARN o.s.w.s.m.s.DefaultHandlerExceptionResolver - Resolved [org.springframework.http.converter.HttpMessageNotWritableException: Could not write JSON: Cannot invoke "cz.cvut.kbss.study.model.RoleGroup.getRoles()" because "this.roleGroup" is null]
467b0f7
to
d23dead
Compare
@palagdan so i guess to simulate the error i can change the institution of the user and then save right? |
Resolves partially kbss-cvut/record-manager-ui#202