-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
feat(azure): support Azure DevOps Server authentication methods #5602
Conversation
lib/platform/azure/azure-helper.ts
Outdated
break; | ||
case 'personalAccessToken': | ||
default: | ||
header = `${headerName}: basic ${Buffer.from(`:${config.token}`).toString( |
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 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
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.
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?
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.
pending discussion, I've committed an update here, if this is preferred I can push that to this PR
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.
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>
.
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.
done
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.
About token:<PAT>
: I tested it with :<PAT>
and it worked on my side
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 testing.
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. |
@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 |
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.
@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
@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 😄 |
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
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).