Fixes #12374: Protect user api

[gzsombor]

Move current '/api/users' to '/api/admin/users' and protect it with admin role.
Create a compatible '/api/users' with filtered data.
Currently, only the backend changes are implemented

---

Please make sure the below checklist is followed for Pull Requests.

• All continuous integration tests are green
• Tests are added where necessary
• jhipster-online was updated if necessary
• Documentation is added/updated where necessary
• Coding Rules & Commit Guidelines as per our CONTRIBUTING.md document are followed

When you are still working on the PR, consider converting it to Draft (bellow reviewers) and adding skip-ci label, you can still see CI build result at your branch.

[mshima]

What about?

• securing the entire controller
• secure api/admin for safety.
• create another controller for the filtered content.
• change the entire controller to api/admin at:
  

  generator-jhipster/generators/server/templates/src/main/java/package/web/rest/UserResource.java.ejs

  Line 130 in 291b607

  @RequestMapping("/api")

[mshima]

Probably we should expose just login field by default, WDYT?

[gzsombor]

Yeah, I thought about separating the public and admin-only controller logic into two separate class, and also the tests, but it looked a bit bigger change... But after staring the code longer, I think, this is necessary, for cleanliness.

[gzsombor]

Probably we should expose just login field by default, WDYT?

Good question, I don't know, what would be a good default. My initial thought was that, if we send all the names, the frontend could format it freely, as 'FamilyName GivenName (login)' or 'FirstName LastName (login)' - etc. But this i18n requirement looks very corner case, so maybe a generic 'Name' field could be enough - so it will be easy to override in the server, how to format the name.

[pascalgrimaud]

@gzsombor : what's the state of this PR ? As it's a little breaking change, it would be cool to have it for v7. But I think we can go for v7 beta release without it, if it's not ready yet

[gzsombor]

Sorry for the delays. The backend code works correctly, and React frontend works too. I haven't verified the Angular and Vue frontends yet.
Initially, I thought, that the PublicUserDTO could contain a field, named as 'name' (I thought that it would be better instead of login, which not only hints that this is a valid login, worth trying, but later the user could customize it more easily - yeah, they could assign 'FirstName LastName' to the login field too, but that would look a bit silly 😉 )
However, it turned out, it would trigger a cascade of changes through the frontends as:

• we would need to generate a 'IUser' and 'IUserForAdmin' or 'UserManagementDTO' (the first would have id + name, the later would have id + login + firstName + etc but not name
• more changes in the admin pages to rename the types everywhere
• changing all the sample json-s and jdl-s
  Doing this, would be a cleaner thing, I believe, but it's definitely would involve more work for the frontends. Opinions?

[mshima]

Do relationships are using PublicUser or User?
IMO instead of having PublicUser and User is better to have User and AdminUser.
So the default for relationship should be the filtered one.

About firstName lastName, we can let the user add it's own field using #13195.
What do you think @pascalgrimaud?

[gzsombor]

Do relationships are using PublicUser or User?
IMO instead of having PublicUser and User is better to have User and AdminUser.
So the default for relationship should be the filtered one.

Yes, you are totally right. I've just started thinking about today, that how come, that I haven't encountered the same problem in the backend, which I've faced on the frontend, and came the conclusion, that I haven't solved the problem totally.

[mshima]

I just remembered that maybe is better to call it JhiUser AdminUser, current database is JhiUser.

And it's better to rename the component to don't confuse the developer.

• UserRepository.java -> JhiUserRepository.java
• UserService.java -> JhiUserService.java
• UserResource.java -> JhiUserResource.java
• UserMapper.java -> JhiUserMapper.java
• UserResourceIT.java -> JhiUserResourceIT.java
• PublicUserService.java -> UserService.java
• PublicUserResourceIT.java -> UserResourceIT.java.ejs

user.model.ts:

• PublicUser -> User
• User -> JhiUser

What do you think?

[mraible]

I'm not sure I like this. But then again, I'm against prefixes as a general rule. 😉

…

On Tue, Dec 15, 2020 at 18:38 Marcelo Shima ***@***.***> wrote: I just remembered that maybe is better to call it JhiUser AdminUser, current database is JhiUser. And it's better to rename the component to don't confuse the developer. - UserRepository.java -> JhiUserRepository.java - UserService.java -> JhiUserService.java - UserResource.java -> JhiUserResource.java - UserMapper.java -> JhiUserMapper.java - UserResourceIT.java -> JhiUserResourceIT.java - PublicUserService.java -> UserService.java - PublicUserResourceIT.java -> UserResourceIT.java.ejs user.model.ts: - PublicUser -> User - User -> JhiUser What do you think? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#13048 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAELZEAU3PFHVPQIZRQL3DSVAFQNANCNFSM4TWRGFNQ> .

[mshima]

Don’t like either just think Jhi prefix is better than Admin prefix.
Maybe move management to an admin domain and keep both as just User?

[mraible]

Maybe move management to an admin domain and keep both as just User?

I like this better than adding a prefix.

Why can't we just create a PublicUser DTO that has the name and id of the user? The UserResource should be able to handle marshaling the values between it and a User entity. I'm guessing it's more complicated than that?

[gzsombor]

I rather too not add a Jhi prefix, it won't help in the understanding, what's the difference between a JhiUserDTO and a UserDTO - (luckily, the jhi prefix is only used for the table names, so it's pretty easy to remove it).
As the User is just a User, I don't think, we need separate Repository or Mapper, we just need the find good names for the two separate aspect (or view) of the User entity: one view is for the public/commonly available information, and the other view for providing private/full/detailed access to the user themselves and for the administrators.
So for the public view, I would pick from this list

• PublicUserDTO, MinimalUserDTO, CommonUserDTO or UserDTO
  And for the private:
• PrivateUserDTO, DetailedUserDTO, UserDetailsDTO, FullUserDTO, AdminUserDTO, UserAdministrationDTO or UserDTO

Using UserDTO for the public view would have that advantage, that when other entities refer to a user, they could just use the general mapping that from Xyz entity, we get XyzDTO, and I guess, it would be easier to spot if potential private information could be leaked, if the private dto is named as it explicitly refers to this sensitivity - eg. if you see that a PostDTO, has a PrivateUserDTO sender field, you can be suspicious. Sure, in the opposite case, if you work on a codebase, after awhile, you will learn that UserDTO is only allowed to self/admins.
So that's why the perfectly self explanatory PublicUserDTO is probably not the best solution

[pascalgrimaud]

I'm sorry, I didn't follow closely the comments here. I didn't expect so much complexity about this feature.
I thought it would be easier:

• in SecurityConfiguration, secure /api/admin/users
• in UserResource:
  • change the api to /api/admin/users
  • in the same class, add new public API to retrieve all users, it would be simply a new PublicUserVM (see vm/ManagedUserVM)
  • create the appropriate mapper to map only the field we want, so the user would be able to map additional fields
• in front:
  • the user-management service will call /api/admin/users instead
  • for relationships, it's unchanged as it will still call /api/users

[pascalgrimaud]

In my opinion, we should not create another class.
In real project, you won't create new class, for new API, for each ROLE.
If you had more than 10 roles (which is low), should you create 10 separates class for similar API ? I don't think so.

[gzsombor]

No, it's not separate API for separate user roles, but separate API for different functionality. One API for managing reference data/master data - in this case user data, and one API to consume this data in a tailored way.
If your question is, what would happen, if a given entity would have 10 different views, for 10 different groups of users? Well, it depends - how dynamic and diverse these 10 different view, but I would try to avoid returning DTO's with fields, which are nulls for some groups.
Originally @mshima suggested this resource split, and after thinking about more, I reached to the conclusion that it's indeed a cleaner solution.

[mshima]

Thinking more about the DTO, maybe just 1 DTO is enough.
It already doesn't have any sensitive info.
Just use a custom mapper for the relationship.
IMO it's good enough already.

[pascalgrimaud]

@gzsombor : very good job. If you feel confident, feel free to go ahead and merge it tomorrow plz (after my minor comment)

[pascalgrimaud]

in fact, it must be merged, as there are some breaking changes. So let's have it for v7 beta