Migration to role group #71
Conversation
|
It should be working as expected. Now I just need to fix the frontend based on the new implementation of the backend. |
palagdan
force-pushed
the
migration-to-role-group
branch
from
748fb42 to
65fd7fb
Compare
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.
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.
? (it seems to me that this code is used for both keycloak-auth and internal-auth)
There was a problem hiding this comment.
The condition is true when:
- The user is not an admin, and
- Either the roles of the instance and the currentUser are different, or:
- The instance has an institution, and its key is different from the currentUser's institution key.
Why can't we just compare by URI, in case the user has the ROLE_USER role and tries to update another user? This condition seems too complicated.
Yes, it is used for both profiles.
src/test/java/cz/cvut/kbss/study/persistence/dao/PatientRecordDaoTest.java
Outdated
Show resolved
Hide resolved
palagdan
force-pushed
the
migration-to-role-group
branch
4 times, most recently
from
6beabca to
3d722f7
Compare
|
@blcham |
palagdan
force-pushed
the
migration-to-role-group
branch
from
3d722f7 to
f736a00
Compare
palagdan
force-pushed
the
migration-to-role-group
branch
3 times, most recently
from
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 |
blcham
force-pushed
the
migration-to-role-group
branch
2 times, most recently
from
631a0b3 to
349001c
Compare
blcham
force-pushed
the
migration-to-role-group
branch
3 times, most recently
from
467b0f7 to
d23dead
Compare
|
@palagdan so i guess to simulate the error i can change the institution of the user and then save right? |
|
@palagdan I added method to get user without security context |
palagdan
force-pushed
the
migration-to-role-group
branch
from
e599913 to
8e21f6d
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]
…out security context
blcham
force-pushed
the
migration-to-role-group
branch
from
8e7d3ec to
cd47a66
Compare
There was a problem hiding this comment.
@palagdan see my comments.
Please carefully go over each line containing Generator.generateUser(institutionAnother, this.roleGroupAdmin). I believe we do not want to have them there as they just might introduce condition in test that will not fail due to this restriction. Or did you do that because in those tests you do not want to test security related issues and you will replace this.roleGroupAdmin with something that will have all permissions?
| "hasAuthority('" + SecurityConstants.ROLE_ADMIN + "') " + | ||
| "or hasAuthority('" + SecurityConstants.ROLE_ADMIN + "') and @securityUtils.isMemberOfInstitution(#institutionKey)") |
There was a problem hiding this comment.
| "hasAuthority('" + SecurityConstants.ROLE_ADMIN + "') " + | |
| "or hasAuthority('" + SecurityConstants.ROLE_ADMIN + "') and @securityUtils.isMemberOfInstitution(#institutionKey)") | |
| "hasAuthority('" + SecurityConstants.ROLE_ADMIN + "') " + | |
| "or hasAuthority('" + SecurityConstants.ROLE_USER + "') and @securityUtils.isMemberOfInstitution(#institutionKey)") |
There was a problem hiding this comment.
I changed it back.
| this.securityUtils = securityUtils; | ||
| this.passwordEncoder = passwordEncoder; | ||
| this.userDao = userDao; | ||
| this.patientRecordDao = patientRecordDao; | ||
| this.email = email; | ||
| this.config = config; | ||
| this.em = em; |
There was a problem hiding this comment.
This is bad practice to include em in service layer ... you need to call userDao instead !
| User currentUser = securityUtils.getCurrentUser(); | ||
| em.detach(currentUser); |
There was a problem hiding this comment.
I was explaining that we could implement it like that but it was rather to explain how it work ...
I was hoping you would not implement it this way. But rather include method securityUtils.getCurrentUserName(). What is the reason to not implement it like that?
There was a problem hiding this comment.
@blcham
I tried implementing this method in SecurityUtils as you suggested, but the issue still occurred because, as you mentioned, we are retrieving the user within the same transaction. Only this solution resolved the problem
There was a problem hiding this comment.
Done, I removed em from the service layer and used securityUtils, but I still have some questions about why it works now..
| @Test | ||
| public void findByKeyReturnsActionWithPayload() { | ||
| Institution institution = Generator.generateInstitution(); | ||
| User user = Generator.generateUser(institution); | ||
|
|
||
| User user = Generator.generateUser(institution, this.roleGroupAdmin); |
There was a problem hiding this comment.
This is confusing. Why we now need roleGroupAdmin and it is not enough to have roleGroupUser? I would not elevate required permissions as we might get lost later when we will try to remove ADMINISTRATOR role. We would not understand why ADMIN role was needed here ....
Note that you are using it in many places within this file.
| @Test | ||
| public void persistedInstanceHasGeneratedUri(){ | ||
| final Institution institution = Generator.generateInstitution(); | ||
| final User user = Generator.generateUser(institution); | ||
| final RoleGroup roleGroupAdmin = Generator.generateRoleGroupWithRoles(Role.administrator); | ||
| final User user = Generator.generateUser(institution, roleGroupAdmin); |
There was a problem hiding this comment.
as explained in other comment we are elevating user's permission to ADMIN and i do not understand why. I suggest to use something like roleGroupUser instead.
| @BeforeEach | ||
| public void setUp() { | ||
| super.setUp(controller); | ||
| Institution institution = Generator.generateInstitution(); | ||
| User user = Generator.generateUser(institution); | ||
| this.roleGroupAdmin = Generator.generateRoleGroupWithRoles(Role.administrator); | ||
| User user = Generator.generateUser(institution, this.roleGroupAdmin); |
There was a problem hiding this comment.
as explained in other comment we are elevating user's permission to ADMIN and i do not understand why. I suggest to use something like roleGroupUser instead.
| public static final String USERNAME = "halsey"; | ||
| public static final String PASSWORD = "john117"; | ||
| public static final String EMAIL = "[email protected]"; | ||
|
|
||
| @BeforeEach | ||
| public void setUp() throws Exception { | ||
| Institution institution = Generator.generateInstitution(); | ||
| user = Generator.getUser(USERNAME, PASSWORD, "John", "Grant", EMAIL, institution); | ||
| this.roleGroupAdmin = Generator.generateRoleGroupWithRoles(Role.administrator); |
There was a problem hiding this comment.
as explained in other comment we are elevating user's permission to ADMIN and i do not understand why. I suggest to use something like roleGroupUser instead.
| @Test | ||
| public void loadUserByUsername() { | ||
| Institution institution = Generator.generateInstitution(); | ||
| institutionService.persist(institution); | ||
|
|
||
| User user = Generator.generateUser(institution); | ||
| User user = Generator.generateUser(institution, this.roleGroupAdmin); |
There was a problem hiding this comment.
as explained in other comment we are elevating user's permission to ADMIN and i do not understand why. I suggest to use something like roleGroupUser instead.
| @@ -95,10 +97,10 @@ private List<PatientRecord> generateRecordsToImport(User originalAuthor) { | |||
|
|
|||
| @Test | |||
| void importRecordsRetainsRecordProvenanceDataWhenCurrentUserIsAdmin() { | |||
| final User originalAuthor = Generator.generateUser(Generator.generateInstitution()); | |||
| final User originalAuthor = Generator.generateUser(Generator.generateInstitution(), this.roleGroupAdmin); | |||
There was a problem hiding this comment.
as explained in other comment we are elevating user's permission to ADMIN and i do not understand why. I suggest to use something like roleGroupUser instead.
| @@ -58,14 +59,17 @@ public class SecurityUtilsTest { | |||
|
|
|||
| private User user; | |||
|
|
|||
| private RoleGroup roleGroupAdmin; | |||
There was a problem hiding this comment.
as explained in other comment we are elevating user's permission to ADMIN and i do not understand why. I suggest to use something like roleGroupUser instead.
palagdan
force-pushed
the
migration-to-role-group
branch
from
62e97c3 to
9c56d49
Compare
|
I investigated the potential problem you described. I reviewed several classes across different layers (controllers, services, and DAOs) and tested using a user with only the "user" role. All tests produced the same results. The issue is that we don't test the @PreAuthorize methods in all cases, so the roles assigned to the user in these tests do not affect the outcome. For reference, see: How do I unit test Spring Security PreAuthorize/HasRole. I’ll work on understanding how to implement tests that also validate roles. In the example below, I show that the method getAllInstitutions has the following restriction:
|
Resolves partially kbss-cvut/record-manager-ui#202