Skip to content

Add GDM Smartcard tests #8216

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
spoore1 merged 3 commits into from
Jan 19, 2026
Merged

Add GDM Smartcard tests #8216

spoore1 merged 3 commits into from
Jan 19, 2026

Conversation

@spoore1
Copy link
Contributor

@spoore1 spoore1 commented Nov 20, 2025

No description provided.

@spoore1 spoore1 marked this pull request as draft November 20, 2025 23:24
gemini-code-assist[bot]
gemini-code-assist bot reviewed Nov 20, 2025
View reviewed changes
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

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 spoore1 requested a review from ikerexxe November 21, 2025 16:09
ikerexxe
ikerexxe reviewed Dec 1, 2025
View reviewed changes
Copy link
Contributor

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

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

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 implementation enroll_smartcard() only works with IPA provider

@spoore1
Copy link
Contributor Author

spoore1 commented Dec 1, 2025

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 implementation enroll_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 spoore1 force-pushed the test_gdm_smartcard branch 4 times, most recently from 90b1f3f to 2a0ef6b Compare December 14, 2025 21:24
ikerexxe
ikerexxe reviewed Dec 15, 2025
View reviewed changes
Copy link
Contributor

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

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

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 spoore1 marked this pull request as ready for review December 16, 2025 16:37
@spoore1 spoore1 requested a review from ikerexxe December 16, 2025 16:37
ikerexxe
ikerexxe reviewed Dec 17, 2025
View reviewed changes
Copy link
Contributor

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

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

Some minor comments inline

@ikerexxe
Copy link
Contributor

ikerexxe commented Dec 17, 2025

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?

madhuriupadhye
madhuriupadhye reviewed Dec 17, 2025
View reviewed changes
Copy link
Contributor

@madhuriupadhye madhuriupadhye left a comment

Choose a reason for hiding this comment

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

see inline

@alexey-tikhonov alexey-tikhonov added the no-backport This should go to target branch only. label Dec 18, 2025
@spoore1
Copy link
Contributor Author

spoore1 commented Dec 18, 2025

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?

@ikerexxe Here are the related PRs (including the framework one):

@spoore1 spoore1 force-pushed the test_gdm_smartcard branch 2 times, most recently from f90bf07 to f8399a4 Compare December 19, 2025 22:17
@spoore1 spoore1 force-pushed the test_gdm_smartcard branch from f8399a4 to 01681ac Compare January 7, 2026 02:07
@spoore1 spoore1 force-pushed the test_gdm_smartcard branch from 01681ac to 1633dde Compare January 15, 2026 18:37
@ikerexxe ikerexxe added the passwordless-gdm PRs related to the Passwordless GDM feature label Jan 16, 2026
ikerexxe
ikerexxe reviewed Jan 16, 2026
View reviewed changes
Copy link
Contributor

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

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

Some minor comments inline. I specially like that you removed the time dependency for the passkey tests. That should make the tests more stable.

@spoore1 spoore1 force-pushed the test_gdm_smartcard branch 3 times, most recently from fd0df2b to 3566707 Compare January 17, 2026 18:50
ikerexxe
ikerexxe approved these changes Jan 19, 2026
View reviewed changes
Copy link
Contributor

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for taking the feedback into consideration

madhuriupadhye
madhuriupadhye approved these changes Jan 19, 2026
View reviewed changes
Copy link
Contributor

@madhuriupadhye madhuriupadhye left a comment

Choose a reason for hiding this comment

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

LGTM.

@spoore1 spoore1 added backport-to-sssd-2-12 and removed no-backport This should go to target branch only. labels Jan 19, 2026
@sssd-bot
Copy link
Contributor

sssd-bot commented Jan 19, 2026

The pull request was accepted by @madhuriupadhye with the following PR CI status:

🟢 CodeFactor (success)
🟢 CodeQL (success)
🟢 osh-diff-scan:fedora-rawhide-x86_64:upstream (success)
🟢 rpm-build:centos-stream-10-x86_64:upstream (success)
🟢 rpm-build:fedora-42-x86_64:upstream (success)
🟢 rpm-build:fedora-43-x86_64:upstream (success)
🟢 rpm-build:fedora-rawhide-x86_64:upstream (success)
🟢 Analyze (target) / cppcheck (success)
🟢 ci / intgcheck (centos-10) (success)
🟢 ci / intgcheck (fedora-42) (success)
🟢 ci / intgcheck (fedora-43) (success)
🟢 ci / intgcheck (fedora-44) (success)
🟢 ci / prepare (success)
🔴 ci / system (centos-10) (failure)
🟢 ci / system (fedora-42) (success)
🟢 ci / system (fedora-43) (success)
🔴 ci / system (fedora-44) (failure)
➖ Coverity scan / coverity (skipped)
🟢 Static code analysis / codeql (success)
🟢 Static code analysis / pre-commit (success)
🟢 Static code analysis / python-system-tests (success)

There are unsuccessful or unfinished checks. Make sure that the failures are not related to this pull request before merging.

@spoore1
Tests: Add GDM Smartcard tests
285e18c
@spoore1
Tests: gdm passkey fixes for timing issues in c10s
6189d6d 
Also removing unused GenericProvider references and update some
docstrings.
@spoore1
Tests: rename and update test_gdm to xidp
bc4fede 
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.
@spoore1 spoore1 force-pushed the test_gdm_smartcard branch from 15d5df0 to bc4fede Compare January 19, 2026 16:17
@spoore1
Copy link
Contributor Author

spoore1 commented Jan 19, 2026

Rebased to pick up changes for black 26.1 update from PR#8379

@spoore1 spoore1 merged commit 7f78c93 into SSSD:master Jan 19, 2026
15 checks passed
@sssd-bot sssd-bot mentioned this pull request Jan 19, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ikerexxe ikerexxe ikerexxe approved these changes

@madhuriupadhye madhuriupadhye madhuriupadhye 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

@ikerexxe ikerexxe

@madhuriupadhye madhuriupadhye

Labels

Accepted backport-to-sssd-2-12 passwordless-gdm PRs related to the Passwordless GDM feature Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@spoore1 @ikerexxe @sssd-bot @madhuriupadhye @alexey-tikhonov