Closed Brzhk closed 5 years ago
Related discussion: https://github.com/jhipster/generator-jhipster/pull/7772#issuecomment-404848962
It's normally used for relationships with the user entity. This isn't possible with the UAA architecture, so we could add @PreAuthorize
based on that condition.
Thanks @Brzhk now I get your issue (personal note: we work together at the moment), and indeed that should be corrected.
hey,
I checked this using monolith with JWT, too. The problem is not only for UAA, but in general for JHipster applications. The question is, if fixing this, should we fix that in general. Maybe there are arguments or reason why we don't do this?
I PRd this in case this is what we want
@xetys See the discussion in the comment I linked above. That's why I think the condition should only be for UAA auth types.
I tried to understand things and got two concerns:
I don't really get why that issue is not present with UAA then when the issue occurs with the drop-down. The UAA should not behave differently on the outside like other security options
The mentioned commit which broke the build was using @Secured
annotation instead of @PreAuthorize
. Logically @Secured('ADMIN')
should work exactly like @PreAuthorize("hasRole('ADMIN')"
but in reality, I always faced @Secured
always throwing 401 for some unknown reason, and this happened every time I tried
I can limit this to UAA, that's not a problem. But would like to make things clear for a better understanding.
You can't generate a User dropdown with UAA, as relationships to the User entity are not allowed. It's needed for other auth types, which allow relationships to the User entity.
Now I'm a little confused. Since when it's not allowed to generate relationships to the user entity using uaa? It is a usual JHipster application where you can generate entities. AFAIK you can do this without limitations inside uaa. Did this change?
It's never been possible for UAA/JWT microservices. OAuth2 microservices allow user relationships because it adds a special user table that syncs the entity from the gateway to the microservice database (using feign clients). I'm guessing this could be implemented for UAA/JWT but it does not work at this time.
This must be a misunderstanding, as I was always doing this with uaa. In my demo project for the microXchg 2016 in Berlin I was demonstrating a one to one relationship with a 'Customer' entity to the user.
Currently I'm working on a private idea called songhub.io, which is built using version 5.7.1 were users are in a many to many relationship with an group entity. The only requirement is that entities related to the user must exist in the uaa. But it was working all the time since I have contributed it. More, it was one of the major intends for uaa, to make the user system compatible with other JHipster features.
In case of this, my PR will also break it for UAA and we need to skip this it find a solution.
I think you are generating the entity on the UAA, rather than a separate microservice (your entity is located in the UAA). I'm talking about a separate microservice having a relation to a User.
I guess it's a valid use-case, so should we just leave it as is? I assumed you would generate the entity in a microservice other than the UAA (like how you normally avoid generating an entity in a gateway app directly).
This is correct. Of course you cannot relate an entity outside of the uaa to the user, as you cannot relate entities across microservices in general. Still you can do this inside uaa. But for the frontend it doesn't matter.
For both cases I mentioned the drop-down issue would occur. IMO fixing this security issue is more important than leaving this as it is. Why?
I see 2 use cases where a relationship to the user makes sense:
See my user to customer relation. In this case the system is designed to have a customer for every user and that should be done automatically (what means the developer should implement this behavior). A normal user should not see other users here. In my user to group case the intended way of building the relationship is an invitation system, where the user should know the other users name.
The admin is doing the relationship. Here it isn't an issue, as the admin has enough privileges anyway.
I don't see a use case where an arbitrary user should see all users for relationship.
However, it we just merge it, it will break functionality, and this is not good. So at least we should document this. A better solution would be possibly to protect user related entities with admin role, too.
Any thoughts on this?
Here's an example use-case which JHipster's user system was based on: https://developer.github.com/v3/users/#get-all-users https://api.github.com/users
This has come up several times and it's not obvious to new JHipster users. So I agree with the change but we definitely need to document it and warn users we don't end up with tons of Github issues. Should it wait for JHipster v6 (based on the comment from @jdubois)?
I think there are use case where being able to select users through a dropdown, however, in all usecases i have in mind, the person using that dropbox can -and should- be awarded some specfic privilege to do so, or the admin one.
And while these use cases exist, i still believe that by default, the architecture we offer as bootstrap with the dual minimum-priviledged user and maximum-priviledged admin profiles should restrict the API as the default behavior.
This was my opinion when i created this ticket, and i only write it down to clarify my intention and reflexion. =)
I stumbled over this issue during my tests with an JWT monolithic application.
In my opinion, this should be a security first approach. Right now without any change the api calls exposes sensible user data (firstname lastname email). Any developer who is not aware of this may come into legal issues.
By default this should only be usable by an admin, documented in a way that any developer who has a different usecase knows the privacy risks.
I have added a @Secured("ROLE_ADMIN") to my UserResource controller.
Also you can switch off all default controllers with @ConditionalOnProperty
I believe this should be closed. As a security-minded developer, I can see the reasoning behind this issue. However, as a regular developer, a user dropdown with no users in it would be puzzling.
If you still feel strongly about this feature, I'd suggest creating a PR with the following features:
@Secured
annotation as mentionedPlease note that you need to have the "ROLE_USER" role to get the user's list, so this is not insecured - it is secured with a role that logged in users have. I have modelled this after the GitHub API initially: when you are logged in you can see the other users, as this is a common business requirement. This is quite usual in companies if you use any kind of directory: for example, I'm pretty sure that in your email client, the names of your co-workers get auto-completed. And this isn't seen as a security risk.
So that's really a choice between security and ease-of-use.
Opened since more than 4 months, and it has been discussed several times. I suggested to close this one, and we can still discuss in this ticket if needed.
Overview of the issue
Anyone logged in with user's privileges can query /api/users and get the list of users.
Motivation for or Use Case
It's not a personal bug to me. But it feels like a very dangerous default behavior.
Reproduce the error
set up an uaa-based project. login with user/user and get the token query the uaa api ??? profit
Related issues
i cant find one.
Suggest a Fix
annotate the UAA's UserResource's "getAllUser" method (around line 145) to require admin role.
JHipster Version(s)
5.7.0
JHipster configuration
Entity configuration(s)
entityName.json
files generated in the.jhipster
directoryN/A
Browsers and Operating System
N/A