Skip to content

keycloak_identity_provider: add hideOnLogin parameter to module #9983

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+12 −0
Open

keycloak_identity_provider: add hideOnLogin parameter to module #9983

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
84f5b7a
add `hideOnLogin` parameter to keycloak idp module
fgruenbauer Apr 10, 2025
ab8dc5d
add changelog fragment
fgruenbauer Apr 14, 2025
4c2c19a
fix option
fgruenbauer Apr 14, 2025
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
2 changes: 2 additions & 0 deletions changelogs/fragments/9983-keycloak_idp-add-hideOnLogin-param.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
bugfixes:
- keycloak_identity_provider - add ``hideOnLogin`` parameter to the module, the old parameter ``config/hideOnLoginPage`` is kept for compatibility (https://github.com/ansible-collections/community.general/pull/9983).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- keycloak_identity_provider - add ``hideOnLogin`` parameter to the module, the old parameter ``config/hideOnLoginPage`` is kept for compatibility (https://github.com/ansible-collections/community.general/pull/9983).
- keycloak_identity_provider - add ``hideOnLogin`` parameter to the module, the old parameter ``config.hideOnLoginPage`` is kept for compatibility (https://github.com/ansible-collections/community.general/pull/9983).

10 changes: 10 additions & 0 deletions plugins/modules/keycloak_identity_provider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,15 @@
- providerId
type: str

hide_on_login:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is it being created at the top level and not as a suboption of config? I understand it could generate some confusion, but that would be temporary and could be mitigated with documentation.

Aren't we breaking from the pattern used in this module?

Copy link
Collaborator

Choose a reason for hiding this comment

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

My guess is that the parameters for this module are more or less identical (up to some parameters that will be removed) to what the Keycloak API expects. In the API the value was moved from config to top-level, so this PR mirrors that.

Whether that's a good pattern - I don't know. Keeping the value in config and solving this in the code could be a liability for the future, if the value inside config is re-used for something else by Keycloak. (Maybe that even was the reason why it was moved away from config in Keycloak?)

Copy link
Contributor Author

@fgruenbauer fgruenbauer Apr 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, in the Keycloak API the parameter was moved to the top-level, which i tried to mirror. I don't know why it was changed, though.

I thought accepting both parameters would be the most flexible solution. Users could provide the module with client configs (for example from an Keycloak config export) from both Keycloak 25 and 26 without having to edit them. Which would also allow users to update Keycloak to 26 without having to edit their existing client configs. But its also confusing to have two parameter for the same Keycloak setting.

I think, determining in code what parameter the API expects is good idea. If we accept only one parameter, i think we have to, otherwise it wouldn't work with the other Keycloak version.

It would be nice to fetch the version only once an share it between all Keycloak modules. Is it possible to set Keycloak module defaults from within a module? Otherwise it might be a good idea to allow users to provide a value, so they can make a single version request in the setup phase and use the value for all Keycloak module calls.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to set Keycloak module defaults from within a module?

That's not possible. The only thing modules can do is set a fact, but they can't read it. (You could add an action plugin for that part, but that's "magic" with its own set of problems.)

Might be worth thinking about once the modules have been moved to a separate collection, though. I don't really want such "magic" in this collection though, it's already does way too much :)

description:
- If hidden, login with this provider is possible only if requested explicitly, for example using the C(kc_idp_hint)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you forgot one line from the original parameter:

Suggested change
- If hidden, login with this provider is possible only if requested explicitly, for example using the C(kc_idp_hint)
- If hidden, login with this provider is possible only if requested explicitly, for example using the C(kc_idp_hint)
parameter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

BTW, what/where is this kc_idp_hint parameter? It does not seem to be in the module whatsoever. Should it be? Should we use O() instead of C()?

- Parameter was added in Keycloak 26, for older Keycloak versions use O(config.hide_on_login_page)
aliases:
- hideOnLogin
type: bool
version_added: 10.6.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs updating

Suggested change
version_added: 10.6.0
version_added: 11.0.0


config:
description:
- Dict specifying the configuration options for the provider; the contents differ depending on the value of O(provider_id).
Expand Down Expand Up @@ -490,6 +499,7 @@ def main():
provider_id=dict(type='str', aliases=['providerId']),
store_token=dict(type='bool', aliases=['storeToken']),
trust_email=dict(type='bool', aliases=['trustEmail']),
hide_on_login=dict(type='bool', aliases=['hideOnLogin']),
mappers=dict(type='list', elements='dict', options=mapper_spec),
)

Expand Down