Enhancement add cliEntraAppId and cliEntraAppTenant configurations settings for cli config commands

[mkm17]

As discussed in this thread #5985 (comment)

The aim of the task is to add the cliEntraAppId and cliEntraAppTenant options to the list of available settings for: cli config get , cli config list , cli config reset , cli config set.

Later the new options will be utilized in cli app add command.

The importance of the app ID setting would be as follows:

1. CLIMICROSOFT365_ENTRAAPPID
2. CLIMICROSOFT365_AADAPPID (obsolete)
3. cliEntraAppId (new setting option)
4. the default one

The importance of the tenant setting would be as follows:

1. CLIMICROSOFT365_TENANT
2. cliEntraAppTenant (new setting option)
3. "common"

[waldekmastykarz]

We should use the new config options wherever we refer to config.cliEntraAppId. I suggest that the value configured in the CLI takes precedence over env var (so the order for the app ID is 3, 1, 2, 4 and tenant 2, 1, 3)

[mkm17]

ok, noted. You can assign me to this task since I've already made some research on this topic.

[milanholemans]

Currently, all our config settings are not tenant-bound. If we implement this, it will clash when switching connections (when switching to another tenant). I'm wondering if this is problematic here.

[mkm17]

Hi, @milanholemans Isn't it similar to using the 'CLI MICROSOFT365 TENANT' option?

Perhaps it's a helpful clue. There's a distinction when a user sets 'CLI MICROSOFT365 TENANT' parameter themselves and encounters an issue, versus when issues arise from setting it the 'supported way' in background, using cli app add (as mentioned in the task).

[milanholemans]

Yes, but CLIMICROSOFT365_TENANT is an environment variable.
If we introduce this both as an environment variable and config setting, I don't see the point anymore why we still support it as environment variable.

[mkm17]

Hi @milanholemans, The new configuration setting will be only used for cli app add to assign the newly created app for CLI purposes.
I agree that it would be best to use only one environment or configuration setting, but if I'm not mistaken, there's no possibility to assign environment settings from the code.

If you're referring only to the cliEntraAppTenant option, perhaps we can remove it. When we create a new app with cli app add, it's created under the current tenant scope, so maybe we don't need to set the tenant in the configuration settings.

But to summarize, we have a few options to choose from:

1. Simply don't add these configuration options and inform a user after running cli app add to set the configuration themselves using CLIMICROSOFT365_ENTRAAPPID and CLIMICROSOFT365_TENANT.
2. Add only one configuration option, cliEntraAppId, and assume that the tenant configuration is correct.
3. Add both options as described at the beginning.

What do you think @milanholemans ?

[milanholemans]

I have no idea whether or not you can update an environment variable to be honest. I'm just a bit concerned that we are creating now 2 paths to achieve the same thing, which I don't really like. In my humble opinion, we should either get rid of the env variables or just not set the app ID and tenant ID of the new app registration at all. Currently, I'm leaning towards the latter.

[mkm17]

@milanholemans I suppose removing environment variables is no longer an option since they are currently being used by users.

Ok, I believe the solution for cli app add would be to provide a message in the documentation on how to set environment variables. Not a perfect solution, but it won't cause any harm or confusion.

@waldekmastykarz Would it be ok?

[milanholemans]

@milanholemans I suppose removing environment variables is no longer an option since they are currently being used by users.

If needed, we can remove them in the next major release.

Have you tried setting an environment variable like we do here?

cli-microsoft365/src/appInsights.ts

Lines 3 to 5 in 5f64790

	process.env.APPLICATION_INSIGHTS_NO_DIAGNOSTIC_CHANNEL = 'none';
	// prevents tests from hanging
	process.env.APPLICATION_INSIGHTS_NO_STATSBEAT = 'true';

Or is this not something persistent?

[mkm17]

@milanholemans Exactly, I have tried the same approach:

Unfortunately, the result is as follows.

In debugging, I can see that this value is assigned correctly, but perhaps it is valid only during the execution.

[milanholemans]

Node.js docs confirm this indeed.

The process core module of Node.js provides the env property which hosts all the environment variables that were set at the moment the process was started.

[mkm17]

Hi @milanholemans, Should we then move in the direction of removing the environmental options CLIMICROSOFT365_ENTRAAPPID, CLIMICROSOFT365_AADAPPID, and CLIMICROSOFT365_TENANT, and replace them with cliEntraAppId and cliEntraAppTenant configuration options, to be set by cli config set, as you mentioned in one of the previous messages?

[milanholemans]

Let's wait for a second opinion of @waldekmastykarz or another @pnp/cli-for-microsoft-365-maintainers.

[waldekmastykarz]

You can't update an env var from Node.js. You can only change its value for the current (your own) process, but when you restart it, it's reverted to its previous value. The only way to set it permanently, would be to update the correct value in shell rc file/registry, etc. which I don't think we should do.

Env vars are convenient in CI/CD scenarios, where they're easier to set than running CLI's configuration commands. On the other hand, for regular use, it's likely easier to use a command to set things up, than to have to dig in your profile to configure an env var.

While it seems like two ways to do the same thing, I'd say there's something to say for keeping both of them, because they're meant to support different use cases.

[milanholemans]

Hi @mkm17 , due to unforeseen recent developments regarding the PnP Management Shell app (https://www.youtube.com/watch?v=VNgc4k_gCT0), we are forced to change our approach. Unfortunately, this means that we have to close this issue in favor of PR #6260, where our new approach is being implemented. We hope for your understanding.

[mkm17]

Hi @milanholemans, do we also need still PR #5985? If the change in #6260 allows setting the appId and tenantId, I can update #5985 to enable users to create a custom Entra app.

[milanholemans]

Good catch @mkm17. I don't know what our stance is for that change. I think it still might come in handy. What do you think @waldekmastykarz?

[waldekmastykarz]

#6260 indeed allows you to configure app ID and tenant (but also cert and its properties), so it would duplicate the functionality. @mkm17 sorry this happened. Would you be willing to have a look at 6260 to see if we cover everything that you contributed?

[mkm17]

Hi @waldekmastykarz, that is even better! 😉 I just wanted to confirm if we can use your change later in the cli app add command (PR here -> #5985). Of course, if it's not needed, we can also close it.

[waldekmastykarz]

Good point! I've adjusted the code, extracting the portion for creating the Entra app into utils. If we're to reuse it in cli app add, we'd copy the relevant calls (resolveApis, createApp, grantAdminConsent), which all are in this function https://github.com/pnp/cli-microsoft365/pull/6260/files#diff-68345dab9b38ebb710ac76370c645344d0d283745787b5f12c05a8e295e47252R336