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

OrchardCore.Email settings in appsettings.json not used #4611

Closed
mattcrossmarc opened this issue Oct 23, 2019 · 10 comments · Fixed by #4637
Closed

OrchardCore.Email settings in appsettings.json not used #4611

mattcrossmarc opened this issue Oct 23, 2019 · 10 comments · Fixed by #4637
Assignees
Labels
Milestone

Comments

@mattcrossmarc
Copy link

If I uncomment and configure the "OrchardCore.Email" settings in appsettings.json for OrchardCore.Cms.Web, the settings appear to never be used. For example, if I enable the Users Registration feature, when I attempt to navigate to the register page, the SmtpService is resolved, but the IOptions instance injected at construction does not have these values.

Here are the settings from appsettings.json, with sensitive values masked:

"OrchardCore": {
   ...
    // Uncomment to configure SMTP settings.
    "OrchardCore.Email": {
      "DeliveryMethod": "Network",
      "DefaultSender": "****",
      "Host": "*****",
      "Port": "465",
      "RequireCredentials": true,
      "EncryptionMethod": "SSLTLS",
      "UserName": "*****",
      "Password": "******"
    }
  }

Is this still supported as a configuration option for SMTP settings.

Thanks,
Matt

@hishamco hishamco added the Email label Oct 23, 2019
@hishamco
Copy link
Member

Let me check ..

@hishamco
Copy link
Member

Seems the options came from the Settings only, we need to support this from appsettings.json too

@hishamco hishamco self-assigned this Oct 23, 2019
@sebastienros sebastienros added this to the 1.0.x milestone Oct 24, 2019
@mattcrossmarc
Copy link
Author

Thanks @hishamco . Appreciate the help with this!

@hishamco
Copy link
Member

I already start working on this, after that went to some localization stuff .. coming back to this :)

@deanmarcussen
Copy link
Member

We should check this from a design perspective (and I'm not saying it's a bad idea, just that we should check)

As far as I know, at the moment configuring anything is done either through IShellConfiguration, i.e. appsettings.json, or through ISite settings, i.e. database

I don't think there are any places were we support configuring something through both techniques.

Not against it, but if we do it for email, then we have to do it for media settings, azure, data protection etc.

We would also need to make a choice about which overrides the other, if both are configured.

Does appsettings win, or do database settings win?

@deanmarcussen
Copy link
Member

@mattcrossmarc Your mitigation here, in case it isn't clear, is to enable the Email Module / Feature, and configure it through site settings, in the admin

@hishamco
Copy link
Member

Not against it, but if we do it for email, then we have to do it for media settings, azure, data protection etc.

You 're right

We would also need to make a choice about which overrides the other, if both are configured.

IMHO appsettings.json settings should override the Admin settings, but let us hear from other folks

@mattcrossmarc
Copy link
Author

mattcrossmarc commented Oct 25, 2019

Thanks guys. If you decide not to support appsettings for SMTP, it would be good to remove the example from the doc and OrchardCore.Csm.Web/appsettings.json. Those are what led me to try it:

https://orchardcore.readthedocs.io/en/dev/OrchardCore.Modules/OrchardCore.Email/#smtp-settings
https://github.com/OrchardCMS/OrchardCore/blob/dev/src/OrchardCore.Cms.Web/appsettings.json

My vote would be to support both options. In my development, I find myself frequently deleting App_Data and restarting from my custom recipe file. While I can (and have) included most SMTP settings in the recipe file, I can't bring myself to put Username and Password in source control, even for a test account. So, I have to reconfigure these every time I publish the site.

The nice thing about the ASP.NET configuration system is I can use User Secrets for local development and other options for test and prod environments so I don't have to store secrets in code. In my case, I'm using AWS Parameter Store, but it's easy enough to pull them from where ever by building a custom provider.

My general preference (although I understand it may take some iterating to get there) would be for site settings to be exposed as part of the ASP.NET configuration system as well. That way, there is one way to read config settings in code, whether stored in the database or elsewhere. Also, it allows for fairly simple changes to precedent by changing the order of the configuration providers.

Out of the box, I think it would probably make most sense to OrchardCore administrators if the settings they can see in Admin take precedent over any settings read from appsettings.json or otherwise. That way, if they need to change the values in a running environment, they can use the nice Admin experience to do that rather than calling on someone to change settings elsewhere.

@deanmarcussen
Copy link
Member

@mattcrossmarc Wanted to give you an update on this to clear any confusion.
We went back and forth a bit over it, and have ended up removing the reference in the documentation, and default appsettings.json file, as it looked like it made it in there by mistake.

But you make a compelling argument above, so here is my counter argument :)

Configuration, and UI Settings are currently 2 separate areas of concerns, and quite loosely connected.

The loose connection is made with the IOptions<T> pattern.

So most anything that is loaded via IShellConfiguration (an extension on IConfiguration) is then configured in DI against an IOptions registration.

Or anything that is set by the UI Settings (Site settings), is also then registered against an IOptions registration.

The IOptions are where the two different ways of setting something meet.

So the way to achieve what you were wanting to do, which is configure email from IConfiguration is to simply override the IOptions<SmtpSettings> Di registration from your module, when in development mode, by increasing the Startup.Order in the module, so the registration comes in after the Settings UI one.

This is also the way aspnetcore configures things, and how you change aspnetcore IOptions to suit your needs.

Does this work for you?

If you have alternate views, please feel free to re-open

@mattcrossmarc
Copy link
Author

@deanmarcussen Thanks for the update and for the guidance on implementing it myself. I think this will work just fine for me. Appreciate you all giving this so much consideration and offering an alternative.

Matt

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

Successfully merging a pull request may close this issue.

4 participants