search

Captain-P-Goldfish / scim-for-keycloak

a third party module that extends keycloak by SCIM functionality
BSD 3-Clause "New" or "Revised" License
186 stars 48 forks source link

Protect the SCIM-API with the realm role scim-api-user #10

Closed msiegenthaler closed 3 years ago

msiegenthaler commented 3 years ago

This PR creates a realm role "scim-api-user" and by default requires that for calls the the SCIM APIs.

msiegenthaler commented 3 years ago

I actually think it would be nicer to have the role on the realm-management client but that would require either:

What do you think?

Captain-P-Goldfish commented 3 years ago

I am missing two things here:

  1. the new role is not added by default to the endpoints. As pointed out in #8 this can be done here: ScimResourceTypeService#createNewResourceTypeEntry(ResourceType, RealmModel). This will require an adjustment on the unit tests . The role must not explicitly be assigned to all endpoints [create, get, list, update, delete]. There is a basic role assignment field in the ResourceType that acts as default for all endpoints. In the ui it is named Common Roles
  2. An additional integration test that proves that the role is being created on new realm creation and that the role is assigned by default to the endpoints.
Captain-P-Goldfish commented 3 years ago

If you got trouble with the integration tests I might add a README.MD to explain how it works in detail

msiegenthaler commented 3 years ago

Thanks for reviewing it! I think I did add it to createNewResourceTypeEntry, but just to endpointRoles, I guesses that it would be the appropriate: image Was this the wrong thing?

Agree, I absolutely need to add unit test, I just wasn't sure about the realm role so I delayed that. But if your fine with that I'll start adding it.

Captain-P-Goldfish commented 3 years ago

you are correct. Seems I am still asleep :-) you did it. So just the integration-test adjustments are needed

msiegenthaler commented 3 years ago

Note to self: also update the README.md

msiegenthaler commented 3 years ago

Although it's a bit funny that the unit tests still pass. Shouldn't they fail now since the user won't have the realm role?

Captain-P-Goldfish commented 3 years ago

I just thought the same and checked but no its okay. You changed the default and not the behaviour. If the role that is now created for each realm will be deleted by an administrator it will still work since if the role does not exist the getRoles method simply returns an empty list and thus no default roles are applied.

Captain-P-Goldfish commented 3 years ago

nice thing to mention by the way. An additional unit test would be nice that shows that the default role is correctly applied if it does exist

msiegenthaler commented 3 years ago

See #11 for a probably better approach

Captain-P-Goldfish commented 3 years ago

I personally would prefer this one

msiegenthaler commented 3 years ago

I'm still concerned that everything will be unprotected if someone happens to delete the role. To be fair that would also happen if you manually assigned a role and then deleted it. Basically I have an issue with the "if the restriction is empty then allow everybody" approach esp. if combined with the "if the role/client if not found then ignore it" logic. That just looks to me as security incidents waiting to happen. Know what I mean?

11 is a bit better because it will fail if you delete the client role. To actually turn it off you have to toggle the switch to off manually. Plus I think it's less likely that someone is going to delete a client role in realm-management.

We could add an switch ("require user role": yes/no) that would enforce the check and deny access even if there are no allowed roles configured. To be consistent I'd probably also add the same switch to the clients.

BTW: how is your position regarding (in addition) checking the usual permission from realm-management (query-users, manage-users etc.)? I think it'd be possible to implement that with a few lines in UserHandler, GroupHandler, RealmRoleHandler (probably along with some supporting stuff in Authentication). If we had that then the problem in this issue would probably be solved anyway.

Captain-P-Goldfish commented 3 years ago

Yeah I know what you mean. I took some thoughts about this issue in the last few hours and I think I found a good solution.

I actually do not like either of the current solutions. Neither yours neither my own. Here comes a new idea and I would like to hear your thoughts about it. We will now deactivate SCIM be default which is perfectly fine. Why not keeping it disabled until properly configured? So the web-admin console must inform the admin that SCIM is unusable until proper authorization was configured. If this is not wanted a switch should be available to turn off the need for authorization. Like this the admin is able to decide if the endpoints should be protected by client-roles or realm-roles. This would also be a better fit for our usecase. what do you think?

Your other idea about checking the roles of the realm-management with "query-users" etc. is also a good approach. I would like to have this feature enabled by default if SCIM is enabled for a specific realm. But it should also be possible to turn this off and return to the current state. So that the admin is able to decide on the roles necessary to access the SCIM endpoints. Do you agree?

Captain-P-Goldfish commented 3 years ago

I am going to close this one. I added some security considerations into the README.MD file