Skip to content

Conversation

@shridhargadekar
Copy link
Contributor

Tests for cache_credentials = true not working in sssd, with specified PAM configuration in /etc/pam.d/system-auth and /etc/pam.d/password-auth

verifies #7968

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

The pull request adds a new test to verify that cache_credentials = true works correctly with a custom PAM configuration when the provider is offline. The test logic is sound and covers the intended scenario. My review focuses on improving the maintainability and clarity of the new test by suggesting a more descriptive name for the test function and updating its docstring to be accurate and complete.

Copy link
Contributor

@thalman thalman left a comment

Choose a reason for hiding this comment

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

Thanks for the effort.

Copy link
Contributor

@sumit-bose sumit-bose left a comment

Choose a reason for hiding this comment

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

Hi,

thank you for the update, ACK.

Please note, this patch can only be committed if #7995 is committed, otherwise CI will always fail.

bye,
Sumit

@alexey-tikhonov
Copy link
Member

alexey-tikhonov commented Jul 7, 2025

Please note, this patch can only be committed if #7995 is committed, otherwise CI will always fail.

#7995 was merged. Re-running CI.

@alexey-tikhonov
Copy link
Member

@shridhargadekar, please rebase.

@alexey-tikhonov
Copy link
Member

alexey-tikhonov commented Jul 8, 2025

tests/test_authentication.py::test_authentication__user_login_with_modified_PAM_stack_provider_is_offline[root-ssh] (ipa) PASSED [ 14%]
tests/test_authentication.py::test_authentication__user_login_with_modified_PAM_stack_provider_is_offline[root-su] (ipa) PASSED [ 14%]
tests/test_authentication.py::test_authentication__user_login_with_modified_PAM_stack_provider_is_offline[sssd-ssh] (ipa) PASSED [ 14%]
tests/test_authentication.py::test_authentication__user_login_with_modified_PAM_stack_provider_is_offline[sssd-su] (ipa) PASSED [ 14%]

Failing tests seems to be unrelated to this PR.

@alexey-tikhonov alexey-tikhonov added Ready to push Ready to push and removed Ready to push Ready to push labels Jul 8, 2025
@alexey-tikhonov
Copy link
Member

Conflict in sssd-2-9.
@shridhargadekar, please open explicit backport PR.

@shridhargadekar
Copy link
Contributor Author

@sumit-bose latest changes are reverting PAM configuration to original state. Let me check with authselect way as well

@shridhargadekar
Copy link
Contributor Author

Looks like previously failing tests are passing now.

@shridhargadekar
Copy link
Contributor Author

Three test cases that are failing here, could not be related to this PR. Those test cases are running before current PR test cases.

@alexey-tikhonov
Copy link
Member

Is this intentional:

pam_file = client.fs.read("/etc/pam.d/system-auth")
...
client.fs.write("/etc/pam.d/system-auth", pam_file)
client.fs.write("/etc/pam.d/password-auth", pam_file)

-- so that "password-auth" == "system-auth" after the test?

@sumit-bose , @thalman , are you fine with the recent updates?

@shridhargadekar shridhargadekar force-pushed the offline branch 3 times, most recently from a413452 to d4d21a2 Compare July 24, 2025 13:52
@shridhargadekar
Copy link
Contributor Author

As Sumit suggested earlier, implementing authselect utility to back-up and restore the PAM stack.

@alexey-tikhonov
Copy link
Member

@shridhargadekar, is this ready?

@shridhargadekar
Copy link
Contributor Author

@alexey-tikhonov Yes, it's ready.

Tests for cache_credentials = true not working in sssd, with specified PAM
configuration in /etc/pam.d/system-auth and /etc/pam.d/password-auth

verifies SSSD#7968
@sumit-bose
Copy link
Contributor

Hi,

I'm still fine withthe current version of the test, CI failures do not look related.

bye,
Sumit

@alexey-tikhonov
Copy link
Member

Pushed PR: #8020

  • master
    • 9856b6d - Tests: cache_credentials = true not working
  • sssd-2-11
    • 61a5ab6 - Tests: cache_credentials = true not working

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants