Skip to content

Tests: cache_credentials = true not working#8020

Closed
shridhargadekar wants to merge 1 commit into
SSSD:masterfrom
shridhargadekar:offline
Closed

Tests: cache_credentials = true not working#8020
shridhargadekar wants to merge 1 commit into
SSSD:masterfrom
shridhargadekar:offline

Conversation

@shridhargadekar

@shridhargadekar shridhargadekar commented Jul 4, 2025

Copy link
Copy Markdown
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

gemini-code-assist[bot]
gemini-code-assist Bot reviewed Jul 4, 2025
View reviewed changes

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

Copy link
Copy Markdown

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.

Comment thread src/tests/system/tests/test_authentication.py Outdated
Comment thread src/tests/system/tests/test_authentication.py
@shridhargadekar
shridhargadekar marked this pull request as ready for review July 4, 2025 13:44
@thalman thalman self-assigned this Jul 4, 2025
thalman
thalman requested changes Jul 4, 2025
View reviewed changes
Comment thread src/tests/system/tests/test_authentication.py
@shridhargadekar
shridhargadekar force-pushed the offline branch 2 times, most recently from e4a75f7 to f8fe707 Compare July 7, 2025 08:26
@shridhargadekar
shridhargadekar requested a review from thalman July 7, 2025 08:26
thalman
thalman approved these changes Jul 7, 2025
View reviewed changes

@thalman thalman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the effort.

sumit-bose
sumit-bose reviewed Jul 7, 2025
View reviewed changes
Comment thread src/tests/system/tests/test_authentication.py Outdated
sumit-bose
sumit-bose approved these changes Jul 7, 2025
View reviewed changes

@sumit-bose sumit-bose left a comment

Copy link
Copy Markdown
Contributor

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 alexey-tikhonov mentioned this pull request Jul 7, 2025
Closed
@alexey-tikhonov

alexey-tikhonov commented Jul 7, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

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

alexey-tikhonov commented Jul 7, 2025

Copy link
Copy Markdown
Member

@shridhargadekar, please rebase.

@alexey-tikhonov

alexey-tikhonov commented Jul 8, 2025 •
edited
Loading

Copy link
Copy Markdown
Member 
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 commented Jul 10, 2025

Copy link
Copy Markdown
Member

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

github-advanced-security[bot]
github-advanced-security AI found potential problems Jul 16, 2025
View reviewed changes
Comment thread src/tests/system/tests/test_authentication.py Fixed
Comment thread src/tests/system/tests/test_authentication.py Fixed
@shridhargadekar

shridhargadekar commented Jul 16, 2025

Copy link
Copy Markdown
Contributor Author

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

@shridhargadekar

shridhargadekar commented Jul 16, 2025

Copy link
Copy Markdown
Contributor Author

Looks like previously failing tests are passing now.

@shridhargadekar

shridhargadekar commented Jul 22, 2025

Copy link
Copy Markdown
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

alexey-tikhonov commented Jul 24, 2025

Copy link
Copy Markdown
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

shridhargadekar commented Jul 24, 2025

Copy link
Copy Markdown
Contributor Author

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

@alexey-tikhonov

alexey-tikhonov commented Jul 25, 2025

Copy link
Copy Markdown
Member

@shridhargadekar, is this ready?

@shridhargadekar

shridhargadekar commented Jul 25, 2025

Copy link
Copy Markdown
Contributor Author

@alexey-tikhonov Yes, it's ready.

sumit-bose
sumit-bose reviewed Jul 25, 2025
View reviewed changes
Comment thread src/tests/system/tests/test_authentication.py Outdated
@shridhargadekar
Tests: cache_credentials = true not working
7baaec5 
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

sumit-bose commented Jul 29, 2025

Copy link
Copy Markdown
Contributor

Hi,

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

bye,
Sumit

@alexey-tikhonov

alexey-tikhonov commented Jul 31, 2025

Copy link
Copy Markdown
Member

Pushed PR: #8020

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

@shridhargadekar shridhargadekar mentioned this pull request Nov 28, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sumit-bose sumit-bose sumit-bose approved these changes
@thalman thalman thalman approved these changes
+1 more reviewer
@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@thalman thalman

@sumit-bose sumit-bose

Labels

Pushed Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@shridhargadekar @alexey-tikhonov @sumit-bose @thalman @github-advanced-security