-
Notifications
You must be signed in to change notification settings - Fork 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
{Keyvault} Fix #22540: Create keyvault with service principal #22543
Conversation
current_user = graph_client.signed_in_user_get() | ||
if current_user and current_user.get('id', None): # pylint:disable=no-member | ||
return current_user['id'] # pylint:disable=no-member |
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 did leave a comment for this weird behavior of checking the existence of id
(#22203 (comment)).
graph_client.signed_in_user_get
will ALWAYS return a user object which MUST have id
property UNLESS GraphError
is raised.
Takeaways:
- Pylint error is a sign of abnormality and should NOT be disabled brutally.
- For anything unusual like
current_user.get('id', None)
orexcept CloudError
, comments MUST be left to explain its behavior. Otherwise, it should not be there.
except GraphError: | ||
pass |
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.
We got tripped by some code written almost 6 years ago! (cb0faa8)
Good catch ! |
I have confirmed the new RPM has the correct code:
|
Related command
az keyvault create
Description
#22203 changed
msrestazure.azure_exceptions.CloudError
toazure.cli.command_modules.role.GraphError
inazure.cli.command_modules.keyvault.custom._get_current_user_object_id
.Some background:
azure-graphrbac
Python SDK raises inconsistent exceptions for Graph errors:DomainsOperations.get
raisesCloudError
:https://github.com/Azure/azure-sdk-for-python/blob/dfa109c36598d93aaf74238feded516352c20f4c/sdk/graphrbac/azure-graphrbac/azure/graphrbac/operations/domains_operations.py#L150-L153
SignedInUserOperations.get
raisesGraphErrorException
:https://github.com/Azure/azure-sdk-for-python/blob/dfa109c36598d93aaf74238feded516352c20f4c/sdk/graphrbac/azure-graphrbac/azure/graphrbac/operations/signed_in_user_operations.py#L79-L80
Since
SignedInUserOperations.get
doesn't raiseCloudError
, thisexcept CloudError
will never be hit:azure-cli/src/azure-cli/azure/cli/command_modules/keyvault/custom.py
Lines 312 to 317 in 1451bd5
Therefore, this
except CloudError
clause has no effect and should NOT be replaced byGraphError
. Doing so will makeGraphError
be intercepted and the outerexcept GraphError
have no effect:azure-cli/src/azure-cli/azure/cli/command_modules/keyvault/custom.py
Lines 747 to 750 in cac3f06
This PR lets
_get_current_user_object_id
raiseGraphError
so that it can be caught by outerexcept GraphError
.Testing Guide
az ad sp create-for-rbac --role Owner --scopes /subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590 --verbose # Copy the login command from log and run: az login --service-principal --username 558c5061-b77b-4dfa-8118-168068c9be4a --password xxx --tenant 54826b22-38d6-4fb2-bad9-b7b93a3e9c5a az group create -n my-kv-testrg -l eastasia az keyvault create -n my-kv-test -g my-kv-testrg