Skip to content

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

Merged
ofedoren merged 1 commit intotheforeman:developfrom
adamruzicka:ldap-rfc4519
Jun 25, 2025
Merged

Fixes #9494 - Introduce RFC4519 group membership for posix ldap#10527
ofedoren merged 1 commit intotheforeman:developfrom
adamruzicka:ldap-rfc4519

Conversation

@adamruzicka
Copy link
Contributor

@adamruzicka adamruzicka commented Apr 22, 2025

Requires theforeman/ldap_fluff#88

Steps to reproduce

  1. Have Foreman
  2. Have FreeIPA
  3. Have FreeIPA configured as a type=posix ldap auth source in foreman
  4. Create a user in FreeIPA
  5. Create a type=non-posix user group in FreeIPA
  6. Add user from 4 to group from 5
  7. Create an external group in foreman, mapping to the group created in 5
  8. Log into foreman as user from 4
  9. (as admin in foreman) check that the user is not in the external group
  10. (as admin in foreman) refresh the external group, see the user is there
  11. 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

<%= 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.') %>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@adamruzicka
Copy link
Contributor Author

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 adamruzicka requested a review from a team as a code owner May 6, 2025 13:23
@adamruzicka
Copy link
Contributor Author

/packit build

@adamruzicka
Copy link
Contributor Author

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

@evgeni
Copy link
Member

evgeni commented May 7, 2025

@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
Copy link
Member

evgeni commented May 7, 2025

/packit build

@evgeni
Copy link
Member

evgeni commented May 7, 2025

it built 🎉

evgeni
evgeni previously approved these changes May 7, 2025
Copy link
Member

@evgeni evgeni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📦 :shipit:

@adamruzicka
Copy link
Contributor Author

No actual changes done, just squashed the commits.

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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

image

image

Comment on lines -93 to +109
if (window.location.pathname.match('auth_source_ldaps/i')) {
if (window.location.pathname.match(/auth_source_ldaps/i)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Comment on lines 48 to 49
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 :ldap_group_membership, AuthSourceLdap::GROUP_MEMBERSHIP_TYPES.keys, :desc => N_("type of group membership to use, applicable only when server_type is posix, free_ipa or netiq. Option rfc4519 is only applicable when server_type is posix.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks the API (intentionally), but I wonder if we could (should?) add a transitional parameter use_netgroups that will translate internally into ldap_group_membership = nis_netgroups.
This will allow people with older Ansible modules to still use NIS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we did this, would it confuse fam if use_netgroups field was accepted as a parameter, but would not be visible in the response?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially. It would always try to update the auth source (so, pretending to be non-idempotent), but it'd "mostly work"
If you add a fake use_netgroups to the json.rabl, then it would not be confused at all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I'd not block on this. If anything, I'd be more concerned about FAM dropping support for use_netgroups (and posted an idea in https://github.com/theforeman/foreman-ansible-modules/pull/1868/files#r2163644097 ) as people tend to update their ansible collections more often than their foreman servers ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just pushed a besteffort-ish layer that should deal with this, but I wouldn't mind dropping it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Let's see what the others think

@ofedoren ofedoren requested a review from Copilot June 24, 2025 12:41
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR replaces the deprecated use_netgroups flag with a new ldap_group_membership setting (including an RFC4519 option), migrates existing data, and updates all related UI, JS, API, model validations, and tests.

  • Introduce ldap_group_membership column via migration and remove use_netgroups
  • Update model (AuthSourceLdap) to validate and sanitize the new setting
  • Revamp UI/JS, API endpoints, tests, lint config, and factories to use and deprecate use_netgroups

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
webpack/assets/javascripts/foreman_auth_source.js Toggle and disable the new RFC4519 option based on LDAP server type
app/models/auth_sources/auth_source_ldap.rb Add GROUP_MEMBERSHIP_TYPES, validations, and sanitize_group_membership hook
db/migrate/20250528092104_extend_ldap_group_membership_options.rb Add ldap_group_membership, migrate data, drop use_netgroups
app/views/auth_source_ldaps/_form.html.erb Replace checkbox with select for ldap_group_membership
app/controllers/concerns/foreman/controller/parameters/auth_source_ldap.rb Handle deprecation and translation of use_netgroups in controller params
app/controllers/api/v2/auth_source_ldaps_controller.rb Deprecate use_netgroups API param, add ldap_group_membership
app/views/api/v2/auth_source_ldaps/main.json.rabl Expose ldap_group_membership and remap use_netgroups
test/models/auth_sources/auth_source_ldap_test.rb Update tests to assert ldap_group_membership behavior for AD
test/factories/auth_source_ldap.rb Set default ldap_group_membership in factory
script/lint/lint_core_config.js Whitelist new tokens posix and rfc4519
Gemfile Bump ldap_fluff dependency to >= 0.9.0
Comments suppressed due to low confidence (1)

test/models/auth_sources/auth_source_ldap_test.rb:60

  • [nitpick] No tests cover the new RFC4519 path. Add a test case for a POSIX server_type with ldap_group_membership = 'rfc4519', asserting both validation and the to_config output include use_rfc4519_group_membership: true.
  test "it enforces use_netgroups to false for active directory" do

def auth_source_ldap_params
self.class.auth_source_ldap_params_filter.filter_params(params, parameter_filter_context)
filtered = self.class.auth_source_ldap_params_filter.filter_params(params, parameter_filter_context)
if filtered["use_netgroups"].present?
Copy link

Copilot AI Jun 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using .present? on a boolean will skip the branch when use_netgroups is false. Consider checking if the key exists (e.g., filtered.key?("use_netgroups")) to properly handle both true and false values.

Suggested change
if filtered["use_netgroups"].present?
if filtered.key?("use_netgroups")

Copilot uses AI. Check for mistakes.
<li><b>#{_("POSIX")}</b> - #{_("Use the memberuid attribute to map user groups to standard LDAP Group objects.")}</li>
<li><b>#{_("POSIX + RFC4519")}</b> - #{_("Use group membership based on the groupOfNames and groupOfUniqueNames attributes as defined in RFC4519 in addition to POSIX groups. Some POSIX LDAP servers support an alternative way of using the groupOfNames - member and groupOfUniqueNames - uniqueMember attributes to model group membership.")}</li>
<li><b>#{_("NIS Netgroups")}</b> - #{_("Use NIS netgroups instead of posix groups. FreeIPA and POSIX LDAP server types supports alternative way of grouping users through Netgroups.")}</li>
</ul>" } %>
Copy link

Copilot AI Jun 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The label_help HTML is passed as a raw string; ensure it's marked HTML-safe (e.g., .html_safe or raw) so it isn't escaped by the template renderer.

Suggested change
</ul>" } %>
</ul>".html_safe } %>

Copilot uses AI. Check for mistakes.
Comment on lines 40 to 41
validates :ldap_group_membership, :presence => true, :inclusion => { :in => GROUP_MEMBERSHIP_TYPES.keys.map(&:to_s) }, :if => proc { |auth| auth.server_type.to_s == 'posix' }
validates :ldap_group_membership, :presence => true, :inclusion => { :in => ['posix', 'nis_netgroups'] }, :if => proc { |auth| %w[netiq free_ipa].include?(auth.server_type.to_s) }
Copy link

Copilot AI Jun 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] You have two separate validations on ldap_group_membership with similar logic for different server types. Consider combining them into one validation with a conditional that covers both posix and free_ipa/netiq to reduce duplication.

Suggested change
validates :ldap_group_membership, :presence => true, :inclusion => { :in => GROUP_MEMBERSHIP_TYPES.keys.map(&:to_s) }, :if => proc { |auth| auth.server_type.to_s == 'posix' }
validates :ldap_group_membership, :presence => true, :inclusion => { :in => ['posix', 'nis_netgroups'] }, :if => proc { |auth| %w[netiq free_ipa].include?(auth.server_type.to_s) }
validates :ldap_group_membership, :presence => true, :inclusion => { :in => proc { |auth|
case auth.server_type.to_s
when 'posix'
GROUP_MEMBERSHIP_TYPES.keys.map(&:to_s)
when 'netiq', 'free_ipa'
['posix', 'nis_netgroups']
else
[]
end
}.call }, :if => proc { |auth| %w[posix netiq free_ipa].include?(auth.server_type.to_s) }

Copilot uses AI. Check for mistakes.
@ofedoren
Copy link
Member

ofedoren commented Jun 24, 2025

Thanks, @adamruzicka, there is nothing from my side except that 🤖 👮 is not happy and maybe some of the :copilot: suggestions make sense, mostly for boolean and a new test.

UPD: Although this is confusing, it's just autolink shenanigans in UI (it suggests that this PR is fixing the linked ones :/)
Screenshot-1750768969991

@adamruzicka
Copy link
Contributor Author

Although this is confusing, it's just autolink shenanigans in UI

Yeah, looks like github can't wrap it's head around my redmine archeology.

ekohl
ekohl previously approved these changes Jun 25, 2025
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From reading through it I think it makes sense, but I must admit I haven't tried it out myself.

ofedoren
ofedoren previously approved these changes Jun 25, 2025
Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a commit squash and an approval from @theforeman/packaging

gem 'ancestry', '~> 4.0'
gem 'scoped_search', '>= 4.1.10', '< 5'
gem 'ldap_fluff', '>= 0.7.0', '< 1.0'
gem 'ldap_fluff', '>= 0.9.0', '< 1.0'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Packaging was updated in theforeman/foreman-packaging@e80816f.

@ekohl
Copy link
Member

ekohl commented Jun 25, 2025

I think this is the first time I see where our Redmine issue number links to an actual PR. We may see that be a problem more often in the future.

@adamruzicka
Copy link
Contributor Author

🍏

@ekohl
Copy link
Member

ekohl commented Jun 25, 2025

@ofedoren mind doing the honors when you think it's good to merge?

@ofedoren
Copy link
Member

@ofedoren mind doing the honors when you think it's good to merge?

That's the third time I'd have pressed the button for this PR. I'd not trust myself, but I'm ready to be shot by "Even AI is better at reviewing" :D

Thanks to everyone involved!

@ofedoren ofedoren merged commit a3962df into theforeman:develop Jun 25, 2025
63 of 65 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants