Skip to content

Fix Keycloak authentication flow configuration issues #9987

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

Merged

Conversation

desand01
Copy link
Contributor

SUMMARY

Fix authentification flow configuration duplication

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

keycloak_authentication.py
keycloak.py

ADDITIONAL INFORMATION

Modification to authentification flow configuration cause the creation of a duplicate record in AUTHENTICATOR_CONFIG and AUTHENTICATOR_CONFIG_ENTRY

KC26 exported realm:

    {
      "id": "f76d998c-6eb4-42fa-b3b1-df8ae2be926c",
      "alias": "my-conditional-reset-otp-config",
      "config": {
        "defaultOtpOutcome": "force",
        "noOtpRequiredForHeaderPattern": "X-Forwarded-For: 10\\.[0-9\\.:]+"
      }
    },
    {
      "id": "e195fad8-4470-47be-ada2-ecf44146a0eb",
      "alias": "my-conditional-reset-otp-config",
      "config": {
        "defaultOtpOutcome": "force",
        "noOtpRequiredForHeaderPattern": "X-Original-Forwarded-For: 10.[0-9\\.:]+"
      }
    }

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added bug This issue/PR relates to a bug identity module module module_utils module_utils plugins plugin (any type) labels Apr 11, 2025
@ansibullbot ansibullbot added integration tests/integration tests tests labels Apr 12, 2025
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-9 Automatically create a backport for the stable-9 branch backport-10 Automatically create a backport for the stable-10 branch labels Apr 12, 2025
@felixfontein
Copy link
Collaborator

Thanks for your contribution! From an Ansible point of view it looks good to me; I can't say anything about the change itself :)

If nobody objects, I'll merge this at the end of the upcoming week.

@desand01
Copy link
Contributor Author

During testing, I realized that the issue seems to be fixed in KC 26.2.0+

@felixfontein
Copy link
Collaborator

Does the fix break anything with 26.2.0+? If not, it's probably best to adjust the changelog fragment to mention that the fix is only needed for < 26.2.0.

Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

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

Couple of comments there.

Comment on lines 9 to 11
# environment:
# https_proxy: http://10.249.120.90:8080
# http_proxy: http://10.249.120.90:8080
Copy link
Collaborator

Choose a reason for hiding this comment

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

This shouldn't be here, I suppose ;-)

return_content: true
status_code: 200
headers:
X-Requested-By: "Jenkins"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curiosity, is this required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Look like it's useless

Copy link
Collaborator

Choose a reason for hiding this comment

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

The test container start (and implicit stop) could be added directly into the test itself. See:

https://github.com/russoz-ansible/community.general/blob/main/tests/integration/targets/mssql_script/tasks/main.yml

as an example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, done

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry I wasn't very thorough in my directions. Please check the meta stuff in the mssql_script integration test as well, with the dependency to the setup_docker role, to ensure that docker is installed (and uninstalled after the test).

To ensure everything is working as it should, it is suggested to run the test on a clean VM (using vagrant could make that easier).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we want to run these tests in CI. That's something that should happen once all Keycloak stuff is moved to its own collection, and then it's time to figure out there how integration tests should run. Investing too much time into that now is potentially wasting resources that could be spent better after a move. (Of course, assuming that a move will happen.)

Andre Desrosiers added 2 commits April 14, 2025 08:14
@felixfontein
Copy link
Collaborator

BTW, @desand01 you might be interested in https://forum.ansible.com/t/keycloak-modules-in-community-general/41746

@desand01
Copy link
Contributor Author

@felixfontein thanks, I'm going to register and follow the discussion

@russoz
Copy link
Collaborator

russoz commented Apr 16, 2025

It seems that changes introduced by KC 26 might be turning into a common theme - see #9983 - maybe we should define a common strategy for these (and future) changes related to that versioning.

Cc: @fgruenbauer

@felixfontein
Copy link
Collaborator

That's another reason why a common collection for all Keycloak modules and plugins would be a great thing, so that there's a good place where such discussions can take place :)

@felixfontein felixfontein merged commit a8b9773 into ansible-collections:main Apr 19, 2025
141 checks passed
Copy link

patchback bot commented Apr 19, 2025

Backport to stable-9: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-9/a8b977320c04a75069f4208cf723743b53ed0d20/pr-9987

Backported as #10017

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Apr 19, 2025
patchback bot pushed a commit that referenced this pull request Apr 19, 2025
* Add delete_authentication_config method and integrate it into create_or_update_executions

* typo

* Sanity

* Add integration tests for keycloak_authentication module with README, tasks, and variables

* Add copyright and license information to access_token.yml

* Sanity

* Refactor Keycloak integration tests: streamline README, update access token task, and enhance variable management

* Maj changelogs fragments

---------

Co-authored-by: Andre Desrosiers <[email protected]>
(cherry picked from commit a8b9773)
Copy link

patchback bot commented Apr 19, 2025

Backport to stable-10: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-10/a8b977320c04a75069f4208cf723743b53ed0d20/pr-9987

Backported as #10018

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@felixfontein
Copy link
Collaborator

@desand01 thanks for your contribution!
@russoz thanks for reviewing!

patchback bot pushed a commit that referenced this pull request Apr 19, 2025
* Add delete_authentication_config method and integrate it into create_or_update_executions

* typo

* Sanity

* Add integration tests for keycloak_authentication module with README, tasks, and variables

* Add copyright and license information to access_token.yml

* Sanity

* Refactor Keycloak integration tests: streamline README, update access token task, and enhance variable management

* Maj changelogs fragments

---------

Co-authored-by: Andre Desrosiers <[email protected]>
(cherry picked from commit a8b9773)
felixfontein pushed a commit that referenced this pull request Apr 19, 2025
…low configuration issues (#10018)

Fix Keycloak authentication flow configuration issues (#9987)

* Add delete_authentication_config method and integrate it into create_or_update_executions

* typo

* Sanity

* Add integration tests for keycloak_authentication module with README, tasks, and variables

* Add copyright and license information to access_token.yml

* Sanity

* Refactor Keycloak integration tests: streamline README, update access token task, and enhance variable management

* Maj changelogs fragments

---------

Co-authored-by: Andre Desrosiers <[email protected]>
(cherry picked from commit a8b9773)

Co-authored-by: desand01 <[email protected]>
felixfontein pushed a commit that referenced this pull request Apr 19, 2025
…ow configuration issues (#10017)

Fix Keycloak authentication flow configuration issues (#9987)

* Add delete_authentication_config method and integrate it into create_or_update_executions

* typo

* Sanity

* Add integration tests for keycloak_authentication module with README, tasks, and variables

* Add copyright and license information to access_token.yml

* Sanity

* Refactor Keycloak integration tests: streamline README, update access token task, and enhance variable management

* Maj changelogs fragments

---------

Co-authored-by: Andre Desrosiers <[email protected]>
(cherry picked from commit a8b9773)

Co-authored-by: desand01 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-9 Automatically create a backport for the stable-9 branch backport-10 Automatically create a backport for the stable-10 branch bug This issue/PR relates to a bug identity integration tests/integration module_utils module_utils module module plugins plugin (any type) tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants