-
Notifications
You must be signed in to change notification settings - Fork 235
Add support for passkey #1372
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.
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
base: master
Are you sure you want to change the base?
Add support for passkey #1372
Conversation
|
The failing tests in CentOS 8 require "guards" to not be executed as 'passkey' is not supported in this version. |
35499be to
4a3975f
Compare
|
@t-woerner the pylint errors are fixed by PR #1380. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- Centralize the supported auth types (including 'passkey') into a shared constant or helper instead of copying the literal choices array in each module to reduce duplication and maintenance overhead.
- Extract the repeated passkey test blocks (ensure change, ensure idempotence, unsupported-message check) into a reusable include or test role to DRY up user/host/service/config test files.
- The passkey support check in env_freeipa_facts.yml uses a grep on
ipa command-find passkeyoutput, which is brittle; consider using ipa_command_exists or inspecting the command return code directly for a more reliable feature detection.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Centralize the supported auth types (including 'passkey') into a shared constant or helper instead of copying the literal choices array in each module to reduce duplication and maintenance overhead.
- Extract the repeated passkey test blocks (ensure change, ensure idempotence, unsupported-message check) into a reusable include or test role to DRY up user/host/service/config test files.
- The passkey support check in env_freeipa_facts.yml uses a grep on `ipa command-find passkey` output, which is brittle; consider using ipa_command_exists or inspecting the command return code directly for a more reliable feature detection.
## Individual Comments
### Comment 1
<location> `plugins/modules/ipapasskeyconfig.py:104-115` </location>
<code_context>
+ supports_check_mode=True,
+ )
+
+ ansible_module._ansible_debug = True
+
+ # Get parameters
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Consider removing or gating the debug flag for production use.
Enabling _ansible_debug in production can leak sensitive data or create noisy logs. Please ensure this is disabled or controlled via a parameter before merging.
```suggestion
require_user_verification=dict(
required=False, type='bool',
aliases=["iparequireuserverification"],
default=None
),
debug=dict(
required=False, type='bool',
default=False
),
),
supports_check_mode=True,
)
ansible_module._ansible_debug = ansible_module.params_get("debug")
# Get parameters
```
</issue_to_address>
### Comment 2
<location> `plugins/modules/ipapasskeyconfig.py:82-90` </location>
<code_context>
+
+def find_passkeyconfig(module):
+ """Find the current passkeyconfig settings."""
+ try:
+ _result = module.ipa_command_no_name(
+ "passkeyconfig_show", {"all": True})
+ except ipalib_errors.NotFound:
+ # An exception is raised if passkeyconfig is not found.
+ return None
</code_context>
<issue_to_address>
**suggestion:** Consider handling unexpected exceptions in find_passkeyconfig.
Only ipalib_errors.NotFound is handled; consider catching or logging other possible exceptions to improve error visibility and diagnostics.
```suggestion
def find_passkeyconfig(module):
"""Find the current passkeyconfig settings."""
try:
_result = module.ipa_command_no_name(
"passkeyconfig_show", {"all": True})
except ipalib_errors.NotFound:
# An exception is raised if passkeyconfig is not found.
return None
except Exception as e:
# Log unexpected exceptions for diagnostics
module.fail_json(msg="Unexpected error in find_passkeyconfig: {}".format(e))
return _result["result"]
```
</issue_to_address>
### Comment 3
<location> `plugins/modules/ipapasskeyconfig.py:127-129` </location>
<code_context>
+ # Connect to IPA API
+ with ansible_module.ipa_connect():
+
+ if not ansible_module.ipa_command_exists("passkeyconfig_show"):
+ msg = "Managing passkeyconfig is not supported by your IPA version"
+ ansible_module.fail_json(msg=msg)
+
+ result = find_passkeyconfig(ansible_module)
</code_context>
<issue_to_address>
**suggestion:** Failing early if passkeyconfig_show is unavailable is good, but consider surfacing the IPA version in the error message.
Displaying the IPA version in the error message will make it easier for users to identify and resolve compatibility problems.
```suggestion
if not ansible_module.ipa_command_exists("passkeyconfig_show"):
ipa_version = getattr(ansible_module.ipa, "version", "unknown")
msg = f"Managing passkeyconfig is not supported by your IPA version ({ipa_version})"
ansible_module.fail_json(msg=msg)
```
</issue_to_address>
### Comment 4
<location> `plugins/modules/ipapasskeyconfig.py:163-167` </location>
<code_context>
if result:
# Map IPA API field to module parameter
if "iparequireuserverification" in result:
exit_args["passkeyconfig"]["require_user_verification"] = \
result["iparequireuserverification"][0]
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/merge-nested-ifs))
```suggestion
if result and "iparequireuserverification" in result:
exit_args["passkeyconfig"]["require_user_verification"] = \
result["iparequireuserverification"][0]
```
<br/><details><summary>Explanation</summary>Too much nesting can make code difficult to understand, and this is especially
true in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>
### Comment 5
<location> `plugins/modules/ipapasskeyconfig.py:151-156` </location>
<code_context>
def main():
ansible_module = IPAAnsibleModule(
argument_spec=dict(
# passkeyconfig
require_user_verification=dict(
required=False, type='bool',
aliases=["iparequireuserverification"],
default=None
),
),
supports_check_mode=True,
)
ansible_module._ansible_debug = True
# Get parameters
require_user_verification = (
ansible_module.params_get("require_user_verification")
)
# Init
changed = False
exit_args = {}
# Connect to IPA API
with ansible_module.ipa_connect():
if not ansible_module.ipa_command_exists("passkeyconfig_show"):
msg = "Managing passkeyconfig is not supported by your IPA version"
ansible_module.fail_json(msg=msg)
result = find_passkeyconfig(ansible_module)
if result is None:
ansible_module.fail_json(msg="Could not retrieve passkeyconfig")
if require_user_verification is not None:
# Generate args
args = gen_args(require_user_verification)
# Check if there are different settings in the find result.
# If yes: modify
if not compare_args_ipa(ansible_module, args, result):
changed = True
if not ansible_module.check_mode:
try:
ansible_module.ipa_command_no_name(
"passkeyconfig_mod", args)
except ipalib_errors.EmptyModlist:
changed = False
except Exception as e:
ansible_module.fail_json(
msg="passkeyconfig_mod failed: %s" % str(e))
else:
# No parameters provided, just return current config
pass
# Get updated config if changes were made
if changed:
result = find_passkeyconfig(ansible_module)
# Prepare exit args
exit_args["passkeyconfig"] = {}
if result:
# Map IPA API field to module parameter
if "iparequireuserverification" in result:
exit_args["passkeyconfig"]["require_user_verification"] = \
result["iparequireuserverification"][0]
# Done
ansible_module.exit_json(changed=changed, **exit_args)
</code_context>
<issue_to_address>
**suggestion (code-quality):** We've found these issues:
- Replace interpolated string formatting with f-string ([`replace-interpolation-with-fstring`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/replace-interpolation-with-fstring/))
- Remove redundant pass statement ([`remove-redundant-pass`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/remove-redundant-pass/))
```suggestion
ansible_module.fail_json(msg=f"passkeyconfig_mod failed: {str(e)}")
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
The certificate test failures are caused by the PKI update that changed the error message. This issue is addressed in another PR (#1392). |
4a3975f to
da803d2
Compare
When testing passkey attributes some version of IPA do not support it, se we need a fact that states that the support is available for proper testing. Signed-off-by: Rafael Guterres Jeffman <[email protected]>
The value 'passkey' was missing as a valid value for user_auth_type attribute. Signed-off-by: Rafael Guterres Jeffman <[email protected]>
The value 'passkey' was missing as a valid value for auth_ind attribute. Signed-off-by: Rafael Guterres Jeffman <[email protected]>
The value 'passkey' was missing as a valid value for auth_ind attribute. Signed-off-by: Rafael Guterres Jeffman <[email protected]>
The value 'passkey' was missing as a valid value for user_auth_type attribute. Signed-off-by: Rafael Guterres Jeffman <[email protected]>
There is a new paskeyconfig management module placed in the plugins
folder:
plugins/modules/ipapasskeyconfig.py
The paskeyconfig module allows to retrieve and modify global passkey
configuration attributes.
Here is the documentation of the module:
README-passkeyconfig.md
New example playbooks have been added:
playbooks/passkeyconfig/passkeyconfig-get.yml
playbooks/passkeyconfig/passkeyconfig-present.yml
New tests for the module can be found at:
tests/passkeyconfig/test_passkeyconfig.yml
tests/passkeyconfig/test_passkeyconfig_client_context.yml
Signed-off-by: Rafael Guterres Jeffman <[email protected]>
da803d2 to
72d46f2
Compare
varunmylaraiah
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All downstream test suites have successfully passed with this PR
|
@varun, did you include the Passkey MR which add more tests? |
|
As downstream for new and changed modules also pass with this PR, I'm removing the "Downstream Tests" label, as tests are already in place. |
This is a proposed fix for #1333, including a new paskeyconfig module, and changes to ipahost, ipauser, ipaservice, and ipaconfig modules.
Please, see individual commits for details.
Summary by Sourcery
Enable passkey authentication across multiple FreeIPA Ansible modules by adding passkey choices, detecting support in test environment, updating tests, and introducing the new ipapasskeyconfig module with associated documentation.
New Features:
Enhancements:
Documentation:
Tests: