Add GDM Smartcard tests #8216
Add GDM Smartcard tests #8216
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 adds new GDM smartcard tests. The changes include updating a test dependency and adding a new test file. My review has identified a critical issue with the dependency change, which points to a personal fork and should be reverted before merging. Additionally, I've found several uses of time.sleep() in the new tests, which can lead to test flakiness and should be replaced with more robust waiting mechanisms.
spoore1
force-pushed
the
test_gdm_smartcard
branch
from
15a30c4 to
0cef101
Compare
ikerexxe
left a comment
ikerexxe
left a comment
There was a problem hiding this comment.
I just took a first glance at this proposal and I'm missing several things:
- Docstring with title, description and test steps is missing.
- Tests only run in IPA environment. Should they also target other environments?
- Related to the previous one,
client_setup_for_smartcard()seems to be for all providers but as I mentioned before tests only cover IPA. In addition, with the current implementationenroll_smartcard()only works with IPA provider
Currently, IPA is all that the test framework can support for smart card testing. We'll expand these when more options are supported in the framework. With that said, I'll review the helper functions closer to try to make them forward compatible if possible. As for the docstrings, I'll update asap. |
spoore1
force-pushed
the
test_gdm_smartcard
branch
4 times, most recently
from
90b1f3f to
2a0ef6b
Compare
ikerexxe
left a comment
ikerexxe
left a comment
There was a problem hiding this comment.
As a general note, I'm missing the most basic smartcard test, where the user is able to login with the correct PIN. I'm telling you this because if test_gdm__smartcard_login_with_certs_and_passkey fails we don't know if it's because smartcard failed or it was passkey.
In addition, commit 3 (Tests: rename and update test_gdm to xidp) doesn't contain the changes specified in the commit message, they are totally unrelated. I don't know if you want to keep this commit, but if you do make sure to change the message or the content.
spoore1
force-pushed
the
test_gdm_smartcard
branch
from
2a0ef6b to
a15f51c
Compare
ikerexxe
left a comment
ikerexxe
left a comment
There was a problem hiding this comment.
Some minor comments inline
spoore1
force-pushed
the
test_gdm_smartcard
branch
from
a15f51c to
550c9d6
Compare
|
The code changes look good to me, but I will refrain from approving them until the framework changes are merged. By the way, is there any open PR for the framework changes? |
| :title: Login via GDM using smart card with PIN | ||
| :setup: | ||
| 1. Configure SSSD for gdm-switchable-auth and pam_cert_auth | ||
| 2. Start SSSD |
There was a problem hiding this comment.
Start SSSD must be with setup 1., Configure SSSD for gdm-switchable-auth and pam_cert_auth.
or do we need explicit start again?
same for all tests.
There was a problem hiding this comment.
No, it happens with step 1 in almost every case. I'll update the docstrings in both test modules.
There was a problem hiding this comment.
Ok, removed the start sssd steps since it's built into step 1.
| 5. Enroll smart card with domain for user | ||
| :steps: | ||
| 1. Select the user from the list. | ||
| 2. Select Smartcard authentication |
There was a problem hiding this comment.
I see two steps and expectedresults , 1 and 2 but are we not asserting those in test cases?
There was a problem hiding this comment.
Reworded the step/expectedresult to match others. Let me know if it makes more sense now or if you think I need to update the tests.
| enroll_smartcard(client, ipa, testuser, id="02", init=False) | ||
|
|
||
| # Set user_auth_type to passkey | ||
| ipa.user(testuser).modify(user_auth_type=["passkey", "pkinit"]) |
There was a problem hiding this comment.
why pkinit?
please add this in setup: also.
There was a problem hiding this comment.
I don't think it's actually needed (except to maybe enforce the kerberos side). I added a comment though to the setup for this though. I may drop this if I can resolve another issue I'm troubleshooting but, for now I added to setup.
spoore1
force-pushed
the
test_gdm_smartcard
branch
from
550c9d6 to
1f6b438
Compare
spoore1
force-pushed
the
test_gdm_smartcard
branch
from
1f6b438 to
74ee50a
Compare
@ikerexxe Here are the related PRs (including the framework one): |
spoore1
force-pushed
the
test_gdm_smartcard
branch
from
74ee50a to
f90bf07
Compare
Renaming test_gdm.py to test_gdm_xidp.py to align with the other test_gdm_* test modules. Also adding authselect for with-switchable-auth which is needed to configure the system for GDM to use the new switchable authentication mechanisms.
point to upstream branch for testing
spoore1
force-pushed
the
test_gdm_smartcard
branch
from
f90bf07 to
f8399a4
Compare
4 participants
No description provided.