Fixes #9494 - Introduce RFC4519 group membership for posix ldap #10527

adamruzicka · 2025-04-22T16:04:18Z

Requires theforeman/ldap_fluff#88

Steps to reproduce

Have Foreman
Have FreeIPA
Have FreeIPA configured as a type=posix ldap auth source in foreman
Create a user in FreeIPA
Create a type=non-posix user group in FreeIPA
Add user from 4 to group from 5
Create an external group in foreman, mapping to the group created in 5
Log into foreman as user from 4
(as admin in foreman) check that the user is not in the external group
(as admin in foreman) refresh the external group, see the user is there
Log out and log in as user from 4

If everything works, the user should still be in the external group.

TODO:

bump dependency on ldap_fluff

aneta-petrova · 2025-04-24T11:14:54Z

app/views/auth_source_ldaps/_form.html.erb

@@ -30,6 +30,7 @@
      <%= text_f f, :base_dn, :label => _("Base DN"), :size => "col-md-8", :label_help => base_dn_help_data[@auth_source_ldap.server_type], :data => { :help => base_dn_help_data } %>
      <%= text_f f, :groups_base, :label => _("Groups base DN"), :size => "col-md-8", :label_help => groups_base_dn_help_data[@auth_source_ldap.server_type], :data => { :help => groups_base_dn_help_data } %>
      <%= checkbox_f f, :use_netgroups, :help_inline => _("Use NIS netgroups instead of posix groups."), :label_help => _('By default we map user groups to standard LDAP Group objects. FreeIPA and POSIX LDAP server types supports alternative way of grouping users through Netgroups. Enable this checkbox if using Netgroups is preferred instead of standard groups.') %>
+      <%= checkbox_f f, :use_rfc4519_group_membership, :label => _('Use RFC4519 group membership'), :help_inline => _("Use group membership as defined in RFC4519 using groupOfNames and groupOfUniqueNames in addition to posix groups."), :label_help => _('By default we map user groups to standard LDAP Group objects using the memberuid attribute. POSIX LDAP servers may support an alternative way of modelling group membership using groupOfNames - member and groupOfUniqueNames - uniqueMember attributes. Enable this checkbox if your LDAP server uses this instead of plain POSIX groups per RFC2307.') %>


I have a few nitpicky suggestions to improve readability:

Suggested change

<%= checkbox_f f, :use_rfc4519_group_membership, :label => _('Use RFC4519 group membership'), :help_inline => _("Use group membership as defined in RFC4519 using groupOfNames and groupOfUniqueNames in addition to posix groups."), :label_help => _('By default we map user groups to standard LDAP Group objects using the memberuid attribute. POSIX LDAP servers may support an alternative way of modelling group membership using groupOfNames - member and groupOfUniqueNames - uniqueMember attributes. Enable this checkbox if your LDAP server uses this instead of plain POSIX groups per RFC2307.') %>

<%= checkbox_f f, :use_rfc4519_group_membership, :label => _('Use RFC4519 group membership'), :help_inline => _("Use group membership based on the groupOfNames and groupOfUniqueNames attributes as defined in RFC4519 in addition to POSIX groups."), :label_help => _('By default we use the memberuid attribute to map user groups to standard LDAP Group objects. Some POSIX LDAP servers support an alternative way of using the groupOfNames - member and groupOfUniqueNames - uniqueMember attributes to model group membership. Enable this checkbox if your LDAP server uses this instead of plain POSIX groups per RFC2307.') %>

However, mostly it would be great if there was a way to make the label more informative. Use RFC4519 group membership references the RFC code but the details of that might be unknown to users. The best I could come up with is this:

Suggested change

<%= checkbox_f f, :use_rfc4519_group_membership, :label => _('Use RFC4519 group membership'), :help_inline => _("Use group membership as defined in RFC4519 using groupOfNames and groupOfUniqueNames in addition to posix groups."), :label_help => _('By default we map user groups to standard LDAP Group objects using the memberuid attribute. POSIX LDAP servers may support an alternative way of modelling group membership using groupOfNames - member and groupOfUniqueNames - uniqueMember attributes. Enable this checkbox if your LDAP server uses this instead of plain POSIX groups per RFC2307.') %>

<%= checkbox_f f, :use_rfc4519_group_membership, :label => _('Use alternative group membership based on RFC4519'), :help_inline => _("Use group membership based on the groupOfNames and groupOfUniqueNames attributes as defined in RFC4519 in addition to POSIX groups."), :label_help => _('By default we use the memberuid attribute to map user groups to standard LDAP Group objects. Some POSIX LDAP servers support an alternative way of using the groupOfNames - member and groupOfUniqueNames - uniqueMember attributes to model group membership. Enable this checkbox if your LDAP server uses this instead of plain POSIX groups per RFC2307.') %>

which at least explains that this is an alternative to the default but if there was a way to succinctly explain the actual behavior (what will actually happen if users select that checkbox), that would be great. It might be tricky so if you can't, that's okay, and I'd still consider this an improvement.

However, mostly it would be great if there was a way to make the label more informative.

The ui doesn't seem to like long labels too much, but we could go with that. Personally I'd prefer a shorter label with more elaborate inline help, would that work?

I'd vote for smaller label with additional information included in inline help.

I left the label as-is, but changed the inline help and tooltip according to the suggestions

Looking at the options: can NIS netgroups and RFC4519 groups be used at the same time? Feels like it should be a drop down now with 3 options:

RFC2307 POSIX groups

RFC4519 groupOfNames and groupOfUniqueNames

NIS netgroups

can NIS netgroups and RFC4519 groups be used at the same time?

Not really, or at least not with how ldap_fluff is currently written. That would mean that for certain types, there would be a dropdown with only two options which isn't ideal, but probably still better than checkboxes which override each other. Maybe radio buttons would work? I'll see what I can do

I think from a usability perspective that's better than cleaning up the user, possibly reverting their (incorrect) choice.

ofedoren

Thanks, @adamruzicka, personally I like the approach. Just to understand the bigger picture: this new flag is to fix a bug, but is that bug common? I mean, if that bug is just a corner case, then it should be false by default, but if it's a quite common bug (more and more users leverage features of RFC4519) then maybe we should enable it by default?

P.S. I'd approve after ldap_fluff bump and green tests.

webpack/assets/javascripts/foreman_auth_source.js

adamruzicka · 2025-04-28T08:22:20Z

this new flag is to fix a bug, but is that bug common?

I'm afraid I can't reliably answer that. It is an issue for people running 389DS (or derivatives). In theory it shouldn't hurt to leave this turned on, but I'd still rather stay on the safe side

adamruzicka · 2025-05-07T07:21:39Z

/packit build

adamruzicka · 2025-05-07T07:25:55Z

Well, it was worth a try. Anyway the packaging pr for bumping the version of ldap_fluff was merged ~an hour ago

evgeni · 2025-05-07T07:46:16Z

@adamruzicka I kicked off https://ci.theforeman.org/job/foreman-nightly-rpm-pipeline/2697/ -- once that passed you should have 0.9.0 available for packit

evgeni · 2025-05-07T08:24:14Z

/packit build

evgeni · 2025-05-07T08:35:43Z

it built 🎉

evgeni

📦

…ldap

adamruzicka · 2025-05-07T09:11:00Z

No actual changes done, just squashed the commits.

evgeni · 2025-05-08T08:29:39Z

app/controllers/api/v2/auth_source_ldaps_controller.rb

@@ -46,6 +46,7 @@ def show
          param :tls, :bool
          param :groups_base, String, :desc => N_("groups base DN")
          param :use_netgroups, :bool, :desc => N_("use NIS netgroups instead of posix groups, applicable only when server_type is posix or free_ipa")
+          param :use_rfc4519_group_membership, :bool, :desc => N_("use RFC4519 group membership in addition to posix groups, applicable only when server_type is posix")


this will require an update to the ansible module too: https://github.com/theforeman/foreman-ansible-modules/blob/ae6b518157a3e099293450f33ead460fd5f195c8/plugins/modules/auth_source_ldap.py#L218

ekohl

Leaving it as a top level comment too: instead of multiple checkboxes I'd like a selection of group type. Ideally with an automatic default derived from the server type

adamruzicka

Replaced the two checkboxes with a single dropdown.

The dropdown is hidden when server_type is set to active_directory
The POSIX + RFC4519 option is disabled when server_type != posix

adamruzicka · 2025-05-28T13:27:54Z

webpack/assets/javascripts/foreman_auth_source.js

-  if (window.location.pathname.match('auth_source_ldaps/i')) {
+  if (window.location.pathname.match(/auth_source_ldaps/i)) {


Wonder if this ever worked

> window.location.pathname '/auth_source_ldaps/new' > window.location.pathname.match('auth_source_ldaps/i') null > window.location.pathname.match(/auth_source_ldaps/i) ['auth_source_ldaps', index: 1, input: '/auth_source_ldaps/new', groups: undefined]

pr-processor bot added the Not yet reviewed label Apr 22, 2025

github-actions bot added the UI label Apr 22, 2025

aneta-petrova reviewed Apr 24, 2025

View reviewed changes

pr-processor bot removed the Not yet reviewed label Apr 24, 2025

ofedoren reviewed Apr 25, 2025

View reviewed changes

webpack/assets/javascripts/foreman_auth_source.js Outdated Show resolved Hide resolved

adamruzicka force-pushed the ldap-rfc4519 branch from 42a6274 to 0abfc10 Compare April 28, 2025 07:51

adamruzicka force-pushed the ldap-rfc4519 branch from 0abfc10 to 8c2e32b Compare April 30, 2025 08:57

adamruzicka requested a review from a team as a code owner May 6, 2025 13:23

evgeni previously approved these changes May 7, 2025

View reviewed changes

Fixes theforeman#9494 - Introduce RFC4519 group membership for posix …

0226c9c

…ldap

adamruzicka force-pushed the ldap-rfc4519 branch from da52015 to 0226c9c Compare May 7, 2025 09:10

evgeni reviewed May 8, 2025

View reviewed changes

ekohl reviewed May 8, 2025

View reviewed changes

adamruzicka dismissed evgeni’s stale review via 0f2f9f2 May 28, 2025 13:26

adamruzicka commented May 28, 2025

View reviewed changes

adamruzicka mentioned this pull request May 28, 2025

LDAP RFC4519 group membership theforeman/foreman-ansible-modules#1868

Draft

adamruzicka force-pushed the ldap-rfc4519 branch from 0f2f9f2 to 78c28ee Compare May 28, 2025 13:36

adamruzicka mentioned this pull request May 28, 2025

Fixes #TBFL - Ldap group membership theforeman/hammer-cli-foreman#641

Draft

Fixes theforeman#9494 - Remodel using a single field

8f60303

adamruzicka force-pushed the ldap-rfc4519 branch from 78c28ee to 8f60303 Compare May 28, 2025 14:07

		if (window.location.pathname.match('auth_source_ldaps/i')) {
		if (window.location.pathname.match(/auth_source_ldaps/i)) {

Fixes #9494 - Introduce RFC4519 group membership for posix ldap #10527

Are you sure you want to change the base?

Fixes #9494 - Introduce RFC4519 group membership for posix ldap #10527

Uh oh!

Conversation

adamruzicka commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ofedoren left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adamruzicka commented Apr 28, 2025

Uh oh!

adamruzicka commented May 7, 2025

Uh oh!

adamruzicka commented May 7, 2025

Uh oh!

evgeni commented May 7, 2025

Uh oh!

evgeni commented May 7, 2025

Uh oh!

evgeni commented May 7, 2025

Uh oh!

evgeni left a comment

Choose a reason for hiding this comment

Uh oh!

adamruzicka commented May 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekohl left a comment

Choose a reason for hiding this comment

Uh oh!

adamruzicka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adamruzicka commented Apr 22, 2025 •

edited

Loading