[AD] `az ad app federated-credential`: Support federated identity credentials #22727

jiasli · 2022-06-03T13:25:40Z

Related command

az ad app federated-credential

Description

Close #20582

Support federated identity credential on both

Application: https://docs.microsoft.com/en-us/graph/api/resources/federatedidentitycredentials-overview?view=graph-rest-beta
Service Principal: {doc not available yet}

Testing Guide

History Notes

[Component Name 1] BREAKING CHANGE: az command a: Make some customer-facing breaking change
[Component Name 2] az command b: Add some customer-facing feature

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

The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.

jiasli · 2022-06-03T13:26:48Z

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

@@ -99,6 +99,16 @@ def load_command_table(self, _):
        g.custom_command('credential list', 'list_application_credentials')
        g.custom_command('credential delete', 'delete_application_credential')

+    # Register federated-credential command group under both app and sp.
+    # We use federated-credential instead of federated-identity-credential or fic, in order to align with


We still need to finalize our choice on the command group naming for federatedIdentityCredential:

federated-credential: Consistent with Azure Portal, but inconsistent with the underlying REST API federatedIdentityCredential

federated-identity-credential: Consistent with the underlying REST API, but too long to type

fic: Consistent with the underlying REST API, but the user will need to learn a new term

federated-credential is probably a good middle ground

Done. Renamed.

jiasli · 2022-06-03T13:27:58Z

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

+            # TODO: We have to decide which name to use
+            #   --credential-id is consistent with API
+            #   --key-id is consistent with passwordCredentials and keyCredentials
+            c.argument('federated_identity_credential_id_or_name', options_list=['--credential-id', '--key-id'],


We need to decide whether show/update/delete command should take --credential-id or --key-id:

--credential-id: Consistent with REST API federatedIdentityCredentialId

--key-id: Consistent with other credentials’ (passwordCredentials, keyCredentials) keyId

For example, passwordCredentials returns keyId:

> az ad app credential list --id 233dd73b-72e3-424a-9367-7588d957267e [ { "customKeyIdentifier": null, "displayName": "rbac", "endDateTime": "2023-05-19T10:30:09Z", "hint": "m6_", "keyId": "5858baa7-1fe3-4659-9ee8-2d4a172b79a4", "secretText": null, "startDateTime": "2022-05-19T10:30:09Z" } ]

but federatedIdentityCredential only returns id:

> az ad app federated-credential list --id 233dd73b-72e3-424a-9367-7588d957267e Command group 'ad app federated-credential' is in preview and under development. Reference and support levels: https://aka.ms/CLI_refstatus [ { "audiences": [ "api://AzureADTokenExchange" ], "description": "", "id": "a8d289e5-28eb-4f16-8243-cc8b43f2d9fe", "issuer": "https://token.actions.githubusercontent.com", "name": "github-fic", "subject": "repo:azure/azure-cli:pull_request" } ]

Can it support both?

Implementing both certainly causes unnecessary learning effort and complication.

jiasli · 2022-06-03T13:29:16Z

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

+            c.argument('audiences', nargs='*',
+                       help='Space-separated audiences that can appear in the external token. This field is mandatory, and '
+                            'defaults to "api://AzureADTokenExchange". It says what Microsoft identity platform should '
+                            'accept in the aud claim in the incoming token. This value represents Azure AD in your '
+                            'external identity provider and has no fixed value across identity providers - you may need '
+                            'to create a new application registration in your identity provider to serve as the audience '
+                            'of this token.')


We should decide whether create command should take JSON or flattened params:

JSON (like --parameters @fic_manifest.json): Consistent with manifest and REST API, but is difficult to input in terminal. It may also trigger PowerShell issue ([Shell] Azure CLI receives corrupted arguments in PowerShell #15529). Luckily that issue can be bypassed using file input.

Flattened params (like --audiences api://AzureADTokenExchange --issuer "https://token.actions.githubusercontent.com"): Easy to type in command line, but user has to run create command multiple times to create multiple credentials.

why is it difficult?

Typing a JSON by hand seems difficult to me. 😂

User would have a JSON file on disc and pass that to this param.

If that's the expected usage, I will implement that way.

yonzhan · 2022-06-03T14:33:09Z

Nice work to support federated identity credentials

jiasli · 2022-06-14T03:02:07Z

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

+def app_federated_credential_list(client, identifier):
+    object_id = _resolve_application(client, identifier)
+    # This call is too simple, so it's unnecessary to extract a common method for both application and servicePrincipal
+    # and invoke it like
+    # _federated_identity_credential_list(client.application_federated_identity_credential_list, object_id)
+    return client.application_federated_identity_credential_list(object_id)


We should be able to directly bind to the SDK and remove this custom function, but since we haven't decided whether to use flattened params or JSON (#22727 (comment)), we leave it here for future customization.

jiasli · 2022-06-14T03:40:25Z

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

+    for item in ['app']:
+        with self.command_group(f'ad {item} federated-credential',
+                                client_factory=get_graph_client, resource_type=PROFILE_TYPE,
+                                exception_handler=graph_err_handler, is_experimental=True) as g:


This command group is far from stable. We mark it as experimental.

jiasli · 2022-06-14T04:20:50Z

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

+                                client_factory=get_graph_client, resource_type=PROFILE_TYPE,
+                                exception_handler=graph_err_handler, is_experimental=True) as g:
+            for command in ['list', 'create', 'show', 'update', 'delete']:
+                g.custom_command(command, f'{item}_federated_credential_{command}')


This actually inspires me of a new naming convention.

Instead of using create_application for az ad app create (which is inconsistent):

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

Line 85 in 8f74ef4

g.custom_command('create', 'create_application')

the custom function can be named app_create to match the command name and the binding can be done in a uniform way.

Also, it becomes much easier to find the function in the function list:

ghost requested a review from yonzhan June 3, 2022 13:25

ghost added the Auto-Assign Auto assign by bot label Jun 3, 2022

ghost requested a review from wangzelin007 June 3, 2022 13:25

ghost assigned jiasli Jun 3, 2022

ghost added this to the Jun 2022 (2022-07-05) milestone Jun 3, 2022

ghost added the Graph az ad label Jun 3, 2022

ghost requested review from evelyn-ys and calvinhzy June 3, 2022 13:25

jiasli commented Jun 3, 2022

View reviewed changes

jiasli force-pushed the fic branch from 61e0655 to dd3dcad Compare June 13, 2022 09:37

jiasli commented Jun 14, 2022

View reviewed changes

fic

8f74ef4

jiasli force-pushed the fic branch from dd3dcad to 8f74ef4 Compare June 14, 2022 03:35

jiasli commented Jun 14, 2022

View reviewed changes

jiasli marked this pull request as ready for review June 14, 2022 03:40

jiasli changed the title ~~[Graph] az ad app/sp federated-credential: Support federated identity credentials~~ [AD] az ad app/sp federated-credential: Support federated identity credentials Jun 14, 2022

jiasli commented Jun 14, 2022

View reviewed changes

calvinhzy approved these changes Jun 16, 2022

View reviewed changes

evelyn-ys approved these changes Jun 16, 2022

View reviewed changes

jiasli changed the title ~~[AD] az ad app/sp federated-credential: Support federated identity credentials~~ [AD] az ad app federated-credential: Support federated identity credentials Jun 17, 2022

jiasli merged commit 33cbd01 into Azure:dev Jun 17, 2022

jiasli deleted the fic branch June 17, 2022 01:55

This was referenced Jul 6, 2022

{Core} Provide error recommendation for validate_file_or_dict #23121

Merged

[AD] az ad app federated-credential: Federated identity credential GA #23122

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AD] `az ad app federated-credential`: Support federated identity credentials #22727

[AD] `az ad app federated-credential`: Support federated identity credentials #22727

jiasli commented Jun 3, 2022 •

edited

Loading

jiasli Jun 3, 2022 •

edited

Loading

jongio Jun 8, 2022

jiasli Jun 13, 2022

jiasli Jun 3, 2022 •

edited

Loading

jongio Jun 8, 2022

jiasli Jun 9, 2022

jiasli Jun 3, 2022 •

edited

Loading

jongio Jun 8, 2022

jiasli Jun 9, 2022

jongio Jun 10, 2022

jiasli Jun 13, 2022

yonzhan commented Jun 3, 2022

jiasli Jun 14, 2022

jiasli Jun 14, 2022

jiasli Jun 14, 2022 •

edited

Loading

[AD] az ad app federated-credential: Support federated identity credentials #22727

[AD] az ad app federated-credential: Support federated identity credentials #22727

Conversation

jiasli commented Jun 3, 2022 • edited Loading

jiasli Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiasli Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiasli Jun 3, 2022 • 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

yonzhan commented Jun 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiasli Jun 14, 2022 • edited Loading

Choose a reason for hiding this comment

[AD] `az ad app federated-credential`: Support federated identity credentials #22727

[AD] `az ad app federated-credential`: Support federated identity credentials #22727

jiasli commented Jun 3, 2022 •

edited

Loading

jiasli Jun 3, 2022 •

edited

Loading

jiasli Jun 3, 2022 •

edited

Loading

jiasli Jun 3, 2022 •

edited

Loading

jiasli Jun 14, 2022 •

edited

Loading