Conversation
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
| TOKEN1_LABEL = "SC_Token_1" | ||
| TOKEN2_LABEL = "SC_Token_2" | ||
| TOKEN_PIN = "123456" |
There was a problem hiding this comment.
To improve readability and maintainability of the polling loop in authenticate_with_smartcard, it's good practice to define the magic numbers used there as constants here.
TOKEN1_LABEL = "SC_Token_1"
TOKEN2_LABEL = "SC_Token_2"
TOKEN_PIN = "123456"
# Constants for the user resolvability polling loop
USER_RESOLVABLE_ATTEMPTS = 15
USER_RESOLVABLE_INTERVAL_S = 2
USER_RESOLVABLE_CACHE_EXPIRY_ATTEMPT = 3| for attempt in range(15): | ||
| time.sleep(2) | ||
| cached = client.tools.getent.passwd(username) | ||
| if cached is not None: | ||
| break | ||
| if attempt == 3: | ||
| client.host.conn.run("sss_cache -E", raise_on_error=False) |
There was a problem hiding this comment.
As suggested in another comment, please use the newly defined constants for the polling loop parameters. This will make the code more readable and easier to maintain.
Additionally, using a fixed time.sleep() can make tests either slow or flaky. For future improvements, consider creating a more robust waiting utility that polls for a condition with a timeout instead of a fixed sleep.
for attempt in range(USER_RESOLVABLE_ATTEMPTS):
time.sleep(USER_RESOLVABLE_INTERVAL_S)
cached = client.tools.getent.passwd(username)
if cached is not None:
break
if attempt == USER_RESOLVABLE_CACHE_EXPIRY_ATTEMPT:
client.host.conn.run("sss_cache -E", raise_on_error=False)There was a problem hiding this comment.
If you move this to the SUAuthenticationUtils, it may make sense to follow this suggestion there. If we have reasonable settings we could use globally, you could add globals to misc/globals.py.
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.
| enroll_to_token(client, ipa, token2_username, token_label=TOKEN2_LABEL) | ||
|
|
||
|
|
||
| def configure_sssd_for_smartcard(client: Client) -> None: |
There was a problem hiding this comment.
I think this can be handled in a new method in the SSSDCommonConfiguration class in the sssd util module. Then you can call something like this in the test:
client.sssd.common.smartcard_with_softhsm()
| client.sssd.start() | ||
|
|
||
|
|
||
| def authenticate_with_smartcard(client: Client, username: str, pin: str, *, num_certs: int = 1) -> None: |
There was a problem hiding this comment.
I think this can move to the authentication utils module in the SUAuthenticationUtils class. Maybe something like smartcard_with_su? you should probably also consider following existing conventions there of having one basic method for simple assertions (like password()) and one that returns output for use within the test case if needed (like password_with_output()).
| for attempt in range(15): | ||
| time.sleep(2) | ||
| cached = client.tools.getent.passwd(username) | ||
| if cached is not None: | ||
| break | ||
| if attempt == 3: | ||
| client.host.conn.run("sss_cache -E", raise_on_error=False) |
There was a problem hiding this comment.
If you move this to the SUAuthenticationUtils, it may make sense to follow this suggestion there. If we have reasonable settings we could use globally, you could add globals to misc/globals.py.
No description provided.