-
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
[AD] az ad app federated-credential
: Support federated identity credentials
#22727
Conversation
@@ -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 |
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 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 typefic
: Consistent with the underlying REST API, but the user will need to learn a new term
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.
federated-credential is probably a good middle ground
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.
Done. Renamed.
# 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'], |
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 need to decide whether show/update/delete
command should take --credential-id
or --key-id
:
--credential-id
: Consistent with REST APIfederatedIdentityCredentialId
--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"
}
]
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.
Can it support both?
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.
Implementing both certainly causes unnecessary learning effort and complication.
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.') |
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 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 runcreate
command multiple times to create multiple credentials.
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.
why is it difficult?
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.
Typing a JSON by hand seems difficult to me. 😂
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.
User would have a JSON file on disc and pass that to this param.
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.
If that's the expected usage, I will implement that way.
Nice work to support federated identity credentials |
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) |
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 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.
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: |
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.
This command group is far from stable. We mark it as experimental.
az ad app/sp federated-credential
: Support federated identity credentialsaz ad app/sp federated-credential
: Support federated identity credentials
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}') |
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.
This actually inspires me of a new naming convention.
Instead of using create_application
for az ad app create
(which is inconsistent):
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:
az ad app/sp federated-credential
: Support federated identity credentialsaz ad app federated-credential
: Support federated identity credentials
Related command
az ad app federated-credential
Description
Close #20582
Support federated identity credential on both
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 featureThis 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.