Skip to content
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

New command: create Azure AD app registration #1962

Closed
waldekmastykarz opened this issue Nov 20, 2020 · 10 comments
Closed

New command: create Azure AD app registration #1962

waldekmastykarz opened this issue Nov 20, 2020 · 10 comments

Comments

@waldekmastykarz
Copy link
Member

waldekmastykarz commented Nov 20, 2020

Usage

aad app add [options]

Description

Creates Azure AD app registration

Options

Option Description
-n, --name <name> Name of the app
--multitenant Specify to make the app available to other tenants
-r, --redirectUris [redirectUris] Comma-separated list of redirect URIs
-p, --platform [platform] Platform for which the redirectUris should be configured. Allowed values spa,web,publicClient
--implicitFlow Specify, to indicate that the authorization endpoint should return ID and access tokens
-s, --withSecret When specified, will create a non-expiring secret for the app named Default
--apisDelegated [apisDelegated] Comma-separated list of delegated permissions to register with the app
--apisApplication [apisApplication] Comma-separated list of application permissions to register with the app
-u, --uri [uri] Application ID URI
--scopeName [scopeName] Name of the scope to add. Requires uri to be specified
--scopeConsentBy [scopeConsentBy] Specifies if the scope can be consented only by admins or by admins and users. Allowed values admins, adminsAndUsers. Default admins
--scopeAdminConsentDisplayName [scopeAdminConsentDisplayName] Scope admin consent display name
--scopeAdminConsentDescription [scopeAdminConsentDescription] Scope admin consent description

Additional Information

After creating the app registration, the command sets the current user as its owner.

The command returns app's ID, ObjectID and the generated secret if one was created.

This command is similar to az ad app create and is meant to simplify the process of creating app registrations with supporting the most common scenarios in an opinionated way.

@waldekmastykarz waldekmastykarz added new feature needs discussion needs peer review Needs second pair of eyes to review the spec or PR labels Nov 20, 2020
@waldekmastykarz
Copy link
Member Author

@pnp/cli-for-microsoft-365-maintainers I'd love your feedback on the above spec

@plamber
Copy link
Contributor

plamber commented Nov 22, 2020

Hi @waldekmastykarz,
not sure about the scope section because I find it difficult to oversimplify that part. Could we use the same naming of permissions as in #1963 instead of using "apis"?

Instead of providing a "secretname" I would just provide a flag --secret. I would also provide an option to generate an upload a certificate with a flag.

Cheers,

@waldekmastykarz
Copy link
Member Author

not sure about the scope section because I find it difficult to oversimplify that part. Could we use the same naming of permissions as in #1963 instead of using "apis"?

The name apis is mentioned to more easily map the option to the UI/docs for reference. When creating a registration, there are a few different types of permissions that you can set. Would apiPermissions be clearer?

Instead of providing a "secretname" I would just provide a flag --secret. I would also provide an option to generate an upload a certificate with a flag.

I've been thinking about this too, but to me an option named secret indicates specifying the actual secret, where in fact, we're expecting the user provide the name of the secret and we generate the secret ourselves.

As for the cert: would you allow folks to upload their own cert or would you rather have them indicate that they want a cert and we then create one on the fly for them?

@garrytrinder
Copy link
Member

--multitenant Specify to make the app available to other tenants

How about --available-to-other-tenants [true|false], if omitted, is defaulted to false, same as how Azure CLI implements it.

--implicitFlow Specify, to indicate that the authorization endpoint should return ID and access tokens

As above, how about --allow-implicit-flow [true|false] ?

-s, --secretName [secretName] When specified, will create a non-expiring secret for the app

As above, how about --use-secret [true|false] ?

-a, --apis [apis] Comma-separated list of permissions to register with the app

When we say permissions, do we mean scopes?

https://graph.microsoft.com/User.Read,https://microsoft.sharepoint-df.com/Sites.Read.All vs User.Read,Sites.Read.All

-u, --uri [uri] Application ID URI
--scopeName [scopeName] Name of the scope to add. Requires uri to be specified
--scopeConsentBy [scopeConsentBy] Specifies if the scope can be consented only by admins or by admins and users. Allowed values admins, adminsAndUsers. Default admins
--scopeAdminConsentDisplayName [scopeAdminConsentDisplayName] Scope admin consent display name
--scopeAdminConsentDescription [scopeAdminConsentDescription] Scope admin consent description

I would consider these options as advanced and would possibly drop them from the initial command, are these options commonly used?

As for the cert: would you allow folks to upload their own cert or would you rather have them indicate that they want a cert and we then create one on the fly for them?

I think we should implement the creation of the certificate for the user.

@waldekmastykarz
Copy link
Member Author

waldekmastykarz commented Nov 27, 2020

How about --available-to-other-tenants [true|false], if omitted, is defaulted to false, same as how Azure CLI implements it.

We don't use - to separate words. So instead, we could use --availableToOtherTenants. I don't think it needs to be a true|false. If we use it as a flag, if not specified, it's a false, so it's easier. Naming-wise, my preference would be still --multitenant which is shorter 🙂

As above, how about --allow-implicit-flow [true|false]

See the remark above

As above, how about --use-secret [true|false]

For the secret we have to specify name, so it's not enough to just say useSecret. We could of course always use a dummy name like Default and then change the option to be named --withSecret

When we say permissions, do we mean scopes?

Yes

I would consider these options as advanced and would possibly drop them from the initial command, are these options commonly used?

In Teams development they are 🙂

I think we should implement the creation of the certificate for the user.

👍

@garrytrinder
Copy link
Member

We don't use - to separate words. So instead, we could use --availableToOtherTenants. I don't think it needs to be a true|false. If we use it as a flag, if not specified, it's a false, so it's easier. Naming-wise, my preference would be still --multitenant which is shorter 🙂

Makes sense, lets go with that 🙂

For the secret we have to specify name, so it's not enough to just say useSecret. We could of course always use a dummy name like Default and then change the option to be named --withSecret

I think setting a default name would be my preferred option, default sounds good, so lets go --withSecret.

In Teams development they are 🙂

I see, happy to go with your suggestion then as I've not done much Teams development, hence why I've not used them 😄

@waldekmastykarz
Copy link
Member Author

waldekmastykarz commented Nov 28, 2020

I see, happy to go with your suggestion then as I've not done much Teams development, hence why I've not used them 😄

Additional thing I was thinking is, that as long as they're not required, there is no problem with introducing them, if there is a chance they'd be helpful.

Updated spec. Is it ready to go now?

@garrytrinder
Copy link
Member

Spec is ready but just to be picky, shouldn't it be multiTenant instead of multitenant? 🙂

@waldekmastykarz
Copy link
Member Author

Following Microsoft's spelling, it's multitenant

image

@waldekmastykarz waldekmastykarz added help wanted and removed needs discussion needs peer review Needs second pair of eyes to review the spec or PR labels Nov 28, 2020
@garrytrinder
Copy link
Member

Well, I can't argue with that 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants