Auth settings should be configured from the configuration provider (Lombiq Technologies: OCORE-115)

[hishamco]

Fixes #13064

[hishamco]

@Piedone I just tested AzureADSettings now you can use IOptions<AzureADSettings> to ready the settings from both configurations or site settings, but we might need to find a way to make unify the AUTH options & OC Auth specific options

[Piedone]

Try to configure an app (dummy parameters are suitable) with this from appsettings:

    "OrchardCore_Microsoft_Authentication_AzureAD": {
      "DisplayName": "Dummy AD login",
      "AppId": "dummy",
      "TenantId": "dummy",
      "CallbackPath": "/signin-oidc"
    }

This won't show the external login button the login screen, unless you also configure one from the admin.

Test all the login providers in a similar way.

[hishamco]

This won't show the external login button the login screen, unless you also configure one from the admin.

Let me check ..

[hishamco]

@Piedone regarding this one if you called ConfigureAzureADSettings() the login button will show up now, but if you are using the admin settings it will show up once you configure the options

[Piedone]

The point is, that if you configure nothing from the admin, then using ConfigureAzureADSettings() alone should make external login possible. If you don't use it, then the admin configuration alone. If you use both, then ConfigureAzureADSettings() should override the admin configuration. Is this what's happening?

[hishamco]

The point is, that if you configure nothing from the admin, then using ConfigureAzureADSettings() alone should make external login possible

I tested this case, it works as expected

If you don't use it, then the admin configuration alone

This works too

If you use both, then ConfigureAzureADSettings() should override the admin configuration. Is this what's happening?

Supposed to be, I will double check it now after the latest changes, but I'm sure it was working

[hishamco]

All the cases have been tested on Azure AD, I might need to do it with other social AUTH provider, but it should be the same after the latest changes

[jtkech]

At least for this one, maybe the same for others as twitter ad so on.

In the above lines 70 to 72

ServiceDescriptor.Transient<IConfigureOptions<AuthenticationOptions>, AzureADOptionsConfiguration>(),
ServiceDescriptor.Transient<IConfigureOptions<AzureADOptions>, AzureADOptionsConfiguration>(),
ServiceDescriptor.Transient<IConfigureOptions<PolicySchemeOptions>, AzureADOptionsConfiguration>(),

We are configuring AuthenticationOptions, AzureADOptions and PolicySchemeOptions, done by the same AzureADOptionsConfiguration, look at it, it uses values from site settings sections to configure the 3 above options. This is these options that I think are resolved somewhere through IOptions<>.

So if AzureADSettings is never resolved through e.g. IOptions<AzureADSettings>, this is useless, even if you tried it by doing it explicitly for testing.

What you would have to do instead is to also configure the 3 same above options AuthenticationOptions, AzureADOptions and PolicySchemeOptions, as done by AzureADOptionsConfiguration that uses site settings, but with another implementation using config values. And I think use IPostConfigureOptions if the intention is that config values overrides site settings, or still register them after, but I recommend post config to well show the intention.

---

Also maybe check for each value if a config value is provided before overriding those from site settings, or maybe only use a config value if site settings doesn't already provide a value, or for simplicity maybe an option to say if all should be configured from config values or all from site settings indifferently.

Yes I know it may be more or less complex but here we need to know what we want to do. If we use post configure it means that config values will have an higher precedence, so to work as the regular config stack I recommend to just check: if a config value is provided from config before overriding those from site settings.

Yes the regular config stack work like this, if a config source doesn't provide a value it keeps the possible one provided by a previous source, here site settings becoming another config source. The only difference is that site settings can't have a medium priority, it can only be used before or after the config values which are already the result of all other regular config sources. Here by using post configure to use config values, site settings have the lowest priority.

Hmm, but some people will say that what is in site settings should be used, other will say the same for config values, so maybe need to be discussed, hmm one solution on the fly would be to show any currently used value and make it non editable if different than the value in the database, meaning it is overridden by a config value. But first need to confirm the intention, then will see for editing, maybe in a separate PR.

Oops sorry for the long comment ;)

Also did you see the warning

// The net5.0 5.0.3 build obsoletes 'AzureADOptions' and 'AzureADDefaults',
//  'Microsoft.Identity.Web' should be used instead.

I really need to leave ;) will see tomorrow.

[hishamco]

Oops sorry for the long comment ;)

Ya its :)

Also did you see the warning

We might rid off these in another PR

[jtkech]

@Piedone So, after reading one of your comment.

The point is, that if you configure nothing from the admin, then using ConfigureAzureADSettings() alone should make external login possible. If you don't use it, then the admin configuration alone. If you use both, then ConfigureAzureADSettings() should override the admin configuration. Is this what's happening?

Okay about the goal, this is what I assumed in one of my too long review comment ;) So AzureADSettings becoming another config source but with a lower precedence. Here my recommendation, when configuring a related IOptions<> with a config value, would be to check if a valid config value exists before overriding a value that may already come from site settings.

Then about the UI there are different options, in the editor we may want to see what is currently used or what is currently in the database, for simplicity we could keep the last option and maybe something indicating if an used value is different than the one in the database, maybe in a separate PR.

---

About the implementation as it was done at some point (some changed was done in the meantime).

AzureADSettings site settings are used in fact to configure AspnetCore options as AzureADOptions but not only through AzureADOptionsConfiguration which is a IConfigureOptions<> of these AspnetCore options. This is these AspnetCore IOptions<> that are resolved and used somewhere, so configuring a new IOptions<AzureADSettings> with an app extension or module startup had no effect.

So my 1st thought was to keep things as is and only add post configurations of the above AspnetCore options with an implementation like the existing AzureADOptionsConfiguration but using config values.

---

In the meantine I saw that @hishamco created a new AzureADSettingsConfiguration that configures IOptions<AzureADSettings> from site settings, while the app level post configure extension can still do that but from config values. And I saw that the existing AzureADOptionsConfiguration now injects and uses IOptions<AzureADSettings> in place of site settings to still configure AspnetCore options.

Looks like to be a good idea, one advantage is that the same AzureADOptionsConfiguration is always used e.g. to do some validations. And with the app level extension, as I remember when we bind from config to an instance of AzureADSettings it only overrides a field if the configured value exists.

Hmm, another advantage while editing is that it will be easier to inject IOptions<AzureADSettings> to indicate if values are the same than in the database, but maybe in another PR.

So maybe all is good now, I trust you, only need to be tried ;)

Edited: Just for info the only concern I think is when binding a configured boolean. Knowing that any config value can be queried individually with GetSection(), one solution is to use GetSection() which returns a IConfigurationSection that has a property Exists or SectionExists (don't remember the name).

[hishamco]

Furthermore we need to rid of deprecated Azure AD options in separated PR