Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use InfoPipe as fallback mechanism to fetch user/group information #2526 #2532

Conversation

FroggyFlox
Copy link
Member

Fixes #2526
@phillxnet, ready for review

We currently use the built-ins pwd or grp to fetch user/group information throughout the project. Unfortunately, this can sometimes fail in systems that have freshly been joined to an Active Directory domain. See #2526 for details on the conditions required for such failures.

Proposed fix

To fix this issue, this pull request (PR) proposes to increase the robustness of users/groups fetching by adding InfoPipe as a fallback mechanism during the creation of a Samba export; we already use such fallback to retrieve the groupname information for a domain user (see #2279).
Briefly, we add this InfoPipe-based fallback mechanism in two places and to retrieve:

  • user information when setting Samba admin users
  • group name for a given user when listing system users

To simplify and harmonize our user retrieval mechanisms, switch to using the already established combined_users() method during _set_admin_users() to retrieve all users seen by getent passwd, complemented by InfoPipe as fallback. See #2526 (comment) for details.

This PR also fixes a related bug in retrieving groupname from an AD user: see #2526 (comment) for details.

It also includes the following minor improvements:

  • a refactoring of the now-replaced ifp_get_groupname() function to make it more flexible: able to retrieve any given property or several properties of either a user or a group
  • simplify our current check for the active status of SSSD with a simple boolean wrapper using init_service_op to return True when the given systemd service is active.
  • small docstring formatting according to PEP257 and Sphinx's Python signatures (https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html#python-signatures)
  • return ifp-fetched groupname in User model property only if SSSD is active
  • Black formatting and update copyright

Demonstration

  1. Configure and turn ON the AD service
  2. Go to Storage > Samba and create a new Samba export using an AD user as Samba admin user
  3. The Samba share is successfully created

Testing

No new unit test was added.

rockdev:/opt/rockstor # cd src/rockstor/ && poetry run django-admin test
(...)
System check identified no issues (0 silenced).
.............................................................................................................................................................................................................................................................
----------------------------------------------------------------------
Ran 253 tests in 13.295s

OK

Final note

I noticed the following behavior. When we create a Samba export, the user selected as Samba admin user (if any) is added to the Django User table along with its link to the SambaShare table. We thus now have an AD user in the User table. When this Samba export is deleted, however, we do NOT clean this up. This means that if the machine leaves the AD domain, we will still have that AD user listed as user. This is beyond the scope of this PR, but we may need to think about that as cleaning this up may prove tricky.

…ckstor#2526

We currently use the built-ins `pwd` or `grp` to fetch user/group information
throughout the project. Unfortunately, this can sometimes fail in systems
that have freshly been joined to an Active Directory domain.

This commit implements InfoPipe as a fallback mechanism to fetch:

  - user information when setting Samba admin users
  - group name for a given user when listing system users

When setting samba admin users, switch to using the already established
`combined_users()` method to retrieve all users seen by `getent passwd`,
complemented by InfoPipe as fallback.

It also includes the following minor improvements:

  - a refactoring of the now-replaced `ifp_get_groupname()` function to make
  it more flexible: able to retrieve any given property or several properties
  of either a user or a group
  - simplify our current check for the active status of SSSD with a simple
  boolean wrapper using init_service_op to return True when the given systemd
  service is active.
  - small docstring formatting according to PEP257 and Sphinx's Python
  signatures
  (https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html#python-signatures)
  - return ifp-fetched groupname in User model property only if SSSD is active
  - Black formatting and update copyright
@Hooverdan96
Copy link
Member

Hooverdan96 commented Apr 17, 2023

@FroggyFlox, excellent!
On your Final Note, would it make sense to open an issue for this to keep track of it?
Also, I assume, there could be two options:

  • manual cleanup, i.e. the SambaShare needs to be edited and the user has to be replaced with an existing one (if that is the solution) - this could be addressed via a paragraph in the documentation as a N.B. or something like that.
  • systemic cleanup - that, like you said, could be much trickier and thus also much further down the road.

Copy link
Member

@phillxnet phillxnet left a comment

Choose a reason for hiding this comment

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

@FroggyFlox This is a most welcome improvement, refactor.
I don't currently have the setup to properly test this which makes this a non-review I'm afraid, but we do have a currently active forum member that you have been engaging with so I'm merging this as-is so that they can test in their setup. Plus we have your own testing. You explanations on the what and why are much appreciated and will serve us well as we revisit this code down the line.

Thanks again for the code improvements and much needed revamp. And for the improved comments. Hopefully in time we can further rationalise these fail-through mechanisms and just use what works all of the time: but this will likely be for when we have more of our libraries etc updated.

@phillxnet phillxnet merged commit 571564f into rockstor:testing Apr 25, 2023
@FroggyFlox FroggyFlox deleted the 2526_Failure_to_set_an_AD_user_as_Samba_admin_user_until_Rockstor_service_restart branch May 27, 2023 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants