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

New Module: Keycloak ClientSecret #3997

Closed

Conversation

fynncfchen
Copy link
Contributor

SUMMARY

Add keycloak_clientsecret to provide management of client secret via Keycloak Admin API.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

keycloak_clientsecret

ADDITIONAL INFORMATION

Example:

- name: Get a Keycloak client secret, authentication with credentials
  community.general.keycloak_clientsecret:
    id: '9d59aa76-2755-48c6-b1af-beb70a82c3cd'
    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: Generate a new Keycloak client secret, authentication with token
  community.general.keycloak_user:
    id: '9d59aa76-2755-48c6-b1af-beb70a82c3cd'
    realm: MyCustomRealm
    state: absent
    auth_client_id: admin-cli
    auth_keycloak_url: https://auth.example.com/auth
    token: TOKEN
  delegate_to: localhost

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added identity module module module_utils module_utils new_contributor Help guide this first time contributor new_plugin New plugin plugins plugin (any type) labels Jan 6, 2022
@ansibullbot

This comment has been minimized.

@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jan 6, 2022
@felixfontein felixfontein added backport-4 check-before-release PR will be looked at again shortly before release and merged if possible. labels Jan 6, 2022
@ansibullbot ansibullbot removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jan 7, 2022
@fynncfchen fynncfchen changed the title Feature: Keycloak ClientSecret New Plugin: Keycloak ClientSecret Jan 8, 2022
@fynncfchen fynncfchen changed the title New Plugin: Keycloak ClientSecret New Module: Keycloak ClientSecret Jan 8, 2022
@ansibullbot ansibullbot added stale_ci CI is older than 7 days, rerun before merging and removed new_contributor Help guide this first time contributor labels Jan 16, 2022
@felixfontein
Copy link
Collaborator

CC @pmdumuid @laurpaum

Co-authored-by: Felix Fontein <felix@fontein.de>
@ansibullbot ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI and removed stale_ci CI is older than 7 days, rerun before merging needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI labels Jan 24, 2022
@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Feb 1, 2022

try:
return json.loads(to_native(open_url(clientsecret_url, method='POST', headers=self.restheaders,
validate_certs=self.validate_certs).read()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@felixfontein
Copy link
Collaborator

needs_info

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

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

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.

Hi @fynncfchen , thanks for your contribution. I am leaving a couple of suggestions around, but they don't affect the outcome of the module itself. Other than that (and Felix's comments), looking good.

'''

RETURN = '''
msg:
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be Nice-to-Have(TM) consistency in the indentation on the YAML blocks. In some places it is 2 spaces, in others 4 spaces.

result['end_state'] = clientsecret
result['msg'] = 'Get client secret successful for ID {id}'.format(id=id)

module.exit_json(**result)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line is redundant, as the exact same command will run after the if-else block in line 232

result['changed'] = False
result['end_state'] = {}
result['msg'] = 'State not specified; doing nothing.'
module.exit_json(**result)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line is redundant, as the exact same command will run after the if-else block in line 232

# only lookup the client_id if id isn't provided.
# in the case that both are provided, prefer the ID, since it's one
# less lookup.
if id is None and client_id is not None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Because of the required_one_of spec in the Module, on line 165, this could be simplified to:

Suggested change
if id is None and client_id is not None:
if id is None:


argument_spec.update(meta_args)

module = AnsibleModule(argument_spec=argument_spec,
Copy link
Collaborator

Choose a reason for hiding this comment

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

From the examples, it looks like one must pass (auth_realm, auth_username, auth_password) or token, but not both.

If that's indeed the case, there should be a mutually_exclusive clause here.

@ansibullbot
Copy link
Collaborator

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

click here for bot help

@ansibullbot
Copy link
Collaborator

@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

@ansibullbot ansibullbot closed this Jul 2, 2022
@github-actions
Copy link

github-actions bot commented Jul 2, 2022

Docs Build 📝

This PR is closed and any previously published docsite has been unpublished.

@johncant
Copy link
Contributor

I would find the functionality in this PR by @fynncfchen useful. @russoz - would you merge if I made your changes?

@russoz
Copy link
Collaborator

russoz commented Nov 25, 2022

Hi @johncant , that's the purpose of PRs :).

Just read it carefully, as Felix has also requested some changes as well.

@johncant
Copy link
Contributor

Thanks @russoz @felixfontein @fynncfchen - now done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
check-before-release PR will be looked at again shortly before release and merged if possible. identity module_utils module_utils module module needs_info This issue requires further information. Please answer any outstanding questions new_plugin New plugin plugins plugin (any type) stale_ci CI is older than 7 days, rerun before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants