Skip to content
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

Merged
merged 143 commits into from
Jan 12, 2024

Conversation

netroms
Copy link
Contributor

@netroms netroms commented Nov 16, 2023

Summary

This PR includes several big changes.

  1. Removal of the CurrentUserService and introduction of UserDetails (renamed from CurrentUserDetails), as an immutable DTO of the persisted User object. The UserDetails DTO purpose is to represent a “hydrated” and detached from persistence version of the User object during the user's “session”.

  2. Removal of getCurrentUserGroupInfo() as a method for keeping up-to-date state on User/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 keep getCurrentUserGroupInfo() in one place. See: InternalHibernateGenericStoreImpl.getSharingPredicates()

  3. Changing the HibernateIdentifiableObjectStore save/update methods to not accept NULL User/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.

  4. 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.

  5. Removal of deprecated Spring Security configuration methods as a preparation for Spring Security 6, which removes the deprecated code.

  6. 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.

  7. Moved all SecurityService methods into UserService to avoid cyclic dependency issues. Needs further refactoring to clean up unused methods etc.

Issues that requires further work/PRs:

  1. Refactor rest of the User object input parameter use into UserDetails. In places where this is still a problem, the usage of UserDetails.fromUser() is an indication that we have to convert back to a User object from UserDetails.
  2. Make tests setup of user related objects more consistent, fewer ways to do the same thing.
  3. Log out users when their UserGroup change, to refresh their UserDetails, see point 2 above.
  4. Upgrade to Spring Security 6
  5. Remove all @async usage and convert to jobs via job scheduler.
  6. Refactor redundant and not used methods, that were moved from SecurityService into UserService.
  7. To fix remaining cyclic dependency issues, a search for use of @lazy annotations is a good way to start.

@netroms netroms marked this pull request as draft November 16, 2023 17:17
@netroms netroms changed the title feat: upgrade to Spring 6 feat: Migrate away from deprecated Spring Security configuration Nov 17, 2023
Copy link

codecov bot commented Nov 21, 2023

Codecov Report

Attention: 486 lines in your changes are missing coverage. Please review.

Comparison is base (7ec1cf0) 66.46% compared to head (1ab6228) 66.46%.

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     
Flag Coverage Δ
integration 50.01% <58.76%> (-0.26%) ⬇️
integration-h2 32.57% <44.68%> (+0.05%) ⬆️
unit 30.37% <9.81%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...n/java/org/hisp/dhis/category/CategoryService.java 100.00% <ø> (ø)
...ava/org/hisp/dhis/common/BaseAnalyticalObject.java 74.43% <100.00%> (ø)
.../java/org/hisp/dhis/common/IdentifiableObject.java 0.00% <ø> (ø)
...rg/hisp/dhis/common/IdentifiableObjectManager.java 100.00% <ø> (ø)
...is-api/src/main/java/org/hisp/dhis/common/UID.java 90.90% <100.00%> (+0.90%) ⬆️
...p/dhis/common/adapter/BaseIdentifiableObject_.java 0.00% <ø> (ø)
.../hisp/dhis/configuration/ConfigurationService.java 100.00% <ø> (ø)
...va/org/hisp/dhis/dataelement/DataElementStore.java 100.00% <ø> (ø)
...i/src/main/java/org/hisp/dhis/dataset/DataSet.java 79.61% <ø> (ø)
...java/org/hisp/dhis/hibernate/HibernateService.java 100.00% <100.00%> (ø)
... and 249 more

... and 148 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ec1cf0...1ab6228. Read the comment docs.

…_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
Copy link

@github-advanced-security github-advanced-security bot left a 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.


/**
* @author Viet Nguyen <[email protected]>
*/
@Component
Copy link
Contributor

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)

Copy link
Contributor

@jbee jbee left a 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 in UserSetting should be explained because there is also a User field
  • agree "system-process_" + CodeGenerator.generateUid(); but should we make this constant for an SuperUser 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 a GuestUser class instead so UserDetails is never null but its methods might return null?
  • saw a couple of // TODO: MAS: comments
  • getSharingPredicates resolves CurrentUserGroupInfo, is this "expensive"? we use this method quite a bit
  • in some places the CurrentUserUtil.getCurrentUserDetails() is checked for null and then a CurrentUserUtil.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 than UserDetailsImpland SuperUser? e.g. DhisOidcUser, can't they just create instances of UserDetailsImpl? that is easier to understand and less risk of having unwanted state in the session
  • setActiveLinkedAccounts uses Instant.now() two times instead of using same instance => could be different value
  • UserDetails.fromUser(...) should be a todo for future PR to eliminate as much as possible by using UserDetails higher up (caller)
  • JdbcEventStore#getEventCount uses UserDetails.fromUser(user) as a query parameter, that seems wrong?! those should be a persisted object like the User?!
  • EventImporterUserService#getCurrentUser() seems like an addition that should be deprecated for be replaced with use of UserDetails on the caller side?
  • @CurrentUser UserDetailsImpl in controller parameters should be @CurrentUser UserDetails => we know how to fetch the instance in the resolver
  • CurrentUserHandlerMethodArgumentResolver and CurrentUserInfoHandlerMethodArgumentResolver seem to be duplicates now after we merged the old user and userinfo objects
  • CurrentUserHandlerMethodArgumentResolver: if (type == UserDetailsImpl.class) { => if (type == UserDetails.class) {, also the throw new NotAuthenticatedException(); logic further below done for User should be the same for UserDetails ?!

Copy link
Contributor

@david-mackessy david-mackessy left a 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

UserDetails has the same name as its supertype
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

This method overrides
JobCreationHelper.create
; it is advisable to add an Override annotation.
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

This method overrides
JobCreationHelper.create
; it is advisable to add an Override annotation.
}

@Transactional
public String create(JobConfiguration config, MimeType contentType, InputStream content)

Check notice

Code scanning / CodeQL

Missing Override annotation Note

This method overrides
JobCreationHelper.create
; it is advisable to add an Override annotation.
private final FileResourceService fileResourceService;

@Transactional
public String create(JobConfiguration config) throws ConflictException {

Check notice

Code scanning / CodeQL

Missing Override annotation Note

This method overrides
JobCreationHelper.create
; it is advisable to add an Override annotation.
"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

Invoking
Hashing.md5
should be avoided because it has been deprecated.
Copy link

sonarcloud bot commented Jan 11, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
1 Security Hotspot
0.0% Coverage on New Code
0.3% Duplication on New Code

See analysis details on SonarCloud

@netroms netroms changed the title feat: Migrate away from deprecated Spring Security configuration feat: Refactoring of CurrentUserService and introduction of UserDetails and more Jan 12, 2024
@netroms netroms merged commit 3a58d32 into master Jan 12, 2024
17 checks passed
@netroms netroms deleted the DHIS2-16169_2.41 branch January 12, 2024 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants