-
Notifications
You must be signed in to change notification settings - Fork 66
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
add support for bitbucket workspace token authentication #508
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. I added some comments about the authtype
flag.
Just to confirm. Are you a part of the Atlassian team?
@gcase555
cmd/platform.go
Outdated
@@ -23,7 +23,8 @@ func configurePlatform(cmd *cobra.Command) { | |||
flags.StringP("base-url", "g", "", "Base URL of the target platform, needs to be changed for GitHub enterprise, a self-hosted GitLab instance, Gitea or BitBucket.") | |||
flags.BoolP("insecure", "", false, "Insecure controls whether a client verifies the server certificate chain and host name. Used only for Bitbucket server.") | |||
flags.StringP("username", "u", "", "The Bitbucket server username.") | |||
flags.StringP("token", "T", "", "The personal access token for the targeting platform. Can also be set using the GITHUB_TOKEN/GITLAB_TOKEN/GITEA_TOKEN/BITBUCKET_SERVER_TOKEN/BITBUCKET_CLOUD_APP_PASSWORD environment variable.") | |||
flags.StringP("token", "T", "", "The personal access token for the targeting platform. Can also be set using the GITHUB_TOKEN/GITLAB_TOKEN/GITEA_TOKEN/BITBUCKET_SERVER_TOKEN/BITBUCKET_CLOUD_APP_PASSWORD/BITBUCKET_CLOUD_WORKSPACE_TOKEN environment variable.") | |||
flags.StringP("authtype", "", "", "The authentication type. Either app-password or workspace-token. Used only for Bitbucket cloud.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be defined as constant values and should error (or succeed) in the parsing. There is also autocompletion that could be done. Please see the conflict-strategy
option for reference and feel free to ask if there is any uncertainties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having no default option here makes this not backwards compatible for no good reason.
I see three options here:
- Set the default value to
app-password
to keep backwards compatability - Set the default value to
workspace-token
if this is the preferred method of authentication. This would break backwards compatibility, but because of a good reason which could be ok because of the state of the BBC implementation. - The most extreme approach would be to remove password authentication altogether. But if there is no real good reason for people to use it, (meaning they could always use tokens instead,) I would be in favor of this option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also minor comment on the actual name. Should probably be auth-type
to match with other names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the comments! And yes I am part of the Atlassian team.
I updated the auth-type
flag and tried to copy the conflict-strategy
pattern. Please let me know if I should structure anything differently.
For the default option, I decided to use app-password
since workspace tokens are a premium feature, so many users will still need to use app password.
Yes, were the contributors that created the original bitbucket cloud integration and have resumed progress adding needed features(like workspace token based auth) and I am currently working on improving the performance with large workspaces that will be handled in a separate PR. I tested out this worked with a workspace token when creating PRs for bitbucket cloud. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a very minor change for the flag description and this LGTM!
cmd/platform.go
Outdated
Available values: | ||
app-password: authenticate using an app password | ||
workspace-token: authenticate using a workspace token |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the descriptions are not providing any additional value, please just have then listed on the same line.
What does this change
Adds support for workspace token authentication in Bitbucket Cloud.
What issue does it fix
This change is needed because it provides more flexibility for authentication type (currently Bitbucket Cloud only supports app password authentication), and workspace token authentication is generally more secure.
Notes for the reviewer
Checklist