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

Fix broken az devops auth #1226

Merged
merged 4 commits into from
Nov 8, 2021
Merged

Fix broken az devops auth #1226

merged 4 commits into from
Nov 8, 2021

Conversation

roshan-sy
Copy link
Contributor

@roshan-sy roshan-sy commented Nov 8, 2021

Please make sure the code is following contribution guidelines in CONTRIBUTING.md

  • : This PR has a corresponding issue open in the Repository.
  • : Approach is signed off on the issue.

@roshan-sy roshan-sy marked this pull request as ready for review November 8, 2021 07:00
@gauravsaralMs
Copy link
Contributor

@roshan-sy

@@ -152,9 +152,12 @@ def get_token_from_az_logins(organization, pat_token_present):
return ''


def get_token_from_az_login(profile, user, tenant):
def get_token_from_az_login(profile, tenant):
Copy link
Member

@jiasli jiasli Nov 9, 2021

Choose a reason for hiding this comment

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

get_raw_token supports user, service principal, managed identity accounts.

By removing user from get_token_from_az_login, get_raw_token will only return token for the currently selected/default account:

1. sub1, user1, from tenant1 - default
2. sub2, user2, from tenant2

This will cause get_raw_token to get a token for user1 in tenant2.

Consider using subscription as the key for get_raw_token.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tokens are independent of subscriptions. as ADO accounts are not linked to any azure subscriptions.

Copy link
Member

@jiasli jiasli Nov 9, 2021

Choose a reason for hiding this comment

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

I understand, but Azure CLI uses subscription as the primary key. If you don't specify subscription as the key for get_raw_token, unmatched user and tenant may occur.

Here you use subscription only as a placeholder for a user+tenant combination, instead of using the subscription.

Copy link
Member

@jiasli jiasli Nov 9, 2021

Choose a reason for hiding this comment

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

The old implementation of get_access_token_for_resource doesn't work for service principal and managed identity, and that's why it is not recommended and removed in the MSAL migration:

https://github.com/Azure/azure-cli/blob/14cc787d0f58bc649d402b486fdecc5625eee9ac/src/azure-cli-core/azure/cli/core/_profile.py#L531-L535

    def get_access_token_for_resource(self, username, tenant, resource):
        tenant = tenant or 'common'
        _, access_token, _ = self._creds_cache.retrieve_token_for_user(
            username, tenant, resource)
        return access_token

Copy link
Member

Choose a reason for hiding this comment

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

User/tenant isolation was proposed in Azure/azure-cli#15005. After that, it will make sense to make get_raw_token support user+tenant.

@jiasli
Copy link
Member

jiasli commented Nov 9, 2021

I don't think it is a good idea to try every Azure CLI account, as this may cause unexpected user credential to be used.

Instead, only the default/selected (subscription, tenant, user) combination should be used to get an access token. If that token doesn't have access, we should explicitly switch account with az account set.

@cveld
Copy link

cveld commented Dec 14, 2021

Maybe good next time to have a good PR description. Now it is not clear to me:

  • What was the original problem
  • What is the proposed solution
  • What are the test cases that are run

In my case I am still not able to use AAD authentication against Azure DevOps. Using az cli 2.30.0 and devops extension 0.22. Also see Azure/azure-cli#20529

And ensure that an issue is linked.


I just processed the instructions (https://github.com/Azure/azure-devops-cli-extension/blob/master/doc/dev_setup.md) to start developing the extension locally. And this installed version 0.23 which seems to work 🥳

@jiasli
Copy link
Member

jiasli commented Dec 21, 2021

Maybe good next time to have a good PR description.

Completely agree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants