New Module: Keycloak User #3996

fynncfchen · 2022-01-06T14:56:48Z

SUMMARY

Add keycloak_user to provide user management via Keycloak Admin API.

ISSUE TYPE

New Module Pull Request

COMPONENT NAME

keycloak_user

ADDITIONAL INFORMATION

Usage:

- name: Create a Keycloak user, authentication with credentials
  community.general.keycloak_user:
    username: my-new-kc-user
    realm: MyCustomRealm
    state: present
    auth_client_id: admin-cli
    auth_keycloak_url: https://auth.example.com/auth
    auth_realm: master
    auth_username: USERNAME
    auth_password: PASSWORD
  delegate_to: localhost

- name: Create a Keycloak user, authentication with token
  community.general.keycloak_user:
    username: my-new-kc-user
    realm: MyCustomRealm
    state: present
    auth_client_id: admin-cli
    auth_keycloak_url: https://auth.example.com/auth
    token: TOKEN
  delegate_to: localhost

- name: Delete a keycloak user
  community.general.keycloak_user:
    id: '9d59aa76-2755-48c6-b1af-beb70a82c3cd'
    state: absent
    realm: MyCustomRealm
    auth_client_id: admin-cli
    auth_keycloak_url: https://auth.example.com/auth
    auth_realm: master
    auth_username: USERNAME
    auth_password: PASSWORD
  delegate_to: localhost

- name: Delete a Keycloak user based on username
  community.general.keycloak_user:
    username: my-user-for-deletion
    state: absent
    auth_client_id: admin-cli
    auth_keycloak_url: https://auth.example.com/auth
    auth_realm: master
    auth_username: USERNAME
    auth_password: PASSWORD
  delegate_to: localhost

- name: Update the first name of a Keycloak user
  community.general.keycloak_user:
    id: '9d59aa76-2755-48c6-b1af-beb70a82c3cd'
    first_name: an-updated-kc-user-first-name
    state: present
    auth_client_id: admin-cli
    auth_keycloak_url: https://auth.example.com/auth
    auth_realm: master
    auth_username: USERNAME
    auth_password: PASSWORD
  delegate_to: localhost

- name: Create a keycloak user with credentials
  community.general.keycloak_user:
    auth_client_id: admin-cli
    auth_keycloak_url: https://auth.example.com/auth
    auth_realm: master
    auth_username: USERNAME
    auth_password: PASSWORD
    username: my-new_user
    credentials:
      - priority: 0
        type: password
        temporary: false
        value: 'PASSWORD'
  delegate_to: localhost

ansibullbot · 2022-01-06T15:03:14Z

cc @eikef @ndclt
click here for bot help

felixfontein · 2022-01-30T21:43:03Z

@fynncfchen @pmdumuid what's the current state of this PR? @fynncfchen are you planning to do more changes? @pmdumuid do you think there should be more changes?

fynncfchen · 2022-01-31T05:51:47Z

I'm good, thanks, any more comments @pmdumuid?

pmdumuid · 2022-01-31T11:25:33Z

@felixfontein - I presume there should be some commit fragment files created with this PR?

There's a lot of reformatting that occurred within this PR (within the file, plugins/module_utils/identity/keycloak/keycloak.py) Is there any issue with this?

I'll take another look at this PR tonight, (comments coming shortly)

pmdumuid · 2022-01-31T11:29:54Z

plugins/module_utils/identity/keycloak/keycloak.py

+                            data=json.dumps(userrep), validate_certs=self.validate_certs)
+        except Exception as e:
+            self.module.fail_json(msg="Could not create user %s in realm %s: %s"
+                                      % (userrep['user_username'], realm, str(e)))


Why is the key, user_username being used in the error message?
There seems to be no test to ensure that when creating a user, the username has been specified.

The API docs also indicate that the username field is actually username and not user_username - see https://www.keycloak.org/docs-api/12.0/rest-api/#_userrepresentation.

pmdumuid · 2022-01-31T11:33:31Z

plugins/module_utils/identity/keycloak/keycloak.py

+                            data=json.dumps(userrep), validate_certs=self.validate_certs)
+        except Exception as e:
+            self.module.fail_json(msg='Could not update user %s in realm %s: %s'
+                                      % (userrep['username'], realm, str(e)))


This error message expects username to be present, but your document states:

This parameter is required only when creating or updating the user

Can I suggest this be swapped with:

userrep.get('username', "<User-CID:%s>" % userrep.get('id', None))

so that an error isn't thrown whilst trying to formulate a nice error message.

pmdumuid · 2022-01-31T11:34:53Z

plugins/module_utils/identity/keycloak/keycloak.py

+        if id is None and username is None:
+            # prefer an exception since this is almost certainly a programming error in the module itself.
+            raise Exception(
+                "Unable to delete user - one of user ID or username must be provided.")


This test for is or username being present should also exist for update_user.

We should also throw an error if both are provided.

pmdumuid · 2022-01-31T11:37:03Z

plugins/modules/identity/keycloak/keycloak_user.py

+
+    module = AnsibleModule(argument_spec=argument_spec,
+                           supports_check_mode=True,
+                           required_one_of=([['id', 'username'],


Would it be beneficial to use mutually_exclusive i.e. to allow username, or id but not both? - See https://docs.ansible.com/ansible/latest/dev_guide/developing_program_flow_modules.html#argument-spec

I agree with you, for sure. The only reason why I choose to have similar solutions from other files instead of best solution, is that I think keeping consistency for those files is more important in the first place, then we can think about to refactoring later.

Adding such restrictions to existing modules is a breaking change and needs a long deperecation period (and extra code to inform users about it), that's usually the main reason for it not being done. But for new modules we should better stick to the best solution :)

This is a brand new module (keycloak_user.py didn't exist prior to this PR), so I didn't think the concern of deprecation applied, (i.e we should instead try to get it as right as possible up front)

pmdumuid · 2022-01-31T11:38:56Z

plugins/modules/identity/keycloak/keycloak_user.py

+    argument_spec = keycloak_argument_spec()
+
+    meta_args = dict(
+        # Remove alias `username` on `auth_username`


Could I propose, (not for this PR) we remove the alias, username and just require all to use auth_username to avoid any ambiguity?

Good suggestion, but I think that's a big deal, we can have a dedicated PR for this, how about that?

Agree, (hence why I said, "not for this PR").

pmdumuid · 2022-01-31T11:43:53Z

plugins/module_utils/identity/keycloak/keycloak.py

+
+        :param username: The username of the user. A lookup will be performed to retrieve the user ID.
+        :param realm: Return the groups of this realm (default "master").
+        """


Why does this need to return a list (and the received thus have a lot of logic around the response, and so forth)?

Surely, keycloak only allows one user with a username per realm, so the response could be None or the user object rather than a list of objects.

The calling code could then be reduced to:

found_user = kc.get_user_by_username(username, realm=realm) before_user = {} if not found_user else found_user

pmdumuid · 2022-01-31T11:45:29Z

plugins/modules/identity/keycloak/keycloak_user.py

+        after_user = kc.get_users_by_username(username, realm=realm)[0]
+
+        result['end_state'] = after_user
+        result['user'] = result['end_state']


Is there a benefit to populating both 'user' and 'end_state'? Can I suggest we just go with 'end_state'? (as per all the other key cloak modules.

AFAIK the 'user' object is to keep consistency with other module:

- Deprecated return value, it will be removed in community.general 6.0.0. Please use the return value I(end_state) instead.

but some of the module doesn't have such object, so I have no idea for which I should follow or not.
Maybe we can keep consistency with this and remove it on version 6.0.0?

I would rather not introduce it in the first place. It is only left in the other modules because it will be removed eventually, there's no need to add more cruft that needs to be removed :)

I agree, this this is a new module (keycloak_user.py didn't exist prior to this PR) so I don't think we should be adding it.

pmdumuid · 2022-01-31T11:47:42Z

plugins/modules/identity/keycloak/keycloak_user.py

+            result['changed'] = True
+
+            if module._diff:
+                result['diff'] = dict(before=before_user, after='')


Given that the before_user is a dict, I think it's best if the after is also a dict rather than an empty string.

pmdumuid · 2022-01-31T11:50:15Z

plugins/modules/identity/keycloak/keycloak_user.py

+    if found_users is None or found_users == []:
+        before_user = {}
+    else:
+        before_user = found_users[0]


The documents above indicate say that username is only required when doing a create, but I can't (from the code) see how the value of before_user (which is used below when deleting a user) is populated properly when the username is not supplied.

pmdumuid · 2022-01-31T11:51:48Z

I think it would be beneficial to add some integration tests (for example as described in ./tests/integration/targets/keycloak_role/README.md). This could test out the user of deleting and updating a user by an ID which I'm not certain works, (nor if mock-based testing tests this sufficiently)

felixfontein · 2022-01-31T18:42:33Z

@felixfontein - I presume there should be some commit fragment files created with this PR?

You mean changelog fragment - these are not required for new modules, so none is needed here.

There's a lot of reformatting that occurred within this PR (within the file, plugins/module_utils/identity/keycloak/keycloak.py) Is there any issue with this?

I would really avoid reformatting.

felixfontein · 2022-02-14T19:00:46Z

plugins/module_utils/identity/keycloak/keycloak.py

+        users_url = URL_USERS.format(url=self.baseurl, realm=realm)
+        try:
+            return open_url(users_url, method='POST', headers=self.restheaders,
+                            data=json.dumps(userrep), validate_certs=self.validate_certs)


This (and others) need to be adjusted similar to #4178.

tleguern · 2022-03-04T16:38:45Z

plugins/modules/identity/keycloak/keycloak_user.py

+            # Process an update
+
+            # no changes
+            if desired_user == before_user:


@fynncfchen First, thank you for the module.I have been testing it against both keycloak 14 and 16.1 during the day and I think I found a bug.Not sure how to fix it sadly.

Consider the following snippet:

- keycloak_user: auth_client_id: admin-cli auth_keycloak_url: ... token: ... username: test realm: master credentials: - type: password temporary: false value: test state: present

During the first execution the user test will be correctly created (“User test has been created with ID blabla”).
On subsequent runs instead of doing nothing the user is updated every time.
In these cases the variables before_user and after_user are strictly identical.
The difference comes with desired_users which contains a key credentials while before_user and after_user don't.

Obviously keycloak is not going to return clear text passwords to compare against and simply comparing before_user and after_user is not going to catch a password change.

felixfontein · 2022-04-23T09:53:15Z

needs_info

felixfontein · 2022-04-26T20:09:33Z

This New Module PR contains a symbolic link from plugins/modules/ to the actual Python file. Since #4562 this is no longer necessary and will soon be flagged as an error. Instead you need to add an entry to meta/runtime.yml, similar to this one: https://github.com/ansible-collections/community.general/blob/main/meta/runtime.yml#L21-L22

See also our updated instructions on creating new modules: https://github.com/ansible-collections/community.general/blob/main/CONTRIBUTING.md#creating-new-modules-or-plugins

ansibullbot · 2022-05-29T06:27:37Z

@fynncfchen This pullrequest is waiting for your response. Please respond or the pullrequest will be closed.

click here for bot help

ansibullbot · 2022-06-30T06:45:16Z

@fynncfchen You have not responded to information requests in this pullrequest so we will assume it no longer affects you. If you are still interested in this, please create a new pullrequest with the requested information.

click here for bot help

fynncfchen added 7 commits January 6, 2022 22:24

feat(plugins/keycloak): add URL constants

6df69eb

feat(plugins/keycloak): add create_user in utils

928522f

feat(plugins/keycloak): add get_users_by_username as util function

cc7738e

feat(plugins/keycloak): add update_user as an util function

3cd6fa9

feat(plugins/keycloak): add delete_user as an util function

0c74514

feat(plugins/keycloak): add keycloak_user module

3809a22

refactor(plugins/keycloak): create symlink for keycloak_user

f6b7f30

This comment has been minimized.

Sign in to view

ansibullbot added identity module module module_utils module_utils needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR new_contributor Help guide this first time contributor new_plugin New plugin plugins plugin (any type) labels Jan 6, 2022

fynncfchen added 2 commits January 6, 2022 23:13

docs: add docs for keycloak_user

b3c0ad7

fix: fix import-before-documentation

ed6b909

felixfontein added backport-4 check-before-release PR will be looked at again shortly before release and merged if possible. labels Jan 6, 2022

fix(plugins/keycloak): fix invalid-ansiblemodule-schema

3871475

8000

This comment has been minimized.

Sign in to view

docs: add docs for keycloak_user

857e7a7

This comment has been minimized.

Sign in to view

fynncfchen added 2 commits January 6, 2022 23:42

style: fix for 2 blank lines

2a7e388

docs(plugins/keycloak): fix doc error

d39dc11

This comment has been minimized.

Sign in to view

docs(plugins/keycloak): fix document error

1780386

ansibullbot added the ci_verified Push fixes to PR branch to re-run CI label Jan 6, 2022

chore: add maintainer in BOTMETA

1df0149

docs(plugins/keycloak): add description of not_before

a648e56

pmdumuid reviewed Jan 31, 2022

View reviewed changes

ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Feb 8, 2022

felixfontein reviewed Feb 14, 2022

View reviewed changes

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 labels Feb 14, 2022

tleguern reviewed Mar 4, 2022

View reviewed changes

ansibullbot added the needs_info This issue requires further information. Please answer any outstanding questions label Apr 23, 2022

felixfontein removed the backport-4 label Apr 27, 2022

ansibullbot closed this Jun 30, 2022

New Module: Keycloak User #3996

New Module: Keycloak User #3996

Uh oh!

Conversation

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

Uh oh!

This comment has been minimized.

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!