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.

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: Authentication management (flows and required actions) #6616

Closed
wants to merge 3 commits into from

Conversation

Skrekulko
Copy link
Contributor

SUMMARY

This is basically a complete rework of the current keycloak_authentication module, that until now, was quite limited (in the way of handling and searching for the right flow or execution). With this new module you can:

  • Flow:
    • Create, create a new flow from a copy of an existing flow, update a flow, and delete a flow.
    • Add or update an execution (step or sub-flow).
  • Required action:
    • Register, update and delete a required action.
ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

keycloak_authentication

ADDITIONAL INFORMATION

Deletion of an execution (step or sub-flow) is still not supported. (Might add later on.)

The biggest issue I've found, is that Keycloak has a really cumbersome way of identifying different flows and executions. If the user doesn't have the ID, there's no guarantee, that the user can find the exact one (since there can be duplicates of some executions differing only by ID, etc.). This module thus does a "best-effort" search, by using the input parameters of the user, and tries to find the closest match (which is also an improvement of the previous version of this module).

Some new or reworked methods for calling the Keycloak API are introduced (keycloak module). Also, a new set of unit tests is included (test_keycloak_authentication).

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added feature This issue/PR relates to a feature request identity module module module_utils module_utils new_contributor Help guide this first time contributor plugins plugin (any type) tests tests unit tests/unit labels Jun 2, 2023
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-7 labels Jun 4, 2023
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! I've added a first comment. Please note that due to the refactoring the diff is very complex and hard to review. I hope that one of the module maintainers will review it.

Please make sure to mark all new module options with version_added: 7.1.0. Also please make sure you do not change any existing module options or module behaviors in non-backwards compatible ways; otherwise this PR cannot be merged.

@Skrekulko
Copy link
Contributor Author

Thanks for your contribution! I've added a first comment. Please note that due to the refactoring the diff is very complex and hard to review. I hope that one of the module maintainers will review it.

Please make sure to mark all new module options with version_added: 7.1.0. Also please make sure you do not change any existing module options or module behaviors in non-backwards compatible ways; otherwise this PR cannot be merged.

I'm not sure this can make it into the 7.1.0, since I introduced breaking changes (options and behavior). The reason is that the name of the module keycloak_authentication indicates, that it should manage Keycloak authentication, which in this case are flows, required actions, and policies (not implemented yet). I tried to avoid doing something like keycloak_authentication_required_actions when implementing the required actions, because I believe, that it would just break the consistency of the naming system that's currently used.

This basically forced me to do, what I did during the "refac". Many options for flows and required actions have the same name, thus I had to put them under their corresponding options (flow, required_action, etc...). I tried to stick to the official Keycloak API documentation as close as possible, which also lead to the current structure of the options.

Anyway, I apologize if I did not make this clear at the beginning. The only viable solution for now, is to wait and merge this PR into a new major version (8.0.0).

@felixfontein
Copy link
Collaborator

Hmm, unfortunately these breaking changes also mean that it won't be possible to merge it for 8.0.0. Breaking changes (with very few exceptions) need a long deprecation period, which should allow to use both the old and new behavior (usually with a switch).

I don't know Keycloak (even though I help getting PRs merged for the keycloak modules) so I have no idea whether there is a better way to do this for this specific module / your changes. Maybe someone of the module maintainers will chime in?

@ansibullbot ansibullbot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR stale_ci CI is older than 7 days, rerun before merging and removed new_contributor Help guide this first time contributor labels Jun 20, 2023
@Skrekulko
Copy link
Contributor Author

I'll be closing this PR, since I separated the required actions from it in #6732, did a small bugfix in #6734 and also some minor changes in #6763. As I introduced breaking changes, and the module is not deprecated, it cannot be merged for now.

@Skrekulko Skrekulko closed this Jun 21, 2023
@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This issue/PR relates to a feature request identity module_utils module_utils module module needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR plugins plugin (any type) stale_ci CI is older than 7 days, rerun before merging tests tests unit tests/unit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants