Skip to content

[Role] az ad app permission list/grant: Refine error message when no associated Service Principal exists for the App #17051

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.

Sign up for GitHub

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

Merged
jiasli merged 1 commit into from
Mar 19, 2021
Merged

[Role] az ad app permission list/grant: Refine error message when no associated Service Principal exists for the App #17051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions src/azure-cli/azure/cli/command_modules/role/custom.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -853,13 +853,17 @@ def create_application(cmd, display_name, homepage=None, identifier_uris=None,

def _get_grant_permissions(graph_client, client_sp_object_id=None, query_filter=None):
query_filter = query_filter or ("clientId eq '{}'".format(client_sp_object_id) if client_sp_object_id else None)
grant_info = graph_client.oauth2_permission_grant.list(filter=query_filter)
try:
grant_info = graph_client.oauth2_permission_grant.list(filter=query_filter)
except CloudError as ex: # Graph doesn't follow the ARM error; otherwise would be caught by msrest-azure
Comment on lines -857 to -858
Copy link
Member Author

@jiasli jiasli Feb 23, 2021

Choose a reason for hiding this comment

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

list returns a Paged object which calls REST API lazily, so this except CloudError will never be hit.

# Make the REST request immediately so that errors can be raised and handled.
return list(grant_info)
except CloudError as ex:
if ex.status_code == 404:
return []
raise CLIError("Service principal with appId or objectId '{id}' doesn't exist. "
"If '{id}' is an appId, make sure an associated service principal is created "
"for the app. To create one, run `az ad sp create --id {id}`."
.format(id=client_sp_object_id))
Comment on lines +862 to +865
Copy link
Member

@fengzhou-msft fengzhou-msft Mar 18, 2021

Choose a reason for hiding this comment

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

Shall we use a UserFault type error here?

Copy link
Member

@evelyn-ys evelyn-ys Mar 18, 2021

Choose a reason for hiding this comment

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

InvalidArgumentValueError for example

Copy link
Member Author

@jiasli jiasli Mar 18, 2021

Choose a reason for hiding this comment

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

This is rather complex here as client_sp_object_id is indeed a valid appId but the app doesn't have associated SP. This command group is already considered deprecated (implicitly) anyway.

raise
return grant_info


def list_permissions(cmd, identifier):
Expand All @@ -870,13 +874,14 @@ def list_permissions(cmd, identifier):

# first get the permission grant history
client_sp_object_id = _resolve_service_principal(graph_client.service_principals, identifier)
grant_permissions = _get_grant_permissions(graph_client, client_sp_object_id=client_sp_object_id)

# get original permissions required by the application, we will cross check the history
# and mark out granted ones
graph_client = _graph_client_factory(cmd.cli_ctx)
application = show_application(graph_client.applications, identifier)
permissions = application.required_resource_access
if permissions:
grant_permissions = _get_grant_permissions(graph_client, client_sp_object_id=client_sp_object_id)
for p in permissions:
result = list(graph_client.service_principals.list(
filter="servicePrincipalNames/any(c:c eq '{}')".format(p.resource_app_id)))
Expand Down
Loading