Add ability to override default excluded permissions with an empty array in GraphMinimalPermissionsGuidancePlugin

[bartizan]

In the GraphMinimalPermissionsGuidancePlugin config, the permissionsToExclude property currently defaults to ["profile", "openid", "offline_access", "email"]. This default applies when the property is undefined, null, or an empty array ([]).

We need to add the ability to override this default list with an empty list of permissions (meaning none will be excluded).

The suggested behavior is that if the permissionsToExclude property is explicitly defined in the configuration, it will completely replace the default permissions.

Here's a breakdown of the expected behavior for the permissionsToExclude property:

permissionsToExclude property	Value	Result
undefined	absent	Default list
defined	null	[ ]
defined	[ ]	[ ]
defined	a non-empty, e.g., ["ids.user.read"]	["ids.user.read"]

Theoretically, a null value for the property could be interpreted as the default permissions, but I'd prefer not to implement that.

How should we proceed with that?

[bartizan]

I'm working on it

[waldekmastykarz]

Where in the code do you see treating an empty array as the default list? As far as I can see, we only revert to default on undefined/null here:

dev-proxy/DevProxy.Plugins/Reporting/GraphMinimalPermissionsGuidancePlugin.cs

Line 69 in 3d31553

if (Configuration.PermissionsToExclude is null)

[bartizan]

Where in the code do you see treating an empty array as the default list? As far as I can see, we only revert to default on undefined/null here:

We add a function to the base plugin class that would resolve a value based on provided property name and its default value. There is we could access IConfiguration interface to check if property exists and its value.
And add a call of the function somewhere during plugin initialization.

[waldekmastykarz]

Let me rephrase, you said:

This default applies when the property is undefined, null, or an empty array ([])

But in our code currently I only see the comparison with null and not an empty array. Where in our code have you seen us default to ["profile", "openid", "offline_access", "email"] when the user specifies an empty array?

[bartizan]

...when the user specifies an empty array?

In brief, it is the line or so you have mentioned above.

The configuration binder works so that if the JSON setting value is null, an empty array ([]), or missing property, the bound value in the settings container is null (actually, it is the initial value of the property which in our case is null... I plan to use an empty list as initial).

dev-proxy/DevProxy.Plugins/Reporting/GraphMinimalPermissionsGuidancePlugin.cs

Line 36 in 3d31553

public IEnumerable<string>? PermissionsToExclude { get; set; }

---

Currently, the null comparison (the line you mentioned) happens with the property of the plugin settings container after its value has been transformed (bound) by Microsoft.Extensions.Configuration.Binder. This is where it happens

dev-proxy/DevProxy.Abstractions/Plugins/BasePlugin.cs

Line 93 in 3d31553

_configuration = ConfigurationSection.Get<TConfiguration>();

[waldekmastykarz]

So if I understand correctly, what you're saying is that the line I referred to in my previous comment is never hit because by the time the value reaches our code it's never null?

[bartizan]

So if I understand correctly, what you're saying is that the line I referred to in my previous comment is never hit because by the time the value reaches our code it's never null?

That statement is not true. Let's break down how it works now:

permissionsToExclude property in appsettings.json	Value (in appsettings.json)	Bound Value (null-check, the line 69)	Result
undefined	absent	null	Default list
defined	null	null	Default list
defined	[ ]	null	Default list
defined	a non-empty, e.g., ["ids.user.read"]	["ids.user.read"]	["ids.user.read"]

There may have been some confusion regarding the bound value. I wanted to clarify that in the upcoming changes, it will be set to an empty array, rather than null (as indicated in the "Bound Value" column).

[waldekmastykarz]

Ah, so the problematic case is setting the value to [] in the config, and it showing up as null in the code, correct?