Skip to content
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

keycloak_user_federation: add module argument that allows excluding bindCredential from update check #8898

Merged
merged 3 commits into from
Oct 7, 2024
Merged

keycloak_user_federation: add module argument that allows excluding bindCredential from update check #8898

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fd5a204
add module argument that allows excluding `bindCredential` from updat…
fgruenbauer Sep 22, 2024
f2b4903
add changelog fragment
fgruenbauer Sep 23, 2024
2a89049
change option name to `bind_credential_update_mode` and change type t…
fgruenbauer Sep 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
add module argument that allows excluding bindCredential from updat… 
…e check
  • Loading branch information
@fgruenbauer
fgruenbauer committed Sep 22, 2024
commit fd5a2045bd5fe64b1c239cfbe2b00653585b64af
24 changes: 21 additions & 3 deletions plugins/modules/keycloak_user_federation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,16 @@
default: true
version_added: 9.4.0

exclude_bind_credential_from_update_check:
description:
- The value of the config parameter O(config.bindCredential) is redacted in the Keycloak responses. Comparing
the redacted value with the desired value always evaluates to not equal.
- Set to V(true) to exclude O(config.bindCredential) when comparing the before state with the desired state to
determine wether an update is required.
type: bool
default: false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, how about using a string state with two states (choices) instead of a boolean? Then it is possible to later add more options, if for example the API is changed to allow other ways to handle this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, sounds good. I'm not sure about the naming though.

Maybe

bind_credential_handling:
    description:
        - The value of the config parameter O(config.bindCredential) is redacted in the Keycloak responses. There is currently no way to get the actual value. Comparing the redacted value with the desired value always evaluates to not equal. This also means the before and desired states are never equal if the parameter is set.
        - Set to V(exclude_from_update_check) to exclude O(config.bindCredential) when comparing the before state with the desired state to determine whether an update is required.
    type: str
    default: include_in_update_check
    choices:
        - include_in_update_check
        - exclude_from_update_check
        (- get_cleartext_value)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would probably adjust the option name a bit - maybe bind_credential_update_mode? - and simplify the choices: never (instead of exclude), always (instead of update). WDYT?

Copy link
Contributor Author

@fgruenbauer fgruenbauer Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I like the option name, but i'm not sure about never. So far the PR only removes the parameter from the update check not the actual update. If there are other changes the parameter would still get updated, so never seems kind of misleading i think. We could adjust the PR accordingly though. I think this would also make a bindCredential update more predictable, either do an update or not. Comparing the redacted value is kind of meaningless. If there is a way to get the actual value later we could then add on_change.

Or maybe skip_check to exclude from the update check, but still leave it in the update.

Copy link
Collaborator

Choose a reason for hiding this comment

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

True, never isn't the best name. How about only_indirect? I think it's a bit easier to understand than skip_check. (I could also be wrong/biased though :) )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I think we got a winner. I will adjust the PR.

version_added: 9.5.0

config:
description:
- Dict specifying the configuration options for the provider; the contents differ depending on
Expand Down Expand Up @@ -822,6 +832,7 @@ def main():
provider_type=dict(type='str', aliases=['providerType'], default='org.keycloak.storage.UserStorageProvider'),
parent_id=dict(type='str', aliases=['parentId']),
remove_unspecified_mappers=dict(type='bool', default=True),
exclude_bind_credential_from_update_check=dict(type='bool', default=False),
mappers=dict(type='list', elements='dict', options=mapper_spec),
)

Expand Down Expand Up @@ -869,8 +880,9 @@ def main():

# Filter and map the parameters names that apply
comp_params = [x for x in module.params
if x not in list(keycloak_argument_spec().keys()) + ['state', 'realm', 'mappers', 'remove_unspecified_mappers'] and
module.params.get(x) is not None]
if x not in list(keycloak_argument_spec().keys())
+ ['state', 'realm', 'mappers', 'remove_unspecified_mappers', 'exclude_bind_credential_from_update_check']
and module.params.get(x) is not None]

# See if it already exists in Keycloak
if cid is None:
Expand Down Expand Up @@ -1012,8 +1024,14 @@ def main():
if state == 'present':
# Process an update

desired_copy = deepcopy(desired_comp)
before_copy = deepcopy(before_comp)
# exclude bindCredential when checking wether an update is required
if module.params['exclude_bind_credential_from_update_check']:
desired_copy.get('config', []).pop('bindCredential', None)
before_copy.get('config', []).pop('bindCredential', None)
# no changes
if desired_comp == before_comp:
if desired_copy == before_copy:
result['changed'] = False
result['end_state'] = sanitize(desired_comp)
result['msg'] = "No changes required to user federation {id}.".format(id=cid)
Expand Down