Skip to content

{Role} Raise GraphError only when Graph request fails#22523

Merged
jiasli merged 1 commit intoAzure:devfrom
jiasli:graph-error
May 19, 2022
Merged

{Role} Raise GraphError only when Graph request fails#22523
jiasli merged 1 commit intoAzure:devfrom
jiasli:graph-error

Conversation

@jiasli
Copy link
Copy Markdown
Member

@jiasli jiasli commented May 19, 2022

Related command
az ad

Description

Currently, all subclasses of CLIError are re-raised as GraphError. This is not correct and causes failure for AuthenticationError handling.

For example, we change microsoft_graph_resource_id to an invalid value like https://graph1.microsoft.com/, which is then used as a scope to get an access token:

azure-cli/src/azure-cli-core/azure/cli/core/cloud.py

Line 308 in 31e396b

microsoft_graph_resource_id='https://graph.microsoft.com/',

The error handling procedure failed:

 > az ad app show --id 0e33fcf1-8a78-4941-9479-75d29681f4dc
...
    result = self._send("GET", "/applications" + _filter_to_query(filter))
  File "d:\cli\azure-cli\src\azure-cli\azure\cli\command_modules\role\msgrpah\_graph_client.py", line 40, in _send
    raise GraphError(ex.response.json()['error']['message'], ex.response) from ex
AttributeError: 'AuthenticationError' object has no attribute 'response'

This PR makes send_raw_request raise a dedicated error HTTPError and it is then re-raised as GraphError. Since HTTPError is a sub-class of CLIError, it is backward compatible.

Now invalid scope will result in

> az ad app show --id 0e33fcf1-8a78-4941-9479-75d29681f4dc
This command or command group has been migrated to Microsoft Graph API. Please carefully review all breaking changes introduced during this migration: https://docs.microsoft.com/cli/azure/microsoft-graph-migration
AADSTS500011: The resource principal named https://graph1.microsoft.com/ was not found in the tenant named AzureSDKTeam. This can happen if the application has not been installed by the administrator of the tenant or consented to by any user in the tenant. You might have sent your authentication request to the wrong tenant.
Trace ID: 4afc0638-c613-4eba-8556-0031e7c8a500
Correlation ID: 17b08664-450c-41fa-a69b-09002c9d5c4d
Timestamp: 2022-05-19 06:52:07Z
To re-authenticate, please run:
az login --scope https://graph1.microsoft.com//.default

@ghost ghost requested a review from yonzhan May 19, 2022 07:36
@ghost ghost added the Auto-Assign Auto assign by bot label May 19, 2022
@ghost ghost assigned jiasli May 19, 2022
@ghost ghost added this to the May 2022 (2022-05-24) - For Build milestone May 19, 2022
@ghost ghost added the RBAC az role label May 19, 2022
@ghost ghost requested a review from wangzelin007 May 19, 2022 07:36
@ghost ghost added the Graph (doesn't work with label-triggered comments; use Graph.Microsoft instead) az ad label May 19, 2022
jiasli
jiasli commented May 19, 2022
View reviewed changes
src/azure-cli-core/azure/cli/core/azclierror.py Outdated
Copy link
Copy Markdown
Member Author

@jiasli jiasli May 19, 2022

Choose a reason for hiding this comment

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

Alternative names can be RequestError or RawRequestError. I chose this name similar to requests.exceptions.HTTPError.

@yonzhan
Copy link
Copy Markdown
Collaborator

yonzhan commented May 19, 2022

Role

@jiasli jiasli changed the title {Role} Only raise GraphError when Graph request fails {Role} Raise GraphError only when Graph request fails May 19, 2022
@jiasli jiasli merged commit 3dbcafb into Azure:dev May 19, 2022
@jiasli jiasli deleted the graph-error branch May 19, 2022 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@evelyn-ys evelyn-ys evelyn-ys approved these changes

@calvinhzy calvinhzy calvinhzy approved these changes

@yonzhan yonzhan yonzhan approved these changes

@jsntcy jsntcy Awaiting requested review from jsntcy

@kairu-ms kairu-ms Awaiting requested review from kairu-ms

@zhoxing-ms zhoxing-ms Awaiting requested review from zhoxing-ms

@wangzelin007 wangzelin007 Awaiting requested review from wangzelin007

Assignees

@jiasli jiasli

Labels

Auto-Assign Auto assign by bot Graph (doesn't work with label-triggered comments; use Graph.Microsoft instead) az ad RBAC az role

Projects

None yet

Milestone

May 2022 (2022-05-24) - For Build

Development

Successfully merging this pull request may close these issues.

4 participants

@jiasli @yonzhan @evelyn-ys @calvinhzy