Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces system tests for multi-token smartcard authentication. It includes new helper functions for setting up tokens and authenticating, along with several test cases covering different scenarios with two tokens. A key change is updating the sssd-test-framework dependency to a personal fork to support these new tests. My review focuses on the risk associated with this dependency and on improving the maintainability and robustness of the new test code by addressing magic numbers and polling logic.
| git+https://github.com/next-actions/pytest-tier | ||
| git+https://github.com/next-actions/pytest-output | ||
| git+https://github.com/SSSD/sssd-test-framework | ||
| git+https://github.com/krishnavema/sssd-test-framework@multi-token-smart-card-support |
There was a problem hiding this comment.
This change introduces a dependency on a personal fork (krishnavema/sssd-test-framework). While this might be acceptable for development, it poses a security and maintenance risk for the main branch. The changes from this fork should be merged into the upstream SSSD/sssd-test-framework repository, and the dependency should point to an official release or commit from the upstream repository before this pull request is merged.
git+https://github.com/SSSD/sssd-test-framework
spoore1
left a comment
spoore1
left a comment
There was a problem hiding this comment.
This is a good start. I've run into a couple snags with testing but basics are working so here's a start for review.
krishnavema
force-pushed
the
smartcard-multi-token-tests
branch
from
2592a18 to
1322416
Compare
|
'changes requested': all platforms fail |
We're investigating this issue now. I hadn't looked at the test failures yet because I was hitting this one locally. |
|
Hi, please try to run the tests with #8629, his PR should hopefully fix the issue with bye, |
Yes, I can confirm that fixes the failure. I posted to that PR as well. I tested on Fedora 42 containers and I can confirm this fixes the Version: Results: |
spoore1
left a comment
spoore1
left a comment
There was a problem hiding this comment.
You can probably move the enroll_to_token() to the framework. It might be useful in other places (like the GDM tests) to consolidate and reduce duplicate code. Overall the tests look good. And with the fix in #8629, all the tests are working now.
|
|
||
| setup_two_tokens(client, ipa, token1_username=username, token2_username=decoy) | ||
| client.sssd.common.smartcard_with_softhsm(client.smartcard) | ||
| assert client.auth.su.smartcard_with_su(username, TOKEN_PIN) |
There was a problem hiding this comment.
Just a note to change this if you rename in SSSD/sssd-test-framework#239
| TOKEN_PIN = "123456" | ||
|
|
||
|
|
||
| def enroll_to_token( |
There was a problem hiding this comment.
I'm wondering now if this would be better to add this to the smartcard util in the framework with this: SSSD/sssd-test-framework#239
Also, if this is moved to the framework, can the initialize_card() be included here to make this more generic?
4 participants
No description provided.