Azure / Azure/azure-devops-cli-extension
[Feature Request] Rewrite AAD token authentication
- Dominant language
- Python
- Stars
- 682
- Forks
- 278
- Avg merge
- 3d 23h
- Merged PRs (30d)
- 3
Description
**Is your feature request related to a problem? Please describe.**
AAD token authentication is really flaky, hides error messages and has terrible UX
Splitted from issue #1258
**Describe the solution you'd like**
Rewrite most of the related code
# Background Story
## Current status
1. Extension uses subscription information from AZ CLI(1) `subscriptions = profile.load_cached_subscriptions(False)` loops through the information
2. tenant of the active subscription is added as first item if tenantsDict (2)
3. other tenant information from the subscription list is being collected into tenantsDict
4. the code goes through collected tenants from tenantsDict
- token for tenant is being fetch from AZ CLI: `token = get_token_from_az_login(profile, key[0])`
1. problems on this step described later
- returned token is validated with ADO endpoint: `validate_token_for_instance(organization, credentials)`
1. if validation passed then correct token/tenant was found and and the token can be used
2. if validation failed than token us not usable
3. if suitable token not found then empty token is returned from `get_token_from_az_logins()` and `_get_credentials()` will raise exception
4. issues with `validate_token_for_instance()` is described later
## get_token_from_az_login() problems
https://github.com/Azure/azure-devops-cli-extension/blob/b3d0392d597a2eae5229e96059359d00fbb2e222/azure-devops/azext_devops/dev/common/services.py#L155-L165
* Exceptions raised in `profile.get_raw_token()` are masked by logger.debug()
* outdated refresh tokens
* outdated MFA token
* other problems
## validate_token_for_instance() problems
https://github.com/Azure/azure-devops-cli-extension/blob/b3d0392d597a2eae5229e96059359d00fbb2e222/azure-devops/azext_devops/dev/common/services.py#L86-L98
* Error messages from ADO API are masked and dislayed only with debug logging
* Minor issue
## get_token_from_az_logins() problem
https://github.com/Azure/azure-devops-cli-extension/blob/b3d0392d597a2eae5229e96059359d00fbb2e222/azure-devops/azext_devops/dev/common/services.py#L116-L152
* Errors are again hidden to debug level to decrease amount of console spam
# Issues
* login token problems are hidden and only visible with `--debug`
* probably done to decrease amount of console spam but also errors with the correct tenant token is hidden
* goes through all tenants available on AZ CLI until working tenant is found
* adds extra delays if user has multiple tenants
* recent commit https://github.com/Azure/azure-devops-cli-extension/pull/1286 partialy solves this problem
* Current error message is not really helpful for end-users
* should mention `--tenant`, `--allow-no-subscriptions` and maybe even give hint about active subscription selection
# Proposed fixes
## Add `--tenant` parameter
If user can applies tenant id then
https://github.com/Azure/azure-devops-cli-extension/blob/b3d0392d597a2eae5229e96059359d00fbb2e222/azure-devops/azext_devops/dev/common/services.py#L123-L128
logic of that code could be simplified and looping all available tenants is not needed
## Request tenant id from Azure Devops service
**ADO API returns tenant id with 403 replies**
The following headers are returned from ADO:
* www-authenticate: Bearer authorization_uri=https://login.microsoftonline.com/0a8edbb9-xxxx-xxxx-xxxx-2cd241f8236f
* www-authenticate: Basic realm="https://tfsprodweu5.visualstudio.com/"
* x-vss-resourcetenant: 0a8edbb9-667f-4120-88fd-2cd241f8236f
Looping through all tenant information is not needed if tenant information is fetch from the ADO API.
## Removal of token validation
* will be result of other changes?
## Removal of the tenant loops
* will be result of other changes?
--------
(1) https://github.com/Azure/azure-devops-cli-extension/blob/b3d0392d597a2eae5229e96059359d00fbb2e222/azure-devops/azext_devops/dev/common/services.py#L119
(2) this code was broken. Fix is in main but not released yet
--------
Current state:
```mermaid
sequenceDiagram
autonumber
participant User
participant Extension
participant ADO_API
participant CLI
User->>Extension: run command
Extension->>CLI: get subscriptions
CLI-->>Extension:
%%Extension->>Extension:
%%activate Extension
loop Validation: subscription/user pairs to find working token
Extension->>CLI: Get token for sub/user
CLI-->Extension:
Extension->>ADO_API: Try fetching data with token
alt failure
ADO_API-->>Extension: fail
Note over ADO_API,Extension: continue loop
else success
ADO_API-->>Extension: success
Note over ADO_API,Extension: exit loop
end
opt After loop exhaustion
Extension-->>User: Display AAD token error
end
opt After successfull token validation
Extension->>ADO_API: Fetch data
ADO_API-->>Extension:
Extension-->>User: Display for ADO API request
end
end
Note right of ADO_API: slow and unneeded loop
```
Future?:
```mermaid
sequenceDiagram
autonumber
participant User
participant Extension
participant ADO_API
participant CLI
User->>Extension: run command
Extension->>ADO_API: get data from protected enpoint
ADO_API-->>Extension:
Extension->>CLI: get subscriptions
CLI-->>Extension:
Extension->Extension: find correct subscription/username pair for given tenant id
Extension->>CLI: Get token for sub/user
CLI-->Extension:
Extension->Extension: handle profile.get_raw_token() errors
opt After getting token for given tenant
Note over ADO_API,Extension: validation not really needed
Extension->>ADO_API: Fetch data
ADO_API-->>Extension:
Extension-->>User: Display for ADO API request
end
opt Token not found or other profile.get_raw_token() exception
Extension-->>User: Display AAD token error
end
```
Contributor guide
Assessment
This issue has not been assessed yet.