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

Adds cli app add command, changes in entra app add and app set Closes #1963 #5985

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mkm17
Copy link
Contributor

@mkm17 mkm17 commented Apr 18, 2024

Adds cli app add command, changes in entra app add and app set

Hello, the PR adds a new command cli app add.

As described in issue #1963, the PR also includes changes to entra app add and entra app set to extract some common code into utility files.

Regarding this command and all changes, I have some additional points to confirm:

  1. Is the location of the utility files correct?
  2. For the cli app add command, I've added the 'name' parameter, which wasn't defined initially in the scope of the task.
  3. In the task, there's information about cli config set, but as I understand, there's no option to set the default app ID using this command yet.
  4. Password creation, is that ok or it should be something more sophisticated?
  5. Certificate creation, I hope that it is ok, unfortunately, I haven't had the opportunity to test it on Mac.
  6. The premissionSets, I definied, are based on the list from https://pnp.github.io/cli-microsoft365/concepts/authorization-tokens . The only difference is the exclusion of the ‘Windows Azure Active Directory’ permission, which I understand to be legacy.

I am looking forward to all comments and suggestions.

@Jwaegebaert
Copy link
Contributor

Awesome work @mkm17, we'll try to review it ASAP!

@milanholemans
Copy link
Contributor

Hi @mkm17, I can give you a few answers

1. Is the location of the utility files correct?

Seems like the right location indeed.

2. For the `cli app add` command, I've added the 'name' parameter, which wasn't defined initially in the scope of the task.

This is to set the name of the app? Looks like a good enhancement to me.

3. In the task, there's information about `cli config set`, but as I understand, there's no option to set the default app ID using this command yet.

Strangely, the only way I know how to set a default app ID is by using environment variables. Since @waldekmastykarz created the issue a few years ago, let's ask him if he still remembers what we had to do for this task.

4. Password creation, is that ok or it should be something more sophisticated?

In the command entra user add we also made a password generator. It's capable of generating more complex passwords. Maybe for the sake of consistency, let's reuse that generator.

5. Certificate creation, I hope that it is ok, unfortunately, I haven't had the opportunity to test it on Mac.

We'll have a look at it during the review.

Great work!

@waldekmastykarz
Copy link
Member

3. In the task, there's information about `cli config set`, but as I understand, there's no option to set the default app ID using this command yet.

Strangely, the only way I know how to set a default app ID is by using environment variables. Since @waldekmastykarz created the issue a few years ago, let's ask him if he still remembers what we had to do for this task.

Correct, that said, we can totally add the option to set the app ID in CLI's settings if that's more convenient for general use.

@mkm17
Copy link
Contributor Author

mkm17 commented Apr 23, 2024

@milanholemans ok, I'll modify the function to generate a password and update the code

@waldekmastykarz Do you mean to extend the cli config set command with options like cliEntraAppId and cliEntraAppTenant? Then we can utilize them internally, such as process.env.CLIMICROSOFT365_ENTRAAPPID. As I understand, there is no way to set environment variables from the code.

In cli app add, we could include a parameter such as --overrideDefaultCliAppSettings to prevent surprising the user with settings changes.

What do you think?

@waldekmastykarz
Copy link
Member

@waldekmastykarz Do you mean to extend the cli config set command with options like cliEntraAppId and cliEntraAppTenant? Then we can utilize them internally, such as process.env.CLIMICROSOFT365_ENTRAAPPID. As I understand, there is no way to set environment variables from the code.

Correct, we'd introduce two new config values like you suggested. Then, in our auth logic, we'd use them on top of existing env vars. So the order is: custom config values, env vars, default.

In cli app add, we could include a parameter such as --overrideDefaultCliAppSettings to prevent surprising the user with settings changes.

Good idea. I suggest a name like --setInCliSettings to more clearly communicate the intent.

@mkm17
Copy link
Contributor Author

mkm17 commented Apr 23, 2024

@waldekmastykarz Alright, let's proceed like this: we can hold off on merging/reviewing this pull request for now. Instead, I'll open a new issue to outline the changes needed for the cli config set. This way, we avoid having one commit that affects too many areas and commands, as it currently touches also entra app add and 'entra app set'. Would it be ok?

@waldekmastykarz
Copy link
Member

I'll mark this PR as draft until the necessary changes are in

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

Successfully merging this pull request may close these issues.

None yet

4 participants