jenkins_plugin: also use false value for force_basic_auth param #9130
Conversation
|
The test The test The test The test The test The test The test |
ansibullbot
added
bug
felixfontein
added
check-before-release
There was a problem hiding this comment.
Thanks for your contribution!
I noticed this was not actually being used in the jenkins plugin module.
That's right, because the parameter is handled by fetch_url: https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/urls.py#L1199
Basically the module always forces the parameter to true, and ignores that the user might have explicitly said false.
| @@ -821,6 +844,7 @@ def main(): | |||
| url_password=dict(no_log=True), | |||
| version=dict(), | |||
| with_dependencies=dict(default=True, type='bool'), | |||
| force_basic_auth=dict(default=False, type='bool') # Add force_basic_auth to arguments | |||
There was a problem hiding this comment.
For backwards compatibility the default needs to be True. Also please remove the comment, and add a trailing comma:
| force_basic_auth=dict(default=False, type='bool') # Add force_basic_auth to arguments | |
| force_basic_auth=dict(default=True, type='bool'), |
You also need to add this default to the documentation in DOCUMENTATION, something like:
options:
[...]
force_basic_auth:
default: true|
|
||
| # If force_basic_auth is True, add the Authorization header | ||
| if self.force_basic_auth: | ||
| headers["Authorization"] = self._create_basic_auth_header() |
There was a problem hiding this comment.
I don't think this assignment/computation is necessary since fetch_url is already doing that.
| @@ -0,0 +1,2 @@ | |||
| bugfixes: | |||
| - jenkins_plugin - use force_basic_auth param to create basic auth headers if set to true | |||
There was a problem hiding this comment.
| - jenkins_plugin - use force_basic_auth param to create basic auth headers if set to true | |
| - jenkins_plugin - actually use the value of the ``force_basic_auth`` parameter. Until now the module behaved as if it was set to ``true`` (https://github.com/ansible-collections/community.general/pull/9130). |
felixfontein
added
the
backport-9
felixfontein
changed the title
| custom_headers = kwargs.get('headers', {}) | ||
|
|
||
| # Merge custom headers with self.crumb, ensuring self.crumb is always included | ||
| headers = {**self.crumb, **custom_headers} |
There was a problem hiding this comment.
By my testing, this will always give priority to elements in custom_headers. Do we really want that? Is there anything users might add in headers that would overwrite something from self.crumb and screw the module up? If so, do we care to shield the module from that or do we delegate to the user?
|
@AMartindale98 ping! needs_info |
ansibullbot
added
the
needs_info
SUMMARY
force_basic_auth param is specified in documentation: https://docs.ansible.com/ansible/latest/collections/community/general/jenkins_plugin_module.html
I noticed this was not actually being used in the jenkins plugin module. We need to set authorization headers when downloading plugins in certain situations so if force_basic_auth is set to true, we create basic auth headers.
ISSUE TYPE
jenkins_plugin
ADDITIONAL INFORMATION
We found that when we have SSO enabled on our Jenkins instance, we have issues with retrieving a crumb to download our plugins: https://webpagefx.mangoapps.com/msc/MjAyNTYyNl8xNDIwNTQ1Mw
We can get around this problem by using an API token instead, but we need to create basic auth headers and use the API token in place of the password - this can be controlled by the force_basic_auth param which previously was not being used in the module.
After this update, we are able to install plugins without issues: https://webpagefx.mangoapps.com/msc/MjAzNjk4MV8xNDIzMDMyOQ