search

witness-org / witness

Your fitness witness!
1 stars 0 forks source link

35: User Management - [merged] #74

Closed raffaelfoidl closed 2 years ago

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Oct 10, 2021, 01:59

Merges 35-user-management -> develop

Closes #35.

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Oct 10, 2021, 02:03

Commented on server/src/test/java/com/witness/server/integration/infrastructure/test/SecurityInfrastructureController.java line 24

This integration-test-only controller is a compromise between test coverage and independence of tests from production code. As you surely recognize, this controller is basically a copy of GreetingController. This is done on purpose because GreetingController will be discarded as soon as the project advances. As I really wanted to test the security (and especially integration test infrastructure), I cannot rely on some production controller that is subject to change and maybe does not offer all features needed to ensure correct behaviour of the SecurityFilter etc. and BaseIntegrationTest.

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Oct 10, 2021, 02:03

requested review from @11712616

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Oct 10, 2021, 12:29

added 5 commits

Compare with previous version

raffaelfoidl commented 3 years ago

In GitLab by @11712616 on Oct 10, 2021, 14:19

Commented on server/src/main/java/com/witness/server/entity/User.java line 79

Is there a reason why you did not use Lombok's @EqualsAndHashCode annotation? https://projectlombok.org/features/EqualsAndHashCode

raffaelfoidl commented 3 years ago

In GitLab by @11712616 on Oct 10, 2021, 14:19

Commented on server/src/main/java/com/witness/server/enumeration/ServerError.java line 57

Where/Why do we need a phone number? Is this necessary for mapping the Firebase errors?

raffaelfoidl commented 3 years ago

In GitLab by @11712616 on Oct 10, 2021, 14:19

Commented on server/src/main/java/com/witness/server/exception/DataAccessException.java line 11

Do we really need the subtypes DataCreationException and DataModificationException? I am just wondering what benefit(s) you see in such a fine granularity since all three exceptions map to a 500 Internal Server Error.

raffaelfoidl commented 3 years ago

In GitLab by @11712616 on Oct 10, 2021, 14:19

Commented on server/src/main/java/com/witness/server/exception/InsufficientRightsException.java line 11

InsufficientRightsException sounds really dramatic in my opinion, kind of like a violation of the Convention of Human Rights. :smile: I would propose something like AccessDeniedException or AccessForbiddenException. However, if you absolutely want to stick to InsufficientRightsException, I can accept that.

raffaelfoidl commented 3 years ago

In GitLab by @11712616 on Oct 10, 2021, 14:19

Commented on server/src/main/java/com/witness/server/exception/ServerException.java line 8

:thumbsup:

raffaelfoidl commented 3 years ago

In GitLab by @11712616 on Oct 10, 2021, 14:19

Commented on server/src/main/resources/banner.txt line 1

:clap:

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Oct 10, 2021, 14:41

Commented on server/src/main/java/com/witness/server/entity/User.java line 79

Yes, for @Entity classes this is highly discouraged because it inflicts "serious performance issues" (don't remember the exact wording from the docs). If you were to change it, you'd get to see this warning.

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Oct 10, 2021, 14:41

Commented on server/src/main/java/com/witness/server/enumeration/ServerError.java line 57

The fields here catch all possible errors that may come from Firebase as they are copied from Firebase's AuthErrorCode enum, in order to work with the getFromFirebaseErrorCode method. I included them so that, on the client side, we can create suitable error messages for these cases. If you really don't want to include them, we can remove them - the method would then return the fallback. I honestly prefer the current variant, but we can also go the way I just described. Your choice.

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Oct 10, 2021, 14:41

Commented on server/src/main/java/com/witness/server/exception/DataAccessException.java line 11

The benefit I saw in doing it like this is not for the "public view", but for the "interface view". This way, I am able to more accurately describe what error state happens when and by which exception it is caused. This means I don't have to write "if X happens or Y happens or Z happens or...",but I can split it into the different exception types and error causes.

So it was just to make it easier for users of the services/service interfaces. That also makes it possible for them to catch specifically only those types of errors and propagate the more general exception to the other scope. And in my opinion, that's a neat thing to offer.

However, I agree that is technically a bit redundant. If you are so kind to check if we somewhere use the "feature" from above (only catch subtype, propagate supertype) and it turns out that we don't actually have it in use, I may as well throw the subtypes away.

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Oct 10, 2021, 14:41

Commented on server/src/main/java/com/witness/server/exception/InsufficientRightsException.java line 11

Will probably go with AccessDeniedException.

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Oct 10, 2021, 14:41

Commented on server/src/main/java/com/witness/server/exception/ServerException.java line 8

"for to" -> "to"

raffaelfoidl commented 3 years ago

In GitLab by @11712616 on Oct 10, 2021, 17:15

Commented on server/src/main/java/com/witness/server/enumeration/ServerError.java line 57

Okay, then leave it as is.

raffaelfoidl commented 3 years ago

In GitLab by @11712616 on Oct 10, 2021, 17:15

Commented on server/src/main/java/com/witness/server/exception/DataAccessException.java line 11

If I am not mistaken, a DataAccessException is only thrown in two places; once after catching a FirebaseAuthException and once in the checkConsistency method if the emails don't match.

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Oct 10, 2021, 19:08

Commented on server/src/main/java/com/witness/server/exception/InsufficientRightsException.java line 11

30fe1964b6e2837b41e9a6bf8fef35bed609b713

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Oct 10, 2021, 19:08

Commented on server/src/main/java/com/witness/server/exception/ServerException.java line 8

9c358d787b423fdf46bfdc6976d82df7d409a51e

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Oct 10, 2021, 19:08

Commented on server/src/main/java/com/witness/server/exception/DataAccessException.java line 11

To be honest, after thinking about it, I would like to keep it as is. For example, have a look at the throws clauses in the JavaDocs of UserSerivce#createUser:

image

I actually do not want to lose the ability do differentiate between those types of errors and give information specifically targeted to them. If the API consumer does not care, they can just catch the super type DataAccessException. But as the provider of the API, from a design point of view, I do see benefit in differentiating between those causes.

Do you see any potential drawbacks?

I have added class docs to the supertype and fixed typos in the commit 025e54255c00c70f15d58b2d695a3d289df1621b

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Oct 10, 2021, 19:09

Commented on server/src/main/java/com/witness/server/exception/InsufficientRightsException.java line 11

changed this line in version 3 of the diff

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Oct 10, 2021, 19:09

added 3 commits

Compare with previous version

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Oct 10, 2021, 19:13

added 1 commit

Compare with previous version

raffaelfoidl commented 3 years ago

In GitLab by @11712616 on Oct 10, 2021, 19:15

Commented on server/src/main/java/com/witness/server/exception/DataAccessException.java line 11

Keeping it as is is fine with me.

raffaelfoidl commented 3 years ago

In GitLab by @11712616 on Oct 10, 2021, 19:18

approved this merge request

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Oct 10, 2021, 19:20

resolved all threads

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Oct 10, 2021, 19:25

added 9 commits

Compare with previous version

raffaelfoidl commented 3 years ago

In GitLab by @11775820 on Oct 10, 2021, 19:27

mentioned in commit a0c3b0b791262cf2643ffd99f0f5093951ef0b73