freeipa / freeipa-webui

FreeIPA Web UI
GNU General Public License v3.0
23 stars 11 forks source link

[User groups][Members] Implement 'User ID Overrides' section #548

Closed carma12 closed 2 weeks ago

carma12 commented 3 weeks ago

The 'User groups' > 'Members' > 'User ID Overrides' section needs to be implemented and its components adapted to reflect the same behavior as in the modern WebUI (e.g., its 'Add' modal is slightly different than in other sections).

How to reproduce (from current WebUI)

  1. Create a new ID View and some Active users (these will be associated later)
  2. Go to the new ID View settings page
  3. In the 'Overrides' tab section, add the newly-created users
  4. Create a new User group and go to its 'Members' > 'User ID Overrides' section
  5. Add any of the users associated to the ID View

Problem

It seems to be an issue that affects the addition and removal of User ID Overrides items. This is affecting both current and modern WebUI. Video that reproduces the issue. Further investigation and possible report will be done on this.

carma12 commented 3 weeks ago

@mreynolds389 - I corrected the code based on your comments. The implementation of the tests are not possible at this point because it requires to add new ID Views and assign users from the 'Overrides' section, and that functionality is not implemented yet in the modern WebUI. So I will create an issue to track this and not forget about it. Feel free to keep reviewing, thanks!

carma12 commented 3 weeks ago

New issue for the tests created: https://github.com/freeipa/freeipa-webui/issues/549

abbra commented 2 weeks ago

So, the core of the problem is that we only should be allowing ID overrides from 'Default Trust View' to be added to groups as 'ID overrides'. These are the only ones that matter because the groups in questions will be used for LDAP access controls. Access control in LDAP will be done against LDAP DN of the bound account which can only be mapped to the entry in "Default Trust View" ID View and not any other ID view.

So we'd need to fix following:

carma12 commented 2 weeks ago

This PR won't be merged due to some existing errors in IPA (reported here and here). We will wait until those are fixed in order to continue with the implementation of this feature.