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

feat(azure): support Azure DevOps Server authentication methods #5602

Merged
merged 3 commits into from Mar 24, 2020
Merged

feat(azure): support Azure DevOps Server authentication methods #5602

merged 3 commits into from Mar 24, 2020

Conversation

kroonprins
Copy link
Contributor

@kroonprins kroonprins commented Feb 29, 2020

I am using "Azure DevOps Server" (former TFS) instead of "Azure DevOps Services", and one without "personal access token" authentication enabled. The only way I am able to authenticate calls to the Azure DevOps Server REST API and git repositories is either via basic username/password (for local development) or via bearer token (for Azure DevOps pipelines). Currently renovate only supports personal access token authentication, which is the most common use case and Azure DevOps preferred way, so this is good. But to also support other authentication types I propose the change of this PR.

I've tested the changes for

  • for Azure DevOps Server with username/password and bearer authentication
  • for Azure DevOps Services with personal access tokens (=> so there should be no impact for existing users)

I have not yet looked into unit tests (draft pull request) as I would first like to get feedback if this is a change that might be considered for merging.

The code uses "http.extraheader" configuration for git clones as it seems the only way that git clones work consistently for Azure DevOps Server and Azure DevOps Services (microsoft/azure-pipelines-agent#1601, https://docs.microsoft.com/en-us/azure/devops/pipelines/repos/pipeline-options-for-git?view=azure-devops).

@JamieMagee JamieMagee self-requested a review February 29, 2020 16:35
break;
case 'personalAccessToken':
default:
header = `${headerName}: basic ${Buffer.from(`:${config.token}`).toString(
Copy link
Member

Choose a reason for hiding this comment

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

This can be unified with username / password, as this is the same as using an empty username and the token as password.

So we only have username / password and token auth as before and don't need authenticationType

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean, but:

  • That would make it a breaking change. For example the azure extension would need to be changed. Maybe an option is to use the same logic as getHandlerFromToken instead? Checking on token length seems a bit dirty, but well if the azure api does it internally, I guess it can also be used here.
  • Azure has another auth type here, which I didn't include because I am not familiar with it and had no idea how to test. Maybe if in the future someone is interested in it, it could be easier to add if authenticationType is there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pending discussion, I've committed an update here, if this is preferred I can push that to this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you pull that commit into this PR? I think it simplifies this by not requiring an extra config option. One thing though: I think it might be required to have a non-empty username when using a PAT. Currently we use token:<PAT>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

About token:<PAT>: I tested it with :<PAT> and it worked on my side

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for testing.

lib/platform/azure/index.ts Outdated Show resolved Hide resolved
lib/platform/azure/index.ts Outdated Show resolved Hide resolved
lib/platform/azure/index.ts Outdated Show resolved Hide resolved
lib/platform/azure/index.ts Outdated Show resolved Hide resolved
lib/platform/azure/index.ts Outdated Show resolved Hide resolved
@JamieMagee
Copy link
Contributor

Just a question about this in general: What's preventing the use of PATs in Azure DevOps Server 2019? I admit I only use Azure DevOps Service day-to-day, but the documentation seems to indicate that PATs are supported all the way back to TFS 2017

@kroonprins
Copy link
Contributor Author

Just a question about this in general: What's preventing the use of PATs in Azure DevOps Server 2019? I admit I only use Azure DevOps Service day-to-day, but the documentation seems to indicate that PATs are supported all the way back to TFS 2017

That is a very valid question :). When Azure DevOps Server was set up in my company, the choice had been made to use basic authentication over https and IP filtering, for all of the development tools (Azure DevOps, artifactory, Sonar, ..). Because of this PATs were not considered at the time and have not gone through a security review here and because of that never allowed.

@JamieMagee
Copy link
Contributor

@kroonprins I'm interested in getting this PR and #5601 merged. Can you resolve your PRs with the latest changes from master?

@kroonprins kroonprins marked this pull request as ready for review March 23, 2020 22:29
@kroonprins
Copy link
Contributor Author

@kroonprins I'm interested in getting this PR and #5601 merged. Can you resolve your PRs with the latest changes from master?

@JamieMagee, both PRs should be up to date now

@rarkins rarkins requested a review from JamieMagee March 24, 2020 06:15
lib/platform/azure/index.ts Show resolved Hide resolved
lib/platform/azure/index.ts Show resolved Hide resolved
lib/platform/azure/index.ts Show resolved Hide resolved
lib/platform/azure/index.ts Show resolved Hide resolved
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

@JamieMagee @rarkins I think we should refactor the platform code, so that all platforms use the clone header approach. So we can (hopefully) simplify / unify the git initialization

@JamieMagee
Copy link
Contributor

JamieMagee commented Mar 24, 2020

@kroonprins Thank you! I'll update both the PRs with current master, and merge shortly.

It's at times like this when I wish GitHub had "Set auto-complete" like Azure DevOps 😄

@JamieMagee JamieMagee merged commit 63b5094 into renovatebot:master Mar 24, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants