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

bitbucket: Support Basic Auth #2045

Merged

Conversation

maxbrunet
Copy link
Contributor

@maxbrunet maxbrunet commented Mar 19, 2021

SUMMARY

Support Basic Auth

TODO:

  • Add support for Basic Auth in module util
  • Settle on usage of username parameter (and potentially on how migrate/deprecate as needed)
    • Option 1. Rename current username to workspace to follow API semantic (that did confuse me at first as my company is not a username)
    • Option 2. Use user for Basic Auth username (risk to be confusing, deprecate current usage of username)
    • Option 3. ?
  • Update modules documentation accordingly
  • Update tests
  • Anything else?

Close #2044

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

bitbucket_access_key
bitbucket_pipeline_key_pair
bitbucket_pipeline_known_host
bitbucket_pipeline_variable

ADDITIONAL INFORMATION

Bitbucket API - Authentication methods:
https://developer.atlassian.com/bitbucket/api/2/reference/meta/authentication#app-pw

@maxbrunet maxbrunet marked this pull request as draft March 19, 2021 04:16
@ansibullbot ansibullbot added affects_2.10 community_review feature This issue/PR relates to a feature request module_utils module_utils needs_triage new_contributor Help guide this first time contributor plugins plugin (any type) labels Mar 19, 2021
@ansibullbot

This comment has been minimized.

@ansibullbot ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed community_review labels Mar 19, 2021
@maxbrunet
Copy link
Contributor Author

cc @catcombo

@maxbrunet
Copy link
Contributor Author

Hey @felixfontein, I would need some advice on how to handle the username parameter here (please see description)? Are breaking changes allowed for the bitbucket_* modules (at least in the next major version)? Or what would be the best way around it like deprecation?

@felixfontein
Copy link
Collaborator

If username is changed to workspace, then username should be kept as an alias for backwards compatibility. Breaking changes are possible, but should really be avoided if they are not strictly necessary. In general, we announce breaking changes a long time in advance with deprecations.

@maxbrunet maxbrunet force-pushed the feature/bitbucket/basic-auth branch from c624557 to 4387c4b Compare April 12, 2021 04:51
@ansibullbot ansibullbot added community_review and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Apr 12, 2021
@ansibullbot
Copy link
Collaborator

The test ansible-test sanity --test validate-modules [explain] failed with 8 errors:

plugins/modules/source_control/bitbucket/bitbucket_access_key.py:0:0: parameter-type-not-in-doc: Argument 'password' in argument_spec defines type as 'str' but documentation doesn't define type
plugins/modules/source_control/bitbucket/bitbucket_access_key.py:0:0: undocumented-parameter: Argument 'password' is listed in the argument_spec, but not documented in the module documentation
plugins/modules/source_control/bitbucket/bitbucket_pipeline_key_pair.py:0:0: parameter-type-not-in-doc: Argument 'password' in argument_spec defines type as 'str' but documentation doesn't define type
plugins/modules/source_control/bitbucket/bitbucket_pipeline_key_pair.py:0:0: undocumented-parameter: Argument 'password' is listed in the argument_spec, but not documented in the module documentation
plugins/modules/source_control/bitbucket/bitbucket_pipeline_known_host.py:0:0: parameter-type-not-in-doc: Argument 'password' in argument_spec defines type as 'str' but documentation doesn't define type
plugins/modules/source_control/bitbucket/bitbucket_pipeline_known_host.py:0:0: undocumented-parameter: Argument 'password' is listed in the argument_spec, but not documented in the module documentation
plugins/modules/source_control/bitbucket/bitbucket_pipeline_variable.py:0:0: parameter-type-not-in-doc: Argument 'password' in argument_spec defines type as 'str' but documentation doesn't define type
plugins/modules/source_control/bitbucket/bitbucket_pipeline_variable.py:0:0: undocumented-parameter: Argument 'password' is listed in the argument_spec, but not documented in the module documentation

The test ansible-test sanity --test validate-modules [explain] failed with 8 errors:

plugins/modules/source_control/bitbucket/bitbucket_access_key.py:0:0: parameter-type-not-in-doc: Argument 'password' in argument_spec defines type as 'str' but documentation doesn't define type
plugins/modules/source_control/bitbucket/bitbucket_access_key.py:0:0: undocumented-parameter: Argument 'password' is listed in the argument_spec, but not documented in the module documentation
plugins/modules/source_control/bitbucket/bitbucket_pipeline_key_pair.py:0:0: parameter-type-not-in-doc: Argument 'password' in argument_spec defines type as 'str' but documentation doesn't define type
plugins/modules/source_control/bitbucket/bitbucket_pipeline_key_pair.py:0:0: undocumented-parameter: Argument 'password' is listed in the argument_spec, but not documented in the module documentation
plugins/modules/source_control/bitbucket/bitbucket_pipeline_known_host.py:0:0: parameter-type-not-in-doc: Argument 'password' in argument_spec defines type as 'str' but documentation doesn't define type
plugins/modules/source_control/bitbucket/bitbucket_pipeline_known_host.py:0:0: undocumented-parameter: Argument 'password' is listed in the argument_spec, but not documented in the module documentation
plugins/modules/source_control/bitbucket/bitbucket_pipeline_variable.py:0:0: parameter-type-not-in-doc: Argument 'password' in argument_spec defines type as 'str' but documentation doesn't define type
plugins/modules/source_control/bitbucket/bitbucket_pipeline_variable.py:0:0: undocumented-parameter: Argument 'password' is listed in the argument_spec, but not documented in the module documentation

The test ansible-test sanity --test validate-modules [explain] failed with 8 errors:

plugins/modules/source_control/bitbucket/bitbucket_access_key.py:0:0: parameter-type-not-in-doc: Argument 'password' in argument_spec defines type as 'str' but documentation doesn't define type
plugins/modules/source_control/bitbucket/bitbucket_access_key.py:0:0: undocumented-parameter: Argument 'password' is listed in the argument_spec, but not documented in the module documentation
plugins/modules/source_control/bitbucket/bitbucket_pipeline_key_pair.py:0:0: parameter-type-not-in-doc: Argument 'password' in argument_spec defines type as 'str' but documentation doesn't define type
plugins/modules/source_control/bitbucket/bitbucket_pipeline_key_pair.py:0:0: undocumented-parameter: Argument 'password' is listed in the argument_spec, but not documented in the module documentation
plugins/modules/source_control/bitbucket/bitbucket_pipeline_known_host.py:0:0: parameter-type-not-in-doc: Argument 'password' in argument_spec defines type as 'str' but documentation doesn't define type
plugins/modules/source_control/bitbucket/bitbucket_pipeline_known_host.py:0:0: undocumented-parameter: Argument 'password' is listed in the argument_spec, but not documented in the module documentation
plugins/modules/source_control/bitbucket/bitbucket_pipeline_variable.py:0:0: parameter-type-not-in-doc: Argument 'password' in argument_spec defines type as 'str' but documentation doesn't define type
plugins/modules/source_control/bitbucket/bitbucket_pipeline_variable.py:0:0: undocumented-parameter: Argument 'password' is listed in the argument_spec, but not documented in the module documentation

The test ansible-test sanity --test validate-modules [explain] failed with 8 errors:

plugins/modules/source_control/bitbucket/bitbucket_access_key.py:0:0: parameter-type-not-in-doc: Argument 'password' in argument_spec defines type as 'str' but documentation doesn't define type
plugins/modules/source_control/bitbucket/bitbucket_access_key.py:0:0: undocumented-parameter: Argument 'password' is listed in the argument_spec, but not documented in the module documentation
plugins/modules/source_control/bitbucket/bitbucket_pipeline_key_pair.py:0:0: parameter-type-not-in-doc: Argument 'password' in argument_spec defines type as 'str' but documentation doesn't define type
plugins/modules/source_control/bitbucket/bitbucket_pipeline_key_pair.py:0:0: undocumented-parameter: Argument 'password' is listed in the argument_spec, but not documented in the module documentation
plugins/modules/source_control/bitbucket/bitbucket_pipeline_known_host.py:0:0: parameter-type-not-in-doc: Argument 'password' in argument_spec defines type as 'str' but documentation doesn't define type
plugins/modules/source_control/bitbucket/bitbucket_pipeline_known_host.py:0:0: undocumented-parameter: Argument 'password' is listed in the argument_spec, but not documented in the module documentation
plugins/modules/source_control/bitbucket/bitbucket_pipeline_variable.py:0:0: parameter-type-not-in-doc: Argument 'password' in argument_spec defines type as 'str' but documentation doesn't define type
plugins/modules/source_control/bitbucket/bitbucket_pipeline_variable.py:0:0: undocumented-parameter: Argument 'password' is listed in the argument_spec, but not documented in the module documentation

click here for bot help

@ansibullbot ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed community_review labels Apr 12, 2021
@russoz
Copy link
Collaborator

russoz commented Apr 19, 2021

Hi @maxbrunet if you could prepend [WIP] to this PR's title, the bot will add the WIP label, making it easier for us to spot it ;-)

(Also feel free to ask for help - here or in the IRC channel)

@felixfontein felixfontein changed the title bitbucket: Support Basic Auth [WIP] bitbucket: Support Basic Auth Apr 19, 2021
@ansibullbot ansibullbot added the WIP Work in progress label Apr 19, 2021
@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label May 1, 2021
@ansibullbot ansibullbot removed the stale_ci CI is older than 7 days, rerun before merging label Jul 8, 2021
@felixfontein
Copy link
Collaborator

@maxbrunet do you still plan to work on this in the near future?

needs_info

@ansibullbot ansibullbot added the needs_info This issue requires further information. Please answer any outstanding questions label Sep 13, 2021
@maxbrunet
Copy link
Contributor Author

Not sure, maybe if I have time, but not really my priority, sorry

@ansibullbot ansibullbot removed the needs_info This issue requires further information. Please answer any outstanding questions label Sep 13, 2021
maxbrunet and others added 6 commits October 30, 2021 11:10
…own_host.py

Co-authored-by: Felix Fontein <felix@fontein.de>
…own_host.py

Co-authored-by: Felix Fontein <felix@fontein.de>
…riable.py

Co-authored-by: Felix Fontein <felix@fontein.de>
…riable.py

Co-authored-by: Felix Fontein <felix@fontein.de>
Co-authored-by: Felix Fontein <felix@fontein.de>
…y_pair.py

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 needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI labels Oct 30, 2021
@maxbrunet
Copy link
Contributor Author

Thank you for the review and suggestions @felixfontein

Since you don't allow maintainer edits

It must be because it is from a fork in my company's org, I do not have control over the setting, I will open PRs from my account next time.

I have documented the precedence between OAuth and Basic Auth credentials, it is usually how multiple credential sources are handled, but I considered making them mutually exclusive, I leave the patch here in case I have a change of mind.

mutually_exclusive.diff
diff --git a/plugins/module_utils/source_control/bitbucket.py b/plugins/module_utils/source_control/bitbucket.py
index bb50626a..4fa9ebc6 100644
--- a/plugins/module_utils/source_control/bitbucket.py
+++ b/plugins/module_utils/source_control/bitbucket.py
@@ -31,6 +31,10 @@ class BitbucketHelper:
             password=dict(type='str', no_log=True, fallback=(env_fallback, ['BITBUCKET_PASSWORD'])),
         )
 
+    @staticmethod
+    def bitbucket_mutually_exclusive():
+        return [[oauth, basic] for oauth in ["client_id", "client_secret"] for basic in ["user", "password"]]
+
     @staticmethod
     def bitbucket_required_one_of():
         return [['client_id', 'client_secret', 'user', 'password']]
diff --git a/plugins/modules/source_control/bitbucket/bitbucket_access_key.py b/plugins/modules/source_control/bitbucket/bitbucket_access_key.py
index 6451d729..94280e51 100644
--- a/plugins/modules/source_control/bitbucket/bitbucket_access_key.py
+++ b/plugins/modules/source_control/bitbucket/bitbucket_access_key.py
@@ -227,6 +227,7 @@ def main():
     module = AnsibleModule(
         argument_spec=argument_spec,
         supports_check_mode=True,
+        mutually_exclusive=BitbucketHelper.bitbucket_mutually_exclusive(),
         required_one_of=BitbucketHelper.bitbucket_required_one_of(),
         required_together=BitbucketHelper.bitbucket_required_together(),
     )
diff --git a/plugins/modules/source_control/bitbucket/bitbucket_pipeline_key_pair.py b/plugins/modules/source_control/bitbucket/bitbucket_pipeline_key_pair.py
index 5d42419d..9d1703c7 100644
--- a/plugins/modules/source_control/bitbucket/bitbucket_pipeline_key_pair.py
+++ b/plugins/modules/source_control/bitbucket/bitbucket_pipeline_key_pair.py
@@ -164,6 +164,7 @@ def main():
     module = AnsibleModule(
         argument_spec=argument_spec,
         supports_check_mode=True,
+        mutually_exclusive=BitbucketHelper.bitbucket_mutually_exclusive(),
         required_one_of=BitbucketHelper.bitbucket_required_one_of(),
         required_together=BitbucketHelper.bitbucket_required_together(),
     )
diff --git a/plugins/modules/source_control/bitbucket/bitbucket_pipeline_known_host.py b/plugins/modules/source_control/bitbucket/bitbucket_pipeline_known_host.py
index 9f4f2b94..2c93feea 100644
--- a/plugins/modules/source_control/bitbucket/bitbucket_pipeline_known_host.py
+++ b/plugins/modules/source_control/bitbucket/bitbucket_pipeline_known_host.py
@@ -265,6 +265,7 @@ def main():
     module = AnsibleModule(
         argument_spec=argument_spec,
         supports_check_mode=True,
+        mutually_exclusive=BitbucketHelper.bitbucket_mutually_exclusive(),
         required_one_of=BitbucketHelper.bitbucket_required_one_of(),
         required_together=BitbucketHelper.bitbucket_required_together(),
     )
diff --git a/plugins/modules/source_control/bitbucket/bitbucket_pipeline_variable.py b/plugins/modules/source_control/bitbucket/bitbucket_pipeline_variable.py
index e5701184..b9458291 100644
--- a/plugins/modules/source_control/bitbucket/bitbucket_pipeline_variable.py
+++ b/plugins/modules/source_control/bitbucket/bitbucket_pipeline_variable.py
@@ -226,6 +226,7 @@ def main():
     module = BitBucketPipelineVariable(
         argument_spec=argument_spec,
         supports_check_mode=True,
+        mutually_exclusive=BitbucketHelper.bitbucket_mutually_exclusive(),
         required_one_of=BitbucketHelper.bitbucket_required_one_of(),
         required_together=BitbucketHelper.bitbucket_required_together(),
     )

@ansibullbot ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Oct 30, 2021
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.

Besides the two version_addeds below (sorry I missed them before), this needs a changelog fragment:

  1. A minor_changes section for the new options (user, password);
  2. A deprecated_features section for that username has been renamed to workspace and the old name username (now an alias) will be removed in community.general 6.0.0;
  3. A major_changes entry that client_id is no longer marked as no_log=True. (major_changes so that this gets included in the porting guide; if anyone things their client_id is sensitive they need to take precautions to keep it out of logs - if it's announced that publicly nobody can complain :) ).

plugins/doc_fragments/bitbucket.py Show resolved Hide resolved
plugins/doc_fragments/bitbucket.py Show resolved Hide resolved
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.

Some wording and formatting issues in the changelog:

Co-authored-by: Felix Fontein <felix@fontein.de>
Co-authored-by: Felix Fontein <felix@fontein.de>
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.

If CI doesn't come up with a surprise, I'll merge this :)

@ansibullbot ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI labels Oct 31, 2021
@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Oct 31, 2021
@ansibullbot ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Oct 31, 2021
@felixfontein felixfontein merged commit aaa0f39 into ansible-collections:main Oct 31, 2021
@felixfontein
Copy link
Collaborator

@maxbrunet thanks a lot for contributing this!

JonEllis pushed a commit to JonEllis/community.general that referenced this pull request Nov 16, 2021
* bitbucket: Support Basic Auth

* Rename username to user

* Document user/password options

* Rename username to workspace

* Deprecate username

* Fix credentials_required error_message

* Fix credentials_required error_message

* Test user/password/workspace options and env vars

* Update a test to use user/password/workspace for each module

* Fix check auth arguments

* Use required_one_of/required_together

* Fix required typo

* Fix fetch_access_token

* Fix tests 🤞

* Switch things up in test_bitbucket_access_key

* Fix username/password are None

* Remove username/password properties, use params directly

* Update plugins/doc_fragments/bitbucket.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update plugins/module_utils/source_control/bitbucket.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update plugins/module_utils/source_control/bitbucket.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update plugins/module_utils/source_control/bitbucket.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update plugins/modules/source_control/bitbucket/bitbucket_access_key.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update plugins/modules/source_control/bitbucket/bitbucket_pipeline_key_pair.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update plugins/modules/source_control/bitbucket/bitbucket_pipeline_known_host.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update plugins/modules/source_control/bitbucket/bitbucket_pipeline_known_host.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update plugins/modules/source_control/bitbucket/bitbucket_pipeline_variable.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update plugins/modules/source_control/bitbucket/bitbucket_pipeline_variable.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update plugins/modules/source_control/bitbucket/bitbucket_access_key.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update plugins/modules/source_control/bitbucket/bitbucket_pipeline_key_pair.py

Co-authored-by: Felix Fontein <felix@fontein.de>

* Document OAuth/Basic Auth precedence

* Apply suggestions from code review

Co-authored-by: Felix Fontein <felix@fontein.de>

* Remove no_log=False from user argument

* Add changelog fragment

* Correct wording and formatting in changelog

Co-authored-by: Felix Fontein <felix@fontein.de>

* Update changelogs/fragments/2045-bitbucket_support_basic_auth.yaml

Co-authored-by: Felix Fontein <felix@fontein.de>

Co-authored-by: Felix Fontein <felix@fontein.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs_fragments docs_fragments plugin (shared docs) feature This issue/PR relates to a feature request module_utils module_utils module module new_contributor Help guide this first time contributor plugins plugin (any type) source_control tests tests unit tests/unit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bitbucket: Support Basic Auth
4 participants