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

Azure AD settings (possibly others) can't actually be configured from the configuration provider #13064

Closed
Piedone opened this issue Jan 9, 2023 · 15 comments · Fixed by #13113
Assignees
Labels
Milestone

Comments

@Piedone
Copy link
Member

Piedone commented Jan 9, 2023

Describe the bug

To Reproduce

  1. Use ConfigureAzureADSettings() as instructed in the docs.
  2. Observe that no configuration takes effect.

This was added in #12033. The reason nothing happens is there is actually no IOptions or anything referencing AzureADSettings, it always comes from site settings via AzureADService.

Expected behavior

The Azure AD settings partially or fully can be provided by configuration providers like the appsettings file.

Note that other such extension added in #12033 should be checked as well to see if a similar commission exists.

@Piedone
Copy link
Member Author

Piedone commented Jan 9, 2023

@hishamco can you take a look at this issue and verify, please?

@hishamco
Copy link
Member

hishamco commented Jan 9, 2023

Unfortunately I don't have an Azure account, I had one from the old days, but I'm not sure if it's still active

@hishamco
Copy link
Member

hishamco commented Jan 9, 2023

I will check if it's still valid ..

@Piedone
Copy link
Member Author

Piedone commented Jan 9, 2023

Thank you! While having access to an AD is nice, it's not strictly necessary for troubleshooting this.

@hishamco
Copy link
Member

hishamco commented Jan 9, 2023

Azure AD settings (possibly others) can't actually be configured from the configuration provider

(possibly others) I can check the email settings if that's a case. Is the issue occurs when we have both configuration & database settings? Or is there a certain scenario?

@Piedone
Copy link
Member Author

Piedone commented Jan 9, 2023

E-mail settings I happen to know work.

The issue is that as far as I see, there's nothing that would make ConfigureAzureADSettings() take effect, since the provider-based configuration is never accessed.

@sebastienros sebastienros added this to the 1.x milestone Jan 12, 2023
@Piedone
Copy link
Member Author

Piedone commented Jan 15, 2023

Is this something you'll look into in the foreseeable future, @hishamco, or do you need help?

@hishamco
Copy link
Member

If someone from your side has access to AAD it would be better, meanwhile I will check a new bug reported in localization

@Piedone
Copy link
Member Author

Piedone commented Jan 15, 2023

You don't need AAD access for this. The only thing you need to check is whether after the fix (or rather, after implementing the logic necessary to actually load settings from the provider) is whether AzureADSettings is correct where it is retrieved.

@hishamco hishamco self-assigned this Jan 16, 2023
@hishamco
Copy link
Member

@Piedone seems there was a lack on the docs, when we add ConfigureAzureADSettings() on the OC.Cms.Web as the following:

builder.Services
    .AddOrchardCms()
    .AddSetupFeatures("OrchardCore.AutoSetup")
    .ConfigureAzureADSettings();

you can able to get the configuration from appsettings.json with IOptions<AzureADSettings>

I just spent an hour or less experiments reading the configuration from admin area as well as the configuration methods introduces in #12033

I will continue checking the other AUTH providers as well to make it sure that everything is working fine

Correct me if I miss some of your reproduced steps

@Piedone
Copy link
Member Author

Piedone commented Jan 16, 2023

So you're saying it matters whether it's called on OrchardCoreBuilder after AddOrchardCms() vs inside it? That's not how it should be, because every other such method, including AddSetupFeatures(), can be used in the delegate of AddOrchardCms():

builder.Services
    .AddOrchardCms(orchardCoreBuilder =>
    {
        orchardCoreBuilder
            .AllowMiniProfilerOnAdmin()
            .AddSetupFeatures("OrchardCore.AutoSetup")
            .ConfigureAzureADSettings();
    });

@hishamco
Copy link
Member

What I mean is whenever you call it via OrchardCoreBuilder the settings will come from configuration instead of admin area, I think that is what supposed to be. Am I right?

@Piedone
Copy link
Member Author

Piedone commented Jan 16, 2023

Not really. In both cases it's a method call on OrchardCoreBuilder so this difference shouldn't be there. I don't understand why there is, BTW, since all such configuration methods do the same, with PostConfigure().

@jtkech
Copy link
Member

jtkech commented Jan 17, 2023

@Piedone @hishamco

Just checked for AzureADSettings, unlike some it is never injected as e.g. IOptions<AzureADSettings> and there is no IConfigureOptions<AzureADSettings> that would use the site settings to configure these options, and that we could PostConfigure() with an extension.

In fact here it is a little more complex, there is a AzureADOptionsConfiguration that uses the related site settings sections, but this is not an IConfigureOptions<AzureADSettings>, because in fact this class is used to configure others AspnetCore options as I saw in the module startup.

// The net5.0 5.0.3 build obsoletes 'AzureADOptions' and 'AzureADDefaults', 'Microsoft.Identity.Web' should be used instead.
// The build warning is disabled temporarily until the code can be migrated.

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

So this is the above aspnetcore options that would need to be post configured from values coming from the configuration, and by looking what the config helper AzureADOptionsConfiguration does.

Also take care about the commented obsoletes warnings.

@hishamco
Copy link
Member

Just checked for AzureADSettings, unlike some it is never injected as e.g. IOptions and there is no IConfigureOptions that would use the site settings to configure these options, and that we could PostConfigure() with an extension.

You 're right @jtkech I saw this yesterday, same thing for other social AUTH providers

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