[autobackport: sssd-2-9-4] adding subid test #8295

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.

Sign up for GitHub

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

Jump to bottom

Draft

sssd-bot wants to merge 1 commit into SSSD:sssd-2-9-4 from sssd-bot:SSSD-sssd-backport-pr8225-to-sssd-2-9-4

+1,061 −0

sssd-bot commented Dec 10, 2025

This is an automatic backport of PR#8225 adding subid test to branch sssd-2-9-4, created by @danlavu.

Caution

@danlavu The patches did not apply cleanly. It is necessary to resolve conflicts before merging this pull request. Commits that introduced conflict are marked with CONFLICT!.

You can push changes to this pull request

git remote add sssd-bot git@github.com:sssd-bot/sssd.git
git fetch sssd-bot refs/heads/SSSD-sssd-backport-pr8225-to-sssd-2-9-4
git checkout SSSD-sssd-backport-pr8225-to-sssd-2-9-4
git push sssd-bot SSSD-sssd-backport-pr8225-to-sssd-2-9-4 --force

Original commits
449913a - adding subid test

Backported commits

578e073 - CONFLICT! adding subid test

Conflicting Files Information (check for deleted and re-added files)

CONFLICT! adding subid test

On branch SSSD-sssd-backport-pr8225-to-sssd-2-9-4
You are currently cherry-picking commit 449913a8a.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Unmerged paths:
  (use "git add/rm <file>..." as appropriate to mark resolution)
	deleted by us:   src/tests/system/tests/test_ipa.py

no changes added to commit (use "git add" and/or "git commit -a")

Original Pull Request Body


          CONFLICT! adding subid test

578e073

(cherry picked from commit 449913a)

sssd-bot assigned alexey-tikhonov and jakub-vavra-cz

sssd-bot requested review from alexey-tikhonov and jakub-vavra-cz

December 10, 2025 17:11

gemini-code-assist bot reviewed

View reviewed changes

gemini-code-assist bot left a comment

Code Review

This pull request backports a new test file, src/tests/system/tests/test_ipa.py, which includes a suite of integration tests for IPA, including the subid test mentioned in the title. While the additions are valuable, I've identified several areas for improvement to enhance the reliability and maintainability of these new tests. My review includes feedback on hardcoded values, the use of fixed-time sleeps that can lead to flaky tests, and instances of incorrect or incomplete test logic. Addressing these points will strengthen the test suite.

src/tests/system/tests/test_ipa.py Show resolved Hide resolved

src/tests/system/tests/test_ipa.py

+                  keys = client.sss_ssh_authorizedkeys("user1").stdout
+                  assert keys, f"Public SSH keys was not found for {user.name}!"
+                  _keys = keys.split(",")
+                  assert key in _keys, f"Public SSH key '{key}' does not match for {user.name}!"

gemini-code-assist bot Dec 10, 2025

The assertion key in _keys checks for exact membership of the key in the list of keys returned by sss_ssh_authorizedkeys. However, sss_ssh_authorizedkeys can return keys with options prefixed, which would cause this assertion to fail. A more robust approach, as used in test_ipa__user_several_authorized_public_ssh_key, is to check if the key is a substring of any of the returned entries. This would make the test more resilient.

Suggested change

      
                assert key in _keys, f"Public SSH key '{key}' does not match for {user.name}!"
          
                assert any(key in x for x in _keys), f"Public SSH key '{key}' does not match for {user.name}!"

src/tests/system/tests/test_ipa.py Show resolved Hide resolved

src/tests/system/tests/test_ipa.py

+                      ssh.run("klist")
+                      ssh.disconnect()
+                  assert not client.auth.sudo.list(user.name), "User found in sudo rule!"
+                  time.sleep(3)

gemini-code-assist bot Dec 10, 2025

Using a fixed time.sleep(3) to wait for log entries to appear can lead to flaky tests. The time for an operation to complete and be logged can vary. A better practice is to implement a polling mechanism that checks for the expected log entry with a reasonable timeout.

src/tests/system/tests/test_ipa.py

Comment on lines +449 to +451

+                      if attr != "login":
+                          result_value = getattr(result, attr, None)
+                          assert result_value == expected_value, f"Overridden {attr}: expected {expected_value}, got {result_value}!"

gemini-code-assist bot Dec 10, 2025

When testing the login attribute override, the test correctly verifies that the user can be looked up by both the original and new names. However, it misses an assertion to confirm that the name attribute of the returned user object is indeed the new, overridden login name. This leaves a gap in the test's validation.

Suggested change

      
                    if attr != "login":
          
                        result_value = getattr(result, attr, None)
          
                        assert result_value == expected_value, f"Overridden {attr}: expected {expected_value}, got {result_value}!"
          
                    if attr == "login":
          
                        assert result.name == expected_value, f"Overridden login: expected {expected_value}, got {result.name}!"
          
                    else:
          
                        result_value = getattr(result, attr, None)
          
                        assert result_value == expected_value, f"Overridden {attr}: expected {expected_value}, got {result_value}!"

src/tests/system/tests/test_ipa.py

Comment on lines +573 to +575

		result = ipa.user("user-1").iduseroverride().show_override("testview1")

		assert certificate_content in result.get("usercertificate", [""])[0], "Certificate content mismatch!"

gemini-code-assist bot Dec 10, 2025

This test aims to verify an ID view override for a user certificate. However, it checks the configuration on the IPA server using ipa.user(...).show_override(...) rather than verifying the outcome on the SSSD client. To properly validate that SSSD has fetched and applied the override, the test should use a client-side tool to inspect the user's certificate as seen by the client.

src/tests/system/tests/test_ipa.py

+                  client.sssd.pam["pam_cert_auth"] = "True"
+                  client.sssd.start()
+                  client.svc.restart("virt_cacard.service")
+                  time.sleep(1)

gemini-code-assist bot Dec 10, 2025

Using time.sleep() introduces potential flakiness into the test suite. Instead of a fixed wait, it would be more robust to poll for the service to be ready or for a specific condition to be met, with a timeout.

src/tests/system/tests/test_ipa.py

+                  client.sssd.restart()
+                  # The 10-second wait is crucial to ensure SSSD updates its cached group-to-HBAC mappings so that
+                  # access control changes take effect correctly during tests.
+                  time.sleep(10)

gemini-code-assist bot Dec 10, 2025

The use of time.sleep(10) makes this test potentially flaky. The comment indicates this is to wait for SSSD to update its cache. A more reliable approach would be to actively poll for the expected state (e.g., by repeatedly checking access until it changes or a timeout is reached) rather than relying on a fixed delay.

danlavu commented Dec 10, 2025

@alexey-tikhonov Did you want this subid test ported to 2-9-4?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet