[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

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

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

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

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 calvinhzy and evelyn-ys 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

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

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

Uh oh!

Conversation

jiasli commented Jun 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jiasli Jun 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiasli Jun 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiasli Jun 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yonzhan commented Jun 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiasli Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[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