feat: Users are no longer deleted immediately, but marked as deleted.… by jekutzsche · Pull Request #761 · iris-connect/iris-client

jekutzsche · 2022-04-26T09:42:24Z

… The marked users can no longer be used and are no longer displayed. However, the data is still available, for example, for working with the audit logs. After all references to the users have been deleted in due time, the users are also permanently deleted.

Entity and database extended by the fields necessary for soft-delete and user locking.
The repository now contains methods that take the deletedAt flag into account.
Extends the loadUserByUsername method to also take into account the enabled and locked flags.
Adds the locked status to the handling of an update. This allows the status to be set initially with the insert and changed later via the update.
A job has been added to permanently delete users as soon as this is possible. If there is an error with the referential integrity, then this is ignored for a configurable period of time, since data such as the audit logs must be stored for a certain time and have references.
User mapper extended to also map from UserInsertDTO to UserAccount and manual mapping removed.
Code of the DTOs rewritten to Records and thus greatly simplified.
InitialAdminLoader has been moved to the users package, because this class has more to do with user management than with authorization.

Refs iris-connect/iris-backlog#235

github-actions · 2022-04-26T09:48:27Z

Unit Test Results

  54 files +2   54 suites +2 3m 25s ⏱️ -44s
319 tests +9 319 ✔️ +9 0 💤 ±0 0 ❌ ±0

Results for commit aa21b9e. ± Comparison against base commit 9c2857d.

This pull request removes 2 and adds 11 tests. Note that renamed tests count towards both.

iris.client_bff.auth.db.InitialAdminLoaderTest ‑ shouldCreateAnAdminUserIfItDoesNotExist
iris.client_bff.auth.db.InitialAdminLoaderTest ‑ shouldNotCreateAnAdminUserIfItDoesExist

iris.client_bff.users.InitialAdminLoaderTest ‑ shouldCreateAnAdminUserIfItDoesNotExist
iris.client_bff.users.InitialAdminLoaderTest ‑ shouldNotCreateAnAdminUserIfItDoesExist
iris.client_bff.users.UserAnonymizationJobIntegrationTests ‑ processUsers_WithRefAfterWarnPeriod_AnonymizeNothingWithWarning
iris.client_bff.users.UserAnonymizationJobIntegrationTests ‑ processUsers_WithRefBeforeWarnPeriod_AnonymizeNothingWithoutWarning
iris.client_bff.users.UserAnonymizationJobIntegrationTests ‑ processUsers_WithoutMarked_AnonymizeNothing
iris.client_bff.users.UserAnonymizationJobIntegrationTests ‑ processUsers_WithoutRef_Anonymize
iris.client_bff.users.UserAnonymizationJobTimeBasedIntegrationTests ‑ processUsers_WithRefAfterWarnPeriod_Anonymize
iris.client_bff.users.UserAnonymizationJobTimeBasedIntegrationTests ‑ processUsers_WithRefBeforeWarnPeriod_AnonymizeNothingWithoutWarning
iris.client_bff.users.UserAnonymizationJobTimeBasedIntegrationTests ‑ processUsers_WithoutMarked_AnonymizeNothing
iris.client_bff.users.UserAnonymizationJobTimeBasedIntegrationTests ‑ processUsers_WithoutRef_AnonymizeNothingWithoutWarning
…

♻️ This comment has been updated with latest results.

lucky-lusa · 2022-04-29T07:39:13Z

+interface UserAccountsRepository extends JpaRepository<UserAccount, UserAccountIdentifier> {

-	long countByRole(UserRole role);
+	Optional<UserAccount> findByUserNameAndDeletedAtIsNull(String userName);


For readability reasons, I would suggest replacing AndDeledetAtIsNull with findActiveUserByUsername.

You are absolutely right in principle! Even if we have to start with @Query, the readability of the method calls is more important, thanks.

I would rename all methods according to a scheme. However, I am not yet happy with findActiveUser..., since users can also be locked, so they are not "active", but should be returned by the methods.

As discussed, I omit "Active", resulting in a name like findUserByUsername.

lucky-lusa · 2022-04-29T07:41:08Z

+	@Scheduled(cron = "${iris.client.user.delete-cron:-}")
+	void deleteUsers() {
+
+		var deletedUsers = users.findAllByDeletedAtIsNotNull();


Same here. findAllDeleted() might be the better choice.

This is fine, thanks.

lucky-lusa · 2022-04-29T07:42:25Z

+	public UserAccount markDeleted() {
+
+		setDeletedAt(Instant.now());
+		setUserName(UUID.randomUUID().toString());


Or just use the users UUID?

This is a good idea.

mad-nuts · 2022-05-02T13:33:37Z

-	long countByRole(UserRole role);
+	String SELECT_BASE = "SELECT u from UserAccount u where ";
+
+	@Query(SELECT_BASE + "u.userName = :userName AND u.deletedAt IS NULL")


As the userName is replaced with the id on soft delete this could cause issues / be confusing when calling "findUserByUsername", as the original userName doesn't exist anymore.

Basically, it is double that the users are no longer found, but it is intended that way. The "deleted" users are no longer directly usable. The records should only be used to be able to resolve the data to the references (IDs).

At least that was the idea behind it, which may well be debatable.

… The marked users can no longer be used and are no longer displayed. However, the data is still available, for example, for working with the audit logs. After all references to the users have been deleted in due time, the users are also permanently deleted. - Entity and database extended by the fields necessary for soft-delete and user locking. - The repository now contains methods that take the `deletedAt` flag into account. - Extends the `loadUserByUsername` method to also take into account the `enabled` and `locked` flags. - Adds the `locked` status to the handling of an update. This allows the status to be set initially with the insert and changed later via the update. - A job has been added to permanently delete users as soon as this is possible. If there is an error with the referential integrity, then this is ignored for a configurable period of time, since data such as the audit logs must be stored for a certain time and have references. - User mapper extended to also map from `UserInsertDTO` to `UserAccount` and manual mapping removed. - Code of the DTOs rewritten to Records and thus greatly simplified. - `InitialAdminLoader` has been moved to the `users` package, because this class has more to do with user management than with authorization. Refs iris-connect/iris-backlog#235

…eference checkers As it turned out in the Daily, the assumption that users can be deleted as soon as there is no more reference was wrong. E.g. `IrisMessageFolder` and `UserAccounts` themselves also have a reference to users and since they have no deletion period, they can prevent deletion. Deletion is thus inconvenient and has been replaced by anonymization. The check if references exist has been limited to certain relevant references. Furthermore, a configurable time-based anonymization has been implemented.

…r` implementations

mad-nuts · 2022-05-05T11:20:25Z


 	Optional<CaseDataRequest> findByIdOrDataAuthorizationToken(DataRequestIdentifier id, String dataAuthorizationToken);
+
+	@Query("select count(r)>0 from CaseDataRequest r where r.metadata.createdBy = :userId OR r.metadata.lastModifiedBy = :userId")


We could avoid rewriting the same code for every single user-referenced entity by creating and extending a dedicated "AggregateRepository" (which itself extends the JpaRepository). The AggregateRepository would come with the method "isReferencedToUser".

Furthermore we could move every meta-data related method to the "AggregateRepository". E.g.: findByMetadataCreatedIsBefore

I think I was able to take up the suggestion sensibly and thus simplify the code. Thanks for the food for thought.

mad-nuts · 2022-05-05T11:23:10Z

 */
 public enum UserRoleDTO {
-	ADMIN, USER
+	ADMIN, USER, DELETED


Do we need the DELETED role in the Dto? Deleted users should never show op in the frontend, right?

In fact, according to the current application, we do not need this. This was just the simplest solution to satisfy MapStruct when creating the mappers.

Alternatively, I need to tell MapStruct to which value the DELETED from the entity should be mapped.

…gregate-level operations and removes duplicate methods in each repository

# [1.6.0-rc.1](v1.5.1...v1.6.0-rc.1) (2022-06-22) ### Bug Fixes * add support for multi-column sort query parameters (fixes broken table sort of iris-message list) ([9daf6a1](9daf6a1)), closes [#801](#801) * **Dependencies:** Updates version of jackson-databind to fix the vulnerability: avd.aquasec.com/nvd/cve-2020-36518 ([84a4b04](84a4b04)) * **Deps:** updates Spring Boot to 2.6.6 to fix the vulnerability avd.aquasec.com/nvd/cve-2022-22965 ([46a50b5](46a50b5)) * fix dependabot security alert and update multiple npm dependencies ([7b71e64](7b71e64)), closes [#729](#729) * fix e2e tests by correcting the spec order ([53fd088](53fd088)), closes [#764](#764) * Fixes a validation error when changing user data of admins. This could lead to an admin not being able to change their data under certain circumstances (only admin and role not transferred with). ([61f6bc3](61f6bc3)), closes [#703](#703) * ga-gotham config tls communication between internal eps ([4b6cf41](4b6cf41)) * removed line breaks at the end of certificates. ([64104a0](64104a0)) ### Features * For JSON-RPC calls (calls from EPS), the client name submitted by EPS is now used as user (if available). Thus, the metadata of records created via JSON-RPC now also contain a user as creator and it is easier to see by whom the data was created. ([71ff56f](71ff56f)), closes [#826](#826) * **Messages:** Messages can now be used to exchange guests of events between health departments. This makes it possible to transmit the guests received through a data request to the responsible department. The data can be transferred directly from the event overview to a message or can also be added to a message as an attachment. This is the beginning, more data types will follow. ([9c3c8cd](9c3c8cd)), closes [#640](#640) * **Messages:** Messages can now be used to exchange vaccination reports between health departments. This makes it possible to transmit received records to the appropriate department through a data transfer. The data can be transferred directly from the vaccination report overview to a message or can also be added as an attachment to a message. ([64636ba](64636ba)), closes [#762](#762) * Old messages are deleted after a configurable time (default is after 180 days) with all associated data. ([d768632](d768632)), closes [#773](#773) * The authentication tokens (JWT) now retain their validity beyond the restart of the IRIS client. This means that, ideally, users notice only little of a restart of the application. ([2442685](2442685)), closes [#804](#804) * The client backend now also supports the use of a refresh token, which can be used to extend the short validity of the authentication. This makes it more convenient to use, especially in conjunction with a two-factor authentication. ([b20ed86](b20ed86)), closes [#803](#803) * The client is now a bit more secure against attacks and authentication token (JWT) stealing. For this, the JWT is now transferred and processed in HTTP-only cookies. In this context, XSRF protection with XSRF-TOKEN cookies has also been enabled. ([ae25da8](ae25da8)), closes [#802](#802) * Users are no longer deleted immediately, but marked as deleted. The marked users can no longer be used and are no longer displayed. However, the data is still available, for example, for working with the audit logs. After all references to the users are deleted according to the respective deadline or after a specified time, the users are finally anonymized. Procedure and time periods are configurable. ([a913eaf](a913eaf)), closes [iris-connect/iris-backlog#235](https://github.com/iris-connect/iris-backlog/issues/235) [#761](#761) * Users can be marked as locked. This makes it possible to temporarily lock users when they are absent. The locked users are not deleted, they are still available in the overview, but cannot be used for a login. ([68d55ec](68d55ec)), closes [#775](#775) ### Reverts * Revert "chore(Deps): removes unnecessary Postgres version (spring declares the same) and improves jackson dependency" ([90bb5fa](90bb5fa))

jekutzsche · 2022-06-22T12:34:39Z

🎉 This PR is included in version 1.6.0-rc.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

# [1.6.0](v1.5.1...v1.6.0) (2022-06-30) ### Bug Fixes * add support for multi-column sort query parameters (fixes broken table sort of iris-message list) ([9daf6a1](9daf6a1)), closes [#801](#801) * Changes NGINX Content-Security-Policy configuration to allow data urls as image src and adds `data:` to the forbidden keywords. ([cedf240](cedf240)), closes [#862](#862) * **Dependencies:** Updates version of jackson-databind to fix the vulnerability: avd.aquasec.com/nvd/cve-2020-36518 ([84a4b04](84a4b04)) * **Deps:** updates Spring Boot to 2.6.6 to fix the vulnerability avd.aquasec.com/nvd/cve-2022-22965 ([46a50b5](46a50b5)) * fix dependabot security alert and update multiple npm dependencies ([7b71e64](7b71e64)), closes [#729](#729) * fix e2e tests by correcting the spec order ([53fd088](53fd088)), closes [#764](#764) * Fixes a validation error when changing user data of admins. This could lead to an admin not being able to change their data under certain circumstances (only admin and role not transferred with). ([61f6bc3](61f6bc3)), closes [#703](#703) * Fixes an occasional `ConstraintViolationException` that can only be caused by parallel processing of multiple requests from the same IP. ([71c1c98](71c1c98)), closes [#828](#828) * ga-gotham config tls communication between internal eps ([4b6cf41](4b6cf41)) * HTTP status code is now set correctly for validation errors with JSON-RPC (400). Related to this, there is now a central place to handle exceptions with JSON-RPC and to configure the correct HTTP status code. ([e0b98f7](e0b98f7)), closes [#827](#827) * removed line breaks at the end of certificates. ([64104a0](64104a0)) * When checking incoming and entered data for possible attacks, case is now ignored for keywords. ([a378e58](a378e58)), closes [#864](#864) ### Features * For JSON-RPC calls (calls from EPS), the client name submitted by EPS is now used as user (if available). Thus, the metadata of records created via JSON-RPC now also contain a user as creator and it is easier to see by whom the data was created. ([71ff56f](71ff56f)), closes [#826](#826) * In the `.env` (see `.env.sample`) now the configuration for the mail dispatch can be done. With this it is now possible to send notifications when new data has been transferred to the IRIS client (at the moment implemented for the data of an event). ([4310bd0](4310bd0)), closes [#557](#557) [#858](#858) * **Messages:** Messages can now be used to exchange guests of events between health departments. This makes it possible to transmit the guests received through a data request to the responsible department. The data can be transferred directly from the event overview to a message or can also be added to a message as an attachment. This is the beginning, more data types will follow. ([9c3c8cd](9c3c8cd)), closes [#640](#640) * **Messages:** Messages can now be used to exchange vaccination reports between health departments. This makes it possible to transmit received records to the appropriate department through a data transfer. The data can be transferred directly from the vaccination report overview to a message or can also be added as an attachment to a message. ([64636ba](64636ba)), closes [#762](#762) * Old messages are deleted after a configurable time (default is after 180 days) with all associated data. ([d768632](d768632)), closes [#773](#773) * The authentication tokens (JWT) now retain their validity beyond the restart of the IRIS client. This means that, ideally, users notice only little of a restart of the application. ([2442685](2442685)), closes [#804](#804) * The client backend now also supports the use of a refresh token, which can be used to extend the short validity of the authentication. This makes it more convenient to use, especially in conjunction with a two-factor authentication. ([b20ed86](b20ed86)), closes [#803](#803) * The client is now a bit more secure against attacks and authentication token (JWT) stealing. For this, the JWT is now transferred and processed in HTTP-only cookies. In this context, XSRF protection with XSRF-TOKEN cookies has also been enabled. ([ae25da8](ae25da8)), closes [#802](#802) * Users are no longer deleted immediately, but marked as deleted. The marked users can no longer be used and are no longer displayed. However, the data is still available, for example, for working with the audit logs. After all references to the users are deleted according to the respective deadline or after a specified time, the users are finally anonymized. Procedure and time periods are configurable. ([a913eaf](a913eaf)), closes [iris-connect/iris-backlog#235](https://github.com/iris-connect/iris-backlog/issues/235) [#761](#761) * Users can be marked as locked. This makes it possible to temporarily lock users when they are absent. The locked users are not deleted, they are still available in the overview, but cannot be used for a login. ([68d55ec](68d55ec)), closes [#775](#775) * Users can now use two-factor authentication with time-based one-time password (TOTP). If it is enabled, a TOTP is expected and verified by a corresponding app after the conventional login. To set up the app, the user is displayed a QR code by IRIS. It is also possible for the admin to activate this mandatorily via environment variable. If a 2FA is expected but has not yet been finally configured for a user with a successful verification, the QR code is displayed after the successful conventional login and the verification is performed. ([03b915c](03b915c)), closes [iris-connect/iris-backlog#251](https://github.com/iris-connect/iris-backlog/issues/251) [#840](#840) ### Reverts * Revert "chore(Deps): removes unnecessary Postgres version (spring declares the same) and improves jackson dependency" ([90bb5fa](90bb5fa))

jekutzsche · 2022-06-30T14:59:38Z

🎉 This PR is included in version 1.6.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

jekutzsche requested review from lucky-lusa and mad-nuts April 26, 2022 09:42

jekutzsche force-pushed the feature/BL-235-Soft-Delete_for_Users branch 2 times, most recently from 92bb5aa to 099cdfb Compare April 26, 2022 13:19

lucky-lusa reviewed Apr 29, 2022

View reviewed changes

jekutzsche requested a review from lucky-lusa April 29, 2022 14:11