Add new az ad app permissions grant and list commands for OAuth2 permissions for AAD registered apps #6975

shanepeckham · 2018-08-07T13:43:27Z

This is needed so that we can programmatically grant and list OAuth2 consent to an application in Azure Active Directory. This is the supported and correct way to do this respecting customer approval workflows. Example use case includes Azure Kubernetes Service RBAC integration with Azure Active Directory.

This PR has the following dependencies upstream that need to be merged first:

Added OAuth2 GET and POST to GraphRBAC.json spec azure-rest-api-specs#3563 - this is the extension to the REST spec which has auto-generated PRs for all of the Azure SDKs
[AutoPR graphrbac/data-plane] Added OAuth2 GET and POST to GraphRBAC.json spec azure-sdk-for-python#3063 - this is the autorest generation of the the azure-sdk-for-python

New command has the following help format:

helps['ad app permission grant'] = """
    type: command
    short-summary: Grant an app OAuth2 permissions for the respective app
    examples:
        - name: Grant a native application with OAuth2 permissions from an existing AAD app with TTL of 1 year
          text: az ad app permission grant --id e042ec79-34cd-498f-9d9f-1234234 --app-id a0322f79-57df-498f-9d9f-12678 --expires 1
"""

helps['ad app permission list'] = """
    type: command
    short-summary: List the app OAuth2 permissions 
    examples:
        - name: List the OAuth2 permissions for an existing AAD app 
          text: az ad app permission list --id e042ec79-34cd-498f-9d9f-1234234
"""

Usage:

az ad app permissions grant --id 14cb4751-8285-4c77-a694-4583a07cc1a7 --app-id ce848207-8603-4eb3-9b8e-59023d31f742 --expires never

az ad app permission list --id 14cb4751-8285-4c77-a694-4583a07cc1a8

This checklist is used to make sure that common guidelines for a pull request are followed.

[ X] I adhere to the Command Guidelines.

yugangw-msft

@shanepeckham, thanks for the contribution. Good improvement! I left a few comments, mostly to help get your change integrate well with the existing commands

yugangw-msft · 2018-08-07T18:34:45Z

src/command_modules/azure-cli-role/azure/cli/command_modules/role/_help.py

+    short-summary: Grant the app OAuth2 permissions for the respective apps
+    examples:
+        - name: update a native application with OAuth2 permissions with space separated clientIds for AAD apps with TTL of 1 year
+          text: az ad app update --id e042ec79-34cd-498f-9d9f-1234234 --appid a0322f79-57df-498f-9d9f-12678 --expires 1


I believe the command should be grant, instead of update

Changed to grant and reworked for new command ad app permission grant

yugangw-msft · 2018-08-07T18:36:39Z

src/command_modules/azure-cli-role/azure/cli/command_modules/role/commands.py

@@ -84,6 +84,7 @@ def load_command_table(self, _):
        g.custom_command('delete', 'delete_application')
        g.custom_command('list', 'list_apps')
        g.custom_show_command('show', 'show_application')
+        g.custom_command('grant', 'grant_application')


Does AAD graph support "permission revoke"? If yes, I suggest you author it like az ad app permission grant so we can add revoke and list later

Changed to g.custom_command('permission grant', 'grant_application')

Also added g.custom_command('permission list', 'list_granted_application')

yugangw-msft · 2018-08-07T18:37:34Z

src/command_modules/azure-cli-role/azure/cli/command_modules/role/custom.py

-            _get_role_property(a, 'scope').lower() == scope.lower()
+                not scope or
+                include_inherited and re.match(_get_role_property(a, 'scope'), scope, re.I) or
+                _get_role_property(a, 'scope').lower() == scope.lower()


I would suggest you back out the formatting change and only submit the command change

Fixed formatting - accidental change

yugangw-msft · 2018-08-07T18:38:23Z

src/command_modules/azure-cli-role/azure/cli/command_modules/role/custom.py

@@ -667,6 +668,62 @@ def create_application(client, display_name, homepage=None, identifier_uris=None
    return result


+def grant_application(cmd, identifier, appid, expires='1'):
+    if not appid:


unnecessary, for not optional argument, CLI command parser will ensure users supply the values.

Removed optional argument validation

yugangw-msft · 2018-08-07T18:44:37Z

src/command_modules/azure-cli-role/azure/cli/command_modules/role/custom.py

+    if appid == '00000002-0000-0000-c000-000000000000':  # This is AD Read
+        scope = 'User.Read'
+    else:
+        scope = "user_impersonation"


It this a right assumption? If you like to focus on common permission, I suggest add a elif by comparing with the real id of impersonation and then keep the guid for the rest. Also move the code block to end since this is only for display.

We pass the scope into the payload and is required to grant permissions, so we can't move it. This is a common well know AAD GUID and as far as I know this scope needs to be set differently as per the code block.

Want to get on the same page with you. The 00000002-0000-0000-c000-000000000000, to me, is the resource id of AAD graph. Scope in this context, means delegated permissions. Right?
If yes, the code here means if the resource is AAD graph, the delegated permission will be READ. For native apps, this permission is way too limited, can we also use user_impersonation, or expose as an argument if we can't make a reasonable default here?

Removed this for now and we can re-implement if required

yugangw-msft · 2018-08-07T18:48:17Z

src/command_modules/azure-cli-role/azure/cli/command_modules/role/custom.py

+
+    if not sps:
+        raise CLIError("'--id'" + identifier + " does not exist on this directory")
+    client_spn = sps[0].object_id


Suggest name it sp_object_id, as object id is not a service principal name. SPN is either related application's id, or app id uri.

Implemented as client_sp_object_id

yugangw-msft · 2018-08-07T18:50:58Z

src/command_modules/azure-cli-role/azure/cli/command_modules/role/custom.py

+    # Get the Service principal ObjectId for the client app
+
+    sps = list(graph_client.service_principals.list(
+        filter="appId eq '{}'".format(identifier)))


I would suggest you just call list_sps in the same file.

Extended list_sps to include search by appid/clientid

def list_sps(client, spn=None, display_name=None, query_filter=None, appid=None): sub_filters = [] if query_filter: sub_filters.append(query_filter) if spn: sub_filters.append("servicePrincipalNames/any(c:c eq '{}')".format(spn)) if display_name: sub_filters.append("startswith(displayName,'{}')".format(display_name)) if appid: sub_filters.append("appId eq '{}'".format(appid)) return client.list(filter=(' and '.join(sub_filters)))

yugangw-msft · 2018-08-07T18:51:43Z

src/command_modules/azure-cli-role/azure/cli/command_modules/role/_params.py

@@ -42,6 +42,11 @@ def load_arguments(self, _):
                   help="resource scopes and roles the application requires access to. Should be in manifest json format. See examples below for details")
        c.argument('native_app', arg_type=get_three_state_flag(), help="an application which can be installed on a user's device or computer")

+    with self.argument_context('ad app grant') as c:
+        c.argument('identifier', options_list=['--id'], help='clientId of the app you want to grant permissions to')
+        c.argument('appid', help='clientId of the app you want to grant permissions for')


should be --app-id

Implemented

yugangw-msft · 2018-08-07T18:54:19Z

src/command_modules/azure-cli-role/azure/cli/command_modules/role/_params.py

@@ -42,6 +42,11 @@ def load_arguments(self, _):
                   help="resource scopes and roles the application requires access to. Should be in manifest json format. See examples below for details")
        c.argument('native_app', arg_type=get_three_state_flag(), help="an application which can be installed on a user's device or computer")

+    with self.argument_context('ad app grant') as c:
+        c.argument('identifier', options_list=['--id'], help='clientId of the app you want to grant permissions to')


I suggest do not redefine, rather let the default one take over

Implemented

yugangw-msft · 2018-08-07T18:56:45Z

src/command_modules/azure-cli-vm/azure/cli/command_modules/vm/commands.py

@@ -91,8 +91,7 @@ def load_command_table(self, _):

    compute_vm_run_sdk = CliCommandType(
        operations_tmpl='azure.mgmt.compute.operations.virtual_machine_run_commands_operations#VirtualMachineRunCommandsOperations.{}',
-        client_factory=cf_run_commands,


Please rebase and get rid of the change which have been merged into the public repo

yugangw-msft

A few more suggestions. Please also fix linter error reported by CI

yugangw-msft · 2018-08-08T16:20:15Z

src/command_modules/azure-cli-role/azure/cli/command_modules/role/custom.py

+    graph_client = _graph_client_factory(cmd.cli_ctx)
+
+    # Get the Service principal ObjectId for the client app
+    sps = list(list_sps(graph_client.service_principals, appid=identifier))


You can just use _resolve_service_principal which returns the object id you need.

Implemented and extended to query by appId

yugangw-msft · 2018-08-08T16:21:49Z

src/command_modules/azure-cli-role/azure/cli/command_modules/role/custom.py

@@ -556,14 +557,16 @@ def list_apps(client, app_id=None, display_name=None, identifier_uri=None, query
    return client.list(filter=(' and '.join(sub_filters)))


-def list_sps(client, spn=None, display_name=None, query_filter=None):
+def list_sps(client, spn=None, display_name=None, query_filter=None, appid=None):


please check out my comments below. If you use _resolve_service_principal, then this change can be reverted

yugangw-msft · 2018-08-08T16:28:48Z

src/command_modules/azure-cli-role/azure/cli/command_modules/role/custom.py

+    if appid == '00000002-0000-0000-c000-000000000000':  # This is AD Read
+        scope = 'User.Read'
+    else:
+        scope = "user_impersonation"


Want to get on the same page with you. The 00000002-0000-0000-c000-000000000000, to me, is the resource id of AAD graph. Scope in this context, means delegated permissions. Right?
If yes, the code here means if the resource is AAD graph, the delegated permission will be READ. For native apps, this permission is way too limited, can we also use user_impersonation, or expose as an argument if we can't make a reasonable default here?

yugangw-msft · 2018-08-08T16:29:51Z

src/command_modules/azure-cli-role/azure/cli/command_modules/role/custom.py

@@ -1076,7 +1141,8 @@ def _get_keyvault_client(cli_ctx):
    from azure.keyvault import KeyVaultClient, KeyVaultAuthentication

    def _get_token(server, resource, scope):  # pylint: disable=unused-argument
-        return Profile(cli_ctx=cli_ctx).get_login_credentials(resource)[0]._token_retriever()  # pylint: disable=protected-access
+        return Profile(cli_ctx=cli_ctx).get_login_credentials(resource)[
+            0]._token_retriever()  # pylint: disable=protected-access


Again, please back out the format change here and below. The change here is not correct anyway.

yugangw-msft

One last suggestion hope to help you write less code. Also please get CI passing by doing 2 things:
a. Add group help to ad app permission
b. fix lint error(you can just suppress them using #pylint: disable=no-member)

Once this finalized, I will coordinate to get you spec pr and sdk published

yugangw-msft · 2018-08-09T15:37:06Z

src/command_modules/azure-cli-role/azure/cli/command_modules/role/custom.py

@@ -884,9 +931,13 @@ def delete_service_principal_credential(cmd, identifier, key_id, cert=False):
        key_id, identifier))


-def _resolve_service_principal(client, identifier):
+def _resolve_service_principal(client, identifier=None, appid=None):


Can you please double check this is needed? appid and app id uri are both service principal names which the existing query below should cover it. I did a quick check just now for reference, but let me know if you do need it.

(env) D:\sdk\azure-cli>az ad app show --id e96b494b-61a2-4a42-a33c-e11ab1f1a6b6 { "appId": "e96b494b-61a2-4a42-a33c-e11ab1f1a6b6", <omit irrelevant fields> } (env) D:\sdk\azure-cli>az ad sp show --id e96b494b-61a2-4a42-a33c-e11ab1f1a6b6 { "appDisplayName": "yugangw-op2", "appId": "e96b494b-61a2-4a42-a33c-e11ab1f1a6b6", "appOwnerTenantId": "54826b22-38d6-4fb2-bad9-b7b93a3e9c5a", "objectId": "e0a58f57-dcae-48ab-98b6-ee173a6097fa", "objectType": "ServicePrincipal", servicePrincipalNames": [ "http://yugangw-op2", "e96b494b-61a2-4a42-a33c-e11ab1f1a6b6" ], "servicePrincipalType": "Application", <omit irrelevant fields> }

@yugangw-msft I can confirm that the appId can be the same as the objectId, but it is not always. In all of my test cases, like for example creating an app in the portal, the SP associated with the app will be autogenerated and needs to be searched for by the appId. The code is definitely needed.

object id is irrelevant here which is different thing from the app id. I am talking about the app id of the application. The service principal provisioned in the tenant for the application should have the app id as part of the servicePrincipalNames array. If you see the appid is not in the servicePrincipalNames of the SP then your change is definitely needed. To get my comment clear, I am talking about the following array in the SP object

servicePrincipalNames": [ "http://yugangw-op2", "e96b494b-61a2-4a42-a33c-e11ab1f1a6b6" <---this one ],

@yugangw-msft Not sure if I am missing something here, this is the flow:

I have the appId/clientId of my app

I need to get the objectId of the SP associated with my app

The app array does not contain the objectId of the SP

I cannot query the SP by the objectId as I do not have the objectId, I only have the appId

I query the SP by the appId which gives me the SP ObjectId

The flow is right. My comment is on step 5 that you don't need to update _resolve_service_principal to take a new argument of appid, rather just pass the app id to the existing argument of identifier which should work.
The reason is the _resolve_service_principal queries for SP using servicePrincipalNames, and appid is one of service principal name, so the query should just work.
If you have tried but not working, please let me know

@yugangw-msft , thank you, in my tests this was not working for me. Will try again

just pass the app id to the existing argument of identifier which should work

thanks. Is it easy for you to share a repro for me to try out?

@yugangw-msft , you are right - my tests were bad, will remove appId from query and rebase

yugangw-msft · 2018-10-18T22:31:42Z

All are taken care through #7611

shanepeckham requested a review from yugangw-msft August 7, 2018 13:43

shanepeckham changed the title ~~run-command: bug fix so it doesn't show up under stack profile (#6906)~~ Add new az ad app grant command to grant OAuth2 permissions to AAD registered apps Aug 7, 2018

shanepeckham mentioned this pull request Aug 7, 2018

Added OAuth2 GET and POST to GraphRBAC.json spec Azure/azure-rest-api-specs#3563

Merged

10 tasks

troydai assigned yugangw-msft Aug 7, 2018

yugangw-msft reviewed Aug 7, 2018

View reviewed changes

shanepeckham force-pushed the az_ad_app_grant_command branch from 1a34dd9 to a0ae68d Compare August 8, 2018 12:55

yugangw-msft mentioned this pull request Aug 8, 2018

Manifest not being applied until button in portal is clicked #6989

Closed

shanepeckham changed the title ~~Add new az ad app grant command to grant OAuth2 permissions to AAD registered apps~~ Add new az ad app permissions grant and list commands for OAuth2 permissions for AAD registered apps Aug 8, 2018

yugangw-msft reviewed Aug 8, 2018

View reviewed changes

shanepeckham force-pushed the az_ad_app_grant_command branch from 5a2acc1 to 2466c9b Compare August 9, 2018 10:42

Rebase az ad app permissions list grant

4445dc3

shanepeckham force-pushed the az_ad_app_grant_command branch from 2466c9b to 4445dc3 Compare August 9, 2018 11:03

yugangw-msft reviewed Aug 9, 2018

View reviewed changes

Rebased az ad app permissions list grant

9b49a63

tjprescott added the RBAC az role label Aug 9, 2018

yugangw-msft mentioned this pull request Sep 17, 2018

[AutoPR graphrbac/data-plane] Added OAuth2 GET and POST to GraphRBAC.json spec Azure/azure-sdk-for-python#3063

Merged

shanepeckham added 2 commits September 27, 2018 15:13

Merge branch 'dev' into az_ad_app_grant_command

9d8ca16

Update _help.py

63312de

yugangw-msft mentioned this pull request Oct 18, 2018

Az ad app grant command #7611

Merged

2 tasks

yugangw-msft closed this Oct 18, 2018

haroldrandom added the Authorization-cli label Oct 25, 2019

Add new az ad app permissions grant and list commands for OAuth2 permissions for AAD registered apps #6975

Add new az ad app permissions grant and list commands for OAuth2 permissions for AAD registered apps #6975

Conversation

shanepeckham commented Aug 7, 2018 • edited Loading

yugangw-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shanepeckham Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shanepeckham Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yugangw-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yugangw-msft left a comment • edited Loading

Choose a reason for hiding this comment

yugangw-msft Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yugangw-msft Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

shanepeckham Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

yugangw-msft Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

shanepeckham Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yugangw-msft commented Oct 18, 2018

shanepeckham commented Aug 7, 2018 •

edited

Loading

shanepeckham Aug 8, 2018 •

edited

Loading

shanepeckham Aug 8, 2018 •

edited

Loading

yugangw-msft left a comment •

edited

Loading

yugangw-msft Aug 9, 2018 •

edited

Loading

yugangw-msft Aug 9, 2018 •

edited

Loading

shanepeckham Aug 9, 2018 •

edited

Loading

yugangw-msft Aug 9, 2018 •

edited

Loading

shanepeckham Aug 9, 2018 •

edited

Loading