Check that allowed_uids in the SSSD config is valid

[rcritten]

If UID 389 or user dirsrv is found in 'allowed_uids' in sssd.conf section [pac] then this will prevent SSSD from resolving the LDAP service account locally and can cause IPA services to fail.

Fixes: #362

Summary by Sourcery

Add a new SSSDAllowedUids389Check plugin to validate that UID 389 or the DS_USER is not included in the allowed_uids option of the [pac] service in sssd.conf, yielding errors for parse failures, missing sections/options, or invalid entries, and add comprehensive unit tests for the new check.

New Features:

• Introduce SSSDAllowedUids389Check plugin to inspect allowed_uids in the [pac] section of sssd.conf

Tests:

• Add unit tests for SSSDAllowedUids389Check covering missing [pac] section, absent allowed_uids, valid entries, and invalid numeric and alphabetic entries

Chores:

• Update license header year to 2024

[sourcery-ai]

Hey @rcritten - I've reviewed your changes - here's some feedback:

• Remove the duplicate logger = logging.getLogger() line so the module logger isn’t unintentionally reset to the root logger.
• Fix the stray curly brace in the invalid‐UID error message ("User/UID invalid} found…") to avoid confusing output.
• In the ‘multiple allowed uids’ test, actually supply more than one UID (e.g. ‘0,1,2’) to exercise the multi‐value parsing logic.

Prompt for AI Agents

Please address the comments from this code review:
## Overall Comments
- Remove the duplicate `logger = logging.getLogger()` line so the module logger isn’t unintentionally reset to the root logger.
- Fix the stray curly brace in the invalid‐UID error message ("User/UID invalid} found…") to avoid confusing output.
- In the ‘multiple allowed uids’ test, actually supply more than one UID (e.g. ‘0,1,2’) to exercise the multi‐value parsing logic.

## Individual Comments

### Comment 1
<location> `src/ipahealthcheck/ipa/config.py:19` </location>
<code_context>
+DS_USER = platformconstants.DS_USER
+
+
+logger = logging.getLogger()


</code_context>

<issue_to_address>
Redundant logger assignment; only one logger is needed.

The second assignment overwrites the first. Remove the redundant assignment to prevent confusion and ensure the logger is named as intended.
</issue_to_address>

### Comment 2
<location> `src/ipahealthcheck/ipa/config.py:97` </location>
<code_context>
+            return
+        else:
+            uids = {s.strip() for s in uids.split(',') if s.strip()}
+            candidates = {'389', DS_USER}
+            invalid = uids.intersection(candidates)
+            if invalid:
+                yield Result(
</code_context>

<issue_to_address>
Mixing string and variable types in 'candidates' set may cause unexpected results.

Ensure DS_USER is converted to a string and formatted consistently with '389' to avoid mismatches during set intersection.
</issue_to_address>

<suggested_fix>
<<<<<<< SEARCH
            uids = {s.strip() for s in uids.split(',') if s.strip()}
            candidates = {'389', DS_USER}
            invalid = uids.intersection(candidates)
=======
            uids = {s.strip() for s in uids.split(',') if s.strip()}
            candidates = {'389', str(DS_USER).strip()}
            invalid = uids.intersection(candidates)
>>>>>>> REPLACE

</suggested_fix>

### Comment 3
<location> `tests/test_ipa_config.py:145` </location>
<code_context>
+        assert result.check == 'SSSDAllowedUids389Check'
+
+    @patch('SSSDConfig.SSSDConfig')
+    def test_sssd_ok_multiple_allowed_uids_configured(self, mock_sssd):
+        """There is now allowed_uids option in the pac section"""
+        mock_sssd.return_value = SSSDConfig(return_service=True,
</code_context>

<issue_to_address>
Test case 'test_sssd_ok_multiple_allowed_uids_configured' does not actually test multiple allowed_uids.

Update the test to use a comma-separated list of multiple UIDs (e.g., '0,1,2') to properly verify handling of multiple allowed_uids.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[frasertweedale]

Ack. For the no [pac] section case, I'll leave the final decision with you.

[rcritten]

I think it makes sense to not emit an error. The test isn't trying to validate that sssd.conf looks a certain way. It is trying to validate that an option within that section is correct. It should be a no-op now. Test updated along with two typo fixes.