-
Notifications
You must be signed in to change notification settings - Fork 354
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
feat: Refactoring of CurrentUserService and introduction of UserDetails and more #15727
Conversation
...-web-api/src/main/java/org/hisp/dhis/webapi/security/config/DhisWebApiWebSecurityConfig.java
Fixed
Show fixed
Hide fixed
dhis-2/dhis-api/src/main/java/org/hisp/dhis/common/IdentifiableObjectManager.java
Dismissed
Show dismissed
Hide dismissed
dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/user/DefaultUserService.java
Fixed
Show fixed
Hide fixed
dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/user/DefaultUserService.java
Fixed
Show fixed
Hide fixed
dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/user/DefaultUserService.java
Fixed
Show fixed
Hide fixed
...-web-api/src/main/java/org/hisp/dhis/webapi/security/config/DhisWebApiWebSecurityConfig.java
Fixed
Show fixed
Hide fixed
...-web-api/src/main/java/org/hisp/dhis/webapi/security/config/DhisWebApiWebSecurityConfig.java
Fixed
Show fixed
Hide fixed
...web-commons/src/main/java/org/hisp/dhis/security/config/DhisWebCommonsWebSecurityConfig.java
Fixed
Show fixed
Hide fixed
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #15727 +/- ##
============================================
- Coverage 66.46% 66.46% -0.01%
- Complexity 31600 31611 +11
============================================
Files 3508 3513 +5
Lines 130769 131118 +349
Branches 15254 15298 +44
============================================
+ Hits 86922 87149 +227
- Misses 36769 36838 +69
- Partials 7078 7131 +53
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 148 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
…_2.41 # Conflicts: # dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/dataapproval/hibernate/HibernateDataApprovalStore.java # dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/organisationunit/DefaultOrganisationUnitService.java # dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/dataapproval/DataApprovalStoreIntegrationTest.java # dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/MvcTestConfig.java # dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/SystemController.java
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.
CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
…_2.41 # Conflicts: # dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/datastore/DefaultDatastoreService.java # dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/tracker/imports/TrackerImportController.java
|
||
/** | ||
* @author Viet Nguyen <[email protected]> | ||
*/ | ||
@Component |
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.
has the removal of this as a spring component been checked? (where is was originally injected elsewhere in the code)
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.
- commented code in
SpringSecurityActionAccessResolver
- is there a benefit to do
UserDetails extends org.springframework.security.core.userdetails.UserDetails
, do we need all spring methods exposed? try to keep "our" API narrow - the role of new field
userId
inUserSetting
should be explained because there is also aUser
field - agree
"system-process_" + CodeGenerator.generateUid();
but should we make this constant for anSuperUser
instance? - maybe not use
CurrentUserUtil.getCurrentUserDetails()
within a stream directly but resolve it before going into a stream operation? - thinking about situations like
user != null ? user.getUsername() : "[None]",
- would it make sense to have aGuestUser
class instead soUserDetails
is never null but its methods might return null? - saw a couple of
// TODO: MAS:
comments getSharingPredicates
resolvesCurrentUserGroupInfo
, is this "expensive"? we use this method quite a bit- in some places the
CurrentUserUtil.getCurrentUserDetails()
is checked for null and then aCurrentUserUtil.getCurrentUserDetails().something()
is done, it might be cheap but we should avoid that and store the instance in a field also for consistency as in theory the result of the call could change between multiple calls - why do we need more implementations of
UserDetails
thanUserDetailsImpl
andSuperUser
? e.g.DhisOidcUser
, can't they just create instances ofUserDetailsImpl
? that is easier to understand and less risk of having unwanted state in the session setActiveLinkedAccounts
usesInstant.now()
two times instead of using same instance => could be different valueUserDetails.fromUser(...)
should be a todo for future PR to eliminate as much as possible by usingUserDetails
higher up (caller)JdbcEventStore#getEventCount
usesUserDetails.fromUser(user)
as a query parameter, that seems wrong?! those should be a persisted object like theUser
?!EventImporterUserService#getCurrentUser()
seems like an addition that should be deprecated for be replaced with use ofUserDetails
on the caller side?@CurrentUser UserDetailsImpl
in controller parameters should be@CurrentUser UserDetails
=> we know how to fetch the instance in the resolverCurrentUserHandlerMethodArgumentResolver
andCurrentUserInfoHandlerMethodArgumentResolver
seem to be duplicates now after we merged the old user and userinfo objectsCurrentUserHandlerMethodArgumentResolver
:if (type == UserDetailsImpl.class) {
=>if (type == UserDetails.class) {
, also thethrow new NotAuthenticatedException();
logic further below done forUser
should be the same forUserDetails
?!
.../dhis-service-core/src/main/java/org/hisp/dhis/user/hibernate/HibernateUserSettingStore.java
Outdated
Show resolved
Hide resolved
...s-service-dxf2/src/main/java/org/hisp/dhis/dxf2/datavalueset/DefaultDataValueSetService.java
Show resolved
Hide resolved
dhis-2/dhis-support/dhis-support-test/src/main/java/org/hisp/dhis/DhisConvenienceTest.java
Outdated
Show resolved
Hide resolved
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.
nice work Morten, It was a big undertaking :)
I would mention UserDetails somewhere in the PR title as that is probably the biggest part of this change.
…_2.41 # Conflicts: # dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/CrudControllerIntegrationTest.java
…_2.41 # Conflicts: # dhis-2/dhis-services/dhis-service-administration/src/main/java/org/hisp/dhis/maintenance/DefaultMaintenanceService.java # dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/datavalue/DefaultDataValueService.java # dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/deduplication/hibernate/HibernatePotentialDuplicateStore.java # dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/program/DefaultEventService.java # dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/trackedentity/DefaultTrackedEntityChangeLogService.java # dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/trackedentity/DefaultTrackedEntityService.java # dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/trackedentityattributevalue/DefaultTrackedEntityAttributeValueService.java # dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/trackedentitydatavalue/DefaultTrackedEntityDataValueChangeLogService.java # dhis-2/dhis-services/dhis-service-dxf2/src/main/java/org/hisp/dhis/dxf2/deprecated/tracker/importer/EventManager.java # dhis-2/dhis-services/dhis-service-tracker/src/main/java/org/hisp/dhis/tracker/export/trackedentity/DefaultTrackedEntityService.java # dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/tracker/imports/bundle/TrackedEntityAttributeValueChangeLogTest.java # dhis-2/dhis-test-web-api/src/test/java/org/hisp/dhis/webapi/controller/CrudControllerIntegrationTest.java
import org.hisp.dhis.common.BaseIdentifiableObject; | ||
import org.springframework.security.core.GrantedAuthority; | ||
|
||
public interface UserDetails extends org.springframework.security.core.userdetails.UserDetails { |
Check notice
Code scanning / CodeQL
Class has same name as super class Note
org.springframework.security.core.userdetails.UserDetails
} | ||
|
||
@Transactional(propagation = Propagation.REQUIRES_NEW) | ||
public String create(JobConfiguration config, MimeType contentType, InputStream content) |
Check notice
Code scanning / CodeQL
Missing Override annotation Note
JobCreationHelper.create
private final FileResourceService fileResourceService; | ||
|
||
@Transactional(propagation = Propagation.REQUIRES_NEW) | ||
public String create(JobConfiguration config) throws ConflictException { |
Check notice
Code scanning / CodeQL
Missing Override annotation Note
JobCreationHelper.create
} | ||
|
||
@Transactional | ||
public String create(JobConfiguration config, MimeType contentType, InputStream content) |
Check notice
Code scanning / CodeQL
Missing Override annotation Note
JobCreationHelper.create
private final FileResourceService fileResourceService; | ||
|
||
@Transactional | ||
public String create(JobConfiguration config) throws ConflictException { |
Check notice
Code scanning / CodeQL
Missing Override annotation Note
JobCreationHelper.create
"job_input_data_for_" + uid, | ||
contentType.toString(), | ||
data.length, | ||
ByteSource.wrap(data).hash(Hashing.md5()).toString(), |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note
Hashing.md5
|
Summary
This PR includes several big changes.
Removal of the
CurrentUserService
and introduction ofUserDetails
(renamed fromCurrentUserDetails
), as an immutable DTO of the persisted User object. TheUserDetails
DTO purpose is to represent a “hydrated” and detached from persistence version of the User object during the user's “session”.Removal of
getCurrentUserGroupInfo()
as a method for keeping up-to-date state onUser
/UserGroup
relationship. This is no longer encouraged and developers should rather rely on hydrated and up to date UserDetail representation of the User and it's relationships. This however is not currently (in this PR) 100% finished as we need to invalidate logged-in users the same way we invalidate users when role membership etc. change. For the future, we will want to update/rebuild the in memory representation (UserDetails
) on such changes. For now, we need to keepgetCurrentUserGroupInfo()
in one place. See:InternalHibernateGenericStoreImpl.getSharingPredicates()
Changing the
HibernateIdentifiableObjectStore
save/update methods to not accept NULLUser
/UserDetails
parameters. This is only part of an ongoing refactoring to remove code relying on a NULL user object parameters to implicitly refer to a superuser.Almost all tests now have a proper admin user during test setup. This still need more refactoring to be more consistent through all tests. We want to have fewer choices on making users during test setup.
Removal of deprecated Spring Security configuration methods as a preparation for Spring Security 6, which removes the deprecated code.
Introduction of
SystemUser
for internal use. This should be used when there are async or internal jobs doing work. Note for further refactoring that we should remove usage of Spring's@async
for async jobs, and rather rely on the job scheduler for such work.Moved all
SecurityService
methods intoUserService
to avoid cyclic dependency issues. Needs further refactoring to clean up unused methods etc.Issues that requires further work/PRs:
UserDetails
. In places where this is still a problem, the usage ofUserDetails.fromUser()
is an indication that we have to convert back to a User object fromUserDetails
.UserGroup
change, to refresh theirUserDetails
, see point 2 above.@async
usage and convert to jobs via job scheduler.SecurityService
intoUserService
.@lazy
annotations is a good way to start.