SqlDatabaseMail: Add parameter UseDefaultCredentials

[ghost]

Pull Request (PR) description

Added the parameter UseDefaultCredentials to the SqlDatabaseMailDsc resource to allow the DatabaseEngine service account to be used for SMTP server authentication. Changes required to this resource to support configuration of the account and password for "basic" or "anonymous" SMTP access is not addressed by this PR.

This Pull Request (PR) fixes the following issues

None

Task list

• Added an entry to the change log under the Unreleased section of the
  file CHANGELOG.md. Entry should say what was changed and how that
  affects users (if applicable), and reference the issue being resolved
  (if applicable).
• Resource documentation updated in the resource's README.md.
• Resource parameter descriptions updated in schema.mof.
• Comment-based help updated, including parameter descriptions.
• Localization strings updated.
• Examples updated.
• Unit tests updated. See DSC Community Testing Guidelines.
• Integration tests updated (where possible). See DSC Community Testing Guidelines.
• Code changes adheres to DSC Community Style Guidelines.

---

This change is 

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94%. Comparing base (0ffc7ad) to head (058cbd9).

Additional details and impacted files

@@         Coverage Diff          @@
##           main   #2001   +/-   ##
====================================
  Coverage    94%     94%           
====================================
  Files        93      93           
  Lines      7848    7862   +14     
====================================
+ Hits       7428    7442   +14     
  Misses      420     420           

Flag	Coverage Δ
unit	94% <100%> (+<1%)	⬆️

Files	Coverage Δ
...urces/DSC_SqlDatabaseMail/DSC_SqlDatabaseMail.psm1	100% <100%> (ø)

[johlju]

I will continue the review once these comment are fixed.

Reviewed all commit messages.
Reviewable status: 0 of 5 files reviewed, 6 unresolved discussions (waiting on @RandyInMarin)

---

CHANGELOG.md line 12 at r1 (raw file):

- SqlServerDsc
  - Added build tasks to generate Wiki documentation for public commands.
- SqlDatabaseMailDsc

Should be just SqlDatabaseMail

Suggestion:

SqlDatabaseMail

---

CHANGELOG.md line 16 at r1 (raw file):

    SMTP server authentication uses the DatabaseEngine service account. The
    default is $false which uses a login specified for SMTP authorization.
    If false and no custom login specified, an anonymous login is used.

How do we specify a custom login?

---

source/DSCResources/DSC_SqlDatabaseMail/DSC_SqlDatabaseMail.psm1 line 149 at r1 (raw file):

                    $returnValue['MailServerName'] = $mailServer.Name
                    $returnValue['TcpPort'] = $mailServer.Port
                    $returnValue['UseDefaultCredentials'] = $mailServer.UseDefaultCredentials # custom

Do we need this comment? It is non descriptive. Throughout.

---

source/DSCResources/DSC_SqlDatabaseMail/DSC_SqlDatabaseMail.psm1 line 241 at r1 (raw file):

        Controls SMTP server authentication.  The database engine credentials are
        the default credentials used when $true.  If not specified, the default is
        $false and an anonymous login is used unless other credentials are provided.

Same as a previous comment. How do we provide other credentials with this change?

---

source/DSCResources/DSC_SqlDatabaseMail/DSC_SqlDatabaseMail.psm1 line 241 at r1 (raw file):

        Controls SMTP server authentication.  The database engine credentials are
        the default credentials used when $true.  If not specified, the default is
        $false and an anonymous login is used unless other credentials are provided.

Suggest changing to this, please change as appropriate if you see it different 🙂 Throughout the changes.

Suggestion:

        Controls the SMTP server authentication. When set to `$true` the database engine
        credentials are used as the SMTP server authentication. When set to `$false` or
        not specified, anonymous login is used unless other credentials are provided.

---

source/DSCResources/DSC_SqlDatabaseMail/en-US/DSC_SqlDatabaseMail.strings.psd1 line 25 at r1 (raw file):

    MailServerPropertyServerName = server name
    MailServerPropertyTcpPort = TCP port
    MailServerPropertyUseDefaultCredentials = SMTP server authentication

We should change this to the property name.

Suggestion:

Use default credentials

[johlju]

@RandyInMarin great work on this! 🙂

[ghost]

@johlju Hi, can you guide me on how I apply the change to the fork? Do I fix locally and push to the existing fork? Then do I push or something the change to the existing pull request?

The "custom" comment does not belong. I copied the code and made edits. I decided to try fork and a pull request instead.

Currently SqlDatabaseMail supports only the $false case, the default. This is the case that uses the configured account or anonymous SMPT login. Adding the option for $true (the one I need) does not address the missing parameters to configure the SMPT login account. (There is another abandoned fork for that, I think.) For now, as before, the configuring the SMPT account is not done here. If somebody does set it via SSMS one at a time, they would have good reason to update this resource to add those parameters.) Is it okay to add support for $true and leave the existing $false support the same... with documentation explaining the lack of support for the login?

Passing in an account name for the login might not be bad. I would not know how to pass in a password securely.

BTW My use case does not include the LCM. I will be using invoke-dscresource. This method matches current practice at work but is more elegant. This should avoid future DSC v3 incompatibilities because existing resources will still work - but not configuration or mof files. That's my understanding, anyway.

[johlju]

@johlju Hi, can you guide me on how I apply the change to the fork? Do I fix locally and push to the existing fork? Then do I push or something the change to the existing pull request?

Yes, you push commits to the same working branch in your fork as is in the PR (you used main in your case)

The "custom" comment does not belong. I copied the code and made edits. I decided to try fork and a pull request instead.

We can update the text to reflect what works per the change.

Currently SqlDatabaseMail supports only the $false case, the default. This is the case that uses the configured account or anonymous SMPT login. Adding the option for $true (the one I need) does not address the missing parameters to configure the SMPT login account. (There is another abandoned fork for that, I think.) For now, as before, the configuring the SMPT account is not done here. If somebody does set it via SSMS one at a time, they would have good reason to update this resource to add those parameters.) Is it okay to add support for $true and leave the existing $false support the same... with documentation explaining the lack of support for the login?

Yes, just chnage the texts explaining what the parameter does. The other can be made in another PR.

Passing in an account name for the login might not be bad. I would not know how to pass in a password securely.

It is done through a parameter with the PSCredential type.

BTW My use case does not include the LCM. I will be using invoke-dscresource. This method matches current practice at work but is more elegant. This should avoid future DSC v3 incompatibilities because existing resources will still work - but not configuration or mof files. That's my understanding, anyway.

I think MOF resources will not work, only class-based resources, but haven't dug into DSCv3 yet.
I can't really tell, but looks like they will try to support both script-based (which I guess is MOF) and class-based resources.

[ghost]

@johlju I made the updates. I changed the documentation to isolate UseDefaultCredentials from basic or anonymous authentication. I used the notes section to clarify.

When designing a dsc resource, should parameters match those use in the SMO? For example, I think "Use default credential" plus an account name and password is confusing. It seems like a UseDefaultCredential switch would be clearer. If the switch is specified, then the account name and password are not allowed parameters? Otherwise, specifying an account name means basic authentication. If no account name or switch, it's anonymous?

I suppose the same can be said for a gMSA when trying to specify a password for a service account. One account name for simplicity, but one of two parameters for the password - password or gMSA switch?

Use a switch only when the corresponding property(s) is not to be specified or must have a specific value? -gMSA -> no password. -UseDefaultCredential -> UseDefaultCredential is $true plus no account or password. -Anonymous -> UseDefaultCredential is $false plus no account or password. -Basic -> UseDefaultCredential is $false plus account and password required.

Perhaps this is too much business logic for a dsc resource....

[johlju]

For example, I think "Use default credential" plus an account name and password is confusing. It seems like a UseDefaultCredential switch would be clearer. If the switch is specified, then the account name and password are not allowed parameters? Otherwise, specifying an account name means basic authentication. If no account name or switch, it's anonymous?

Sounds good to me.

I suppose the same can be said for a gMSA when trying to specify a password for a service account. One account name for simplicity, but one of two parameters for the password - password or gMSA switch?

We have a design pattern for this here: https://github.com/dsccommunity/SqlServerDsc/blob/main/CONTRIBUTING.md#credentials-that-does-not-have-password

[johlju]

When designing a dsc resource, should parameters match those use in the SMO?

Not necessarily.

[johlju]

Reviewed 1 of 5 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @RandyInMarin)

---

source/DSCResources/DSC_SqlDatabaseMail/DSC_SqlDatabaseMail.schema.mof line 16 at r2 (raw file):

    [Write, Description("The logging level that the _Database Mail_ will use. If not specified the default logging level is `'Extended'`."), ValueMap{"Normal","Extended","Verbose"}, Values{"Normal","Extended","Verbose"}] String LoggingLevel;
    [Write, Description("The TCP port used for communication. Default value is port `25`.")] UInt16 TcpPort;
    [Write, Description("Specifies if the DatabaseEngine credentials are used for SMTP server authentication. The default is `$false`.")] Boolean UseDefaultCredentials;

Should no longer say "The default is $false."

[johlju]

Just a tiny comment left, then I think this is ready to merge.

[ghost]

Just a tiny comment left, then I think this is ready to merge.

@johlju Made the tiny change.

[johlju]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @RandyInMarin)

[johlju]

@RandyInMarin great work on this! 🙂