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

SqlServerDatabaseMail: Added Authentication configuration #1374

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

t3mi
Copy link

@t3mi t3mi commented Jun 6, 2019

Pull Request (PR) description

Changes to SqlServerDatabaseMail:

  • Added new parameter EnableSsl which controls encryption of communication using Secure Sockets Layer.
  • Added new parameter Authentication and SMTPAccount for configuration of SMTP authentication mode and credential used.
  • Added new helper function Get-MailServerCredentialId which gets credential Id used by mail server.
  • Added new helper function Get-ServiceMasterKey which gets unencrypted Service Master Key for specified SQL Instance.
  • Added new helper function Get-SqlPSCredential which decrypts and returns PSCredential object of SQL Credential by Id.
  • Added DAC switch to function 'Connect-SQL' with default value $false. Used to specify that non-pooled Dedicated Admin Connection should be established.
  • Added UsingDAC switch to function 'Invoke-Query' with default value $false. Used to specify that query should be executed using Dedicated Admin Connection. After execution DAC connection will be closed.
  • Added PSCredential option to function 'Test-DscParameterState' which will compare PSCredential objects using username/password keys.

Submission containing materials of a third party: Antti Rantasaari 2014, NetSPI
License: BSD 3-Clause https://opensource.org/licenses/BSD-3-Clause
Source: https://github.com/NetSPI/Powershell-Modules/blob/master/Get-MSSQLCredentialPasswords.psm1

This Pull Request (PR) fixes the following issues

Fixes issue #1215

Task list

  • Added an entry under the Unreleased section of the change log in the CHANGELOG.md.
    Entry should say what was changed, and how that affects users (if applicable).
  • Resource documentation added/updated in README.md.
  • Resource parameter descriptions added/updated in README.md, schema.mof
    and comment-based help.
  • Comment-based help added/updated.
  • Localization strings added/updated in all localization files as appropriate.
  • Examples appropriately added/updated.
  • Unit tests added/updated. See DSC Resource Testing Guidelines.
  • Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
  • New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

This change is Reviewable

@codecov-io
Copy link

codecov-io commented Jun 6, 2019

Codecov Report

Attention: Patch coverage is 95.67901% with 7 lines in your changes missing coverage. Please review.

Project coverage is 98%. Comparing base (728a1cf) to head (267b931).
Report is 336 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##           main   #1374    +/-   ##
=====================================
  Coverage    98%     98%            
=====================================
  Files        36      36            
  Lines      5359    5515   +156     
=====================================
+ Hits       5253    5406   +153     
- Misses      106     109     +3     

@johlju johlju added the needs review The pull request needs a code review. label Jun 8, 2019
@johlju
Copy link
Member

johlju commented Jun 8, 2019

@t3mi Awesome work on this, I will review in a day or so.

Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 9 of 13 files at r1, 4 of 4 files at r2.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @t3mi)


DSCResources/MSFT_SqlServerDatabaseMail/MSFT_SqlServerDatabaseMail.psm1, line 170 at r2 (raw file):

$returnValue['SMTPAccount'] = Get-SqlPSCredential

In the integration tests this are tested to -BeNullOrEmpty, so is there a point of returning this? Why does it return nothing in the integration tests. I guess this some sort of protection (by LCM) for not returning the actual password? 🤔


DSCResources/MSFT_SqlServerDatabaseMail/MSFT_SqlServerDatabaseMail.psm1, line 445 at r2 (raw file):

switch($Authentication)

Space after the switch-statement.


DSCResources/MSFT_SqlServerDatabaseMail/MSFT_SqlServerDatabaseMail.psm1, line 608 at r2 (raw file):

switch(

Space after the switch-statement.


DSCResources/MSFT_SqlServerDatabaseMail/MSFT_SqlServerDatabaseMail.psm1, line 611 at r2 (raw file):

                            'Windows' { $true }
                            Default   { $false }

We should have the open brace and closing brace on separate rows.


DSCResources/MSFT_SqlServerDatabaseMail/MSFT_SqlServerDatabaseMail.psm1, line 648 at r2 (raw file):

Quoted 4 lines of code…
                            <#
                                Message will not include real password values unless password is not set.
                                This was done on purpose.
                            #>

What was you thoughts around this? Should we should output the password at all, isn't better to just say that the password mismatched?


Examples/Resources/SqlServerDatabaseMail/1-EnableDatabaseMail.ps1, line 21 at r2 (raw file):

EnableSsl      = $true

Can we create new examples instead? Maybe x-EnableDatabaseMailWithSsl and x-EnableDatabaseMailWithAuthentication?
It would make it easier for users to see the different options?


Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1, line 494 at r2 (raw file):

Quoted 4 lines of code…
                                    <#
                                        Message will not include real password values unless password is not set.
                                        This was done on purpose.
                                    #>

Same question as above. What was your thoughts about writing out the passwords?


Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1, line 2460 at r2 (raw file):

License: BSD 3-Clause http://opensource.org/licenses/BSD-3-Clause

This license will clash with this projects MIT license.

Please re-read the CLA, especially under the section "Originality of Work"
https://cla.opensource.microsoft.com/PowerShell/SqlServerDsc?pullRequest=1374

As I see it we must either add the correct notices, or if possible rewrite this code as your own. It is okay to be inspired by other sources, but it would be much easier if the licenses do not clash.


Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1, line 2520 at r2 (raw file):

This function is partialy based

Same as previous comment.


Modules/SqlServerDsc.Common/en-US/SqlServerDsc.Common.strings.psd1, line 57 at r2 (raw file):

DisconnectFromSQLInstance = Disconnected from '{0}' instance. (SQLCOMMON0054)

Maybe the text could reference the admin connection is disconnected? Then the string key could be changed as well so it should be used (not disconnecting any connections).


Tests/Unit/SqlServerDsc.Common.Tests.ps1, line 1567 at r2 (raw file):

Context 'Execute

We should start context-blocks with 'When...'


Tests/Unit/SqlServerDsc.Common.Tests.ps1, line 3194 at r2 (raw file):

            $mockThrowLocalizedMessage = {
                throw $Message
            }

See next review comment


Tests/Unit/SqlServerDsc.Common.Tests.ps1, line 3211 at r2 (raw file):

Mock -CommandName New-InvalidOperationException -MockWith $mockThrowLocalizedMessage -Verifiable

This mock should not be needed to test the correct error messages? I don't need to mock these functions to assert the error message the same way that is done here.


Tests/Unit/SqlServerDsc.Common.Tests.ps1, line 3214 at r2 (raw file):

Context 'Get

We should start context-blocks with 'When...'


Tests/Unit/SqlServerDsc.Common.Tests.ps1, line 3291 at r2 (raw file):

Context 'Get

We should start context-blocks with 'When...'


Tests/Unit/SqlServerDsc.Common.Tests.ps1, line 3354 at r2 (raw file):

            $mockThrowLocalizedMessage = {
                throw $Message
            }

See a previous comment


Tests/Unit/SqlServerDsc.Common.Tests.ps1, line 3372 at r2 (raw file):

Mock -CommandName New-InvalidOperationException -MockWith $mockThrowLocalizedMessage -Verifiable

See a previous comment


Tests/Unit/SqlServerDsc.Common.Tests.ps1, line 3377 at r2 (raw file):

Context 'Get

We should start context-blocks with 'When...'


Tests/Unit/SqlServerDsc.Common.Tests.ps1, line 3385 at r2 (raw file):

Quoted 15 lines of code…
                $mockSMK                = [System.Text.Encoding]::UTF8.GetBytes('ABCDEFGHIJKLMNOP')
                $cryptoProvider         = New-Object -TypeName System.Security.Cryptography.TripleDESCryptoServiceProvider
                $cryptoProvider.Padding = 'PKCS7'
                $cryptoProvider.Mode    = 'CBC'
                $cryptoProvider.Key     = $mockSMK
                $mockIV                 = $cryptoProvider.IV
                $encryptor              = $cryptoProvider.CreateEncryptor()
                $memoryStream           = New-Object System.IO.MemoryStream
                $cryptoStream           = New-Object System.Security.Cryptography.CryptoStream ($memoryStream, $encryptor, [System.Security.Cryptography.CryptoStreamMode]::Write)
                $innerMessage           = [byte[]]('0x0D','0xF0', '0xAD', '0xBA') + [BitConverter]::GetBytes([int16]0) + [BitConverter]::GetBytes([int16]$mockUnicodePassword.Length) + $mockUnicodePassword
                $cryptoStream.Write($innerMessage, 0, $innerMessage.Length)
                $cryptoStream.Close()
                $memoryStream.Close()
                $cryptoProvider.Clear()
                $mockEnc_message = $memoryStream.ToArray()

What is all this doing? It can't be used in the mock, since the mock has already been added prior to this It-block?


Tests/Unit/SqlServerDsc.Common.Tests.ps1, line 3411 at r2 (raw file):

Quoted 15 lines of code…
                $mockSMK                = [System.Text.Encoding]::UTF8.GetBytes('ABCDEFGHIJKLMNOPQRSTUVWXYZ012345')
                $cryptoProvider         = New-Object System.Security.Cryptography.AESCryptoServiceProvider
                $cryptoProvider.Padding = 'PKCS7'
                $cryptoProvider.Mode    = 'CBC'
                $cryptoProvider.Key     = $mockSMK
                $mockIV                 = $cryptoProvider.IV
                $encryptor              = $cryptoProvider.CreateEncryptor()
                $memoryStream           = New-Object System.IO.MemoryStream
                $cryptoStream           = New-Object System.Security.Cryptography.CryptoStream ($memoryStream, $encryptor, [System.Security.Cryptography.CryptoStreamMode]::Write)
                $innerMessage           = [byte[]]('0x0D','0xF0', '0xAD', '0xBA') + [BitConverter]::GetBytes([int16]0) + [BitConverter]::GetBytes([int16]$mockUnicodePassword.Length) + $mockUnicodePassword
                $cryptoStream.Write($innerMessage, 0, $innerMessage.Length)
                $cryptoStream.Close()
                $memoryStream.Close()
                $cryptoProvider.Clear()
                $mockEnc_message = $memoryStream.ToArray()

Same question as the previous comment.

@johlju
Copy link
Member

johlju commented Jun 9, 2019

@t3mi Impressive work, clean coding! 😃

@johlju johlju added waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. and removed needs review The pull request needs a code review. labels Jun 9, 2019
@t3mi
Copy link
Author

t3mi commented Jun 9, 2019

@johlju thanks for reviewing this mess :) I'll try to fix in a day or so.

@johlju
Copy link
Member

johlju commented Jun 10, 2019

I has thoughts about if we actually need to verify and enforce that the password is correct. It’s a lot of code just to be able to read the password, and the need to get an ADMIN: connection to do it.

Could we just skip and enforce the password? What is your thoughts about that?

@t3mi
Copy link
Author

t3mi commented Jun 10, 2019

I would leave the check for password. For my use case we have single username and api tokens as passwords and having the password check is crucial if we face expired/modified password or if it was initially set incorrectly we just put the right password instead of spending time to recreate mail account. I understand that possibility of this is low but anyway it's present and if we know how to implement it in the right way - why not? :) Some other parts of the SQL are also using credentials and with this code it would be easy to have desired functionality implemented in those areas as well.
I don't like admin connection either but it's the only way to obtain encrypted information. And as you checked I'm not using pooled connection and disconnecting right after getting the results so there would not be any idle/sleeping connection left which will block any subsequent admin connections.

@johlju
Copy link
Member

johlju commented Jun 10, 2019

Thank you for that use case. Let’s leave the check as-is then :)

You mention closing the admin-connection, I wonder if we should add that to another helper function so that other contributors close the connection the same way.

It might also be easier for me to remember that using DAC should also use the close-function. 😄

@johlju
Copy link
Member

johlju commented Jun 10, 2019

As-is = after the review comments been resolved I mean :)

Copy link
Author

@t3mi t3mi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 7 of 16 files reviewed, 20 unresolved discussions (waiting on @johlju)


DSCResources/MSFT_SqlServerDatabaseMail/MSFT_SqlServerDatabaseMail.psm1, line 170 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
$returnValue['SMTPAccount'] = Get-SqlPSCredential

In the integration tests this are tested to -BeNullOrEmpty, so is there a point of returning this? Why does it return nothing in the integration tests. I guess this some sort of protection (by LCM) for not returning the actual password? 🤔

I've also thinking into LCM protection way but can't find the proof. Get() needs to return it because it's used in the Test() function to compare desired state with current one.


DSCResources/MSFT_SqlServerDatabaseMail/MSFT_SqlServerDatabaseMail.psm1, line 445 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
switch($Authentication)

Space after the switch-statement.

Done. Sorry for this.


DSCResources/MSFT_SqlServerDatabaseMail/MSFT_SqlServerDatabaseMail.psm1, line 608 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
switch(

Space after the switch-statement.

Done.


DSCResources/MSFT_SqlServerDatabaseMail/MSFT_SqlServerDatabaseMail.psm1, line 611 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
                            'Windows' { $true }
                            Default   { $false }

We should have the open brace and closing brace on separate rows.

Done.


DSCResources/MSFT_SqlServerDatabaseMail/MSFT_SqlServerDatabaseMail.psm1, line 648 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
                            <#
                                Message will not include real password values unless password is not set.
                                This was done on purpose.
                            #>

What was you thoughts around this? Should we should output the password at all, isn't better to just say that the password mismatched?

I just don't want passwords to be present in the logs but wanted to have some output when there is no password (empty string) set and used generic output string except that instead of real password value there is Secure.String type.


Examples/Resources/SqlServerDatabaseMail/1-EnableDatabaseMail.ps1, line 21 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
EnableSsl      = $true

Can we create new examples instead? Maybe x-EnableDatabaseMailWithSsl and x-EnableDatabaseMailWithAuthentication?
It would make it easier for users to see the different options?

Done with references in README as well.


Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1, line 494 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
                                    <#
                                        Message will not include real password values unless password is not set.
                                        This was done on purpose.
                                    #>

Same question as above. What was your thoughts about writing out the passwords?

I just don't want passwords to be present in the logs but wanted to have some output when there is no password (empty string) set and used generic output string except that instead of real password value there is Secure.String type.


Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1, line 2460 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
License: BSD 3-Clause http://opensource.org/licenses/BSD-3-Clause

This license will clash with this projects MIT license.

Please re-read the CLA, especially under the section "Originality of Work"
https://cla.opensource.microsoft.com/PowerShell/SqlServerDsc?pullRequest=1374

As I see it we must either add the correct notices, or if possible rewrite this code as your own. It is okay to be inspired by other sources, but it would be much easier if the licenses do not clash.

I've tried but the logic is the same. Any chance you can verify please if its enough or the notice should be added anyway?


Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1, line 2520 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
This function is partialy based

Same as previous comment.

Removed.


Modules/SqlServerDsc.Common/en-US/SqlServerDsc.Common.strings.psd1, line 57 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
DisconnectFromSQLInstance = Disconnected from '{0}' instance. (SQLCOMMON0054)

Maybe the text could reference the admin connection is disconnected? Then the string key could be changed as well so it should be used (not disconnecting any connections).

I can create a separate string for that but in the output its properly saying that Disconnect was done from the ADMIN session.

VERBOSE: [WIN-G10ASDOAENJ]: [[SqlServerDatabaseMail]EnableDatabaseMail] Connected to SQL instance 'ADMIN:WIN-G10ASDOAENJ'. (SQLCOMMON0018)
VERBOSE: [WIN-G10ASDOAENJ]: [[SqlServerDatabaseMail]EnableDatabaseMail] Disconnected from 'ADMIN:WIN-G10ASDOAENJ' instance. (SQLCOMMON0054)


Tests/Unit/SqlServerDsc.Common.Tests.ps1, line 1567 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
Context 'Execute

We should start context-blocks with 'When...'

Done. Sorry for that and I've fixed some others as well.


Tests/Unit/SqlServerDsc.Common.Tests.ps1, line 3194 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
            $mockThrowLocalizedMessage = {
                throw $Message
            }

See next review comment

Done.


Tests/Unit/SqlServerDsc.Common.Tests.ps1, line 3211 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
Mock -CommandName New-InvalidOperationException -MockWith $mockThrowLocalizedMessage -Verifiable

This mock should not be needed to test the correct error messages? I don't need to mock these functions to assert the error message the same way that is done here.

Correct, didn't checked that. Fixed others as well.


Tests/Unit/SqlServerDsc.Common.Tests.ps1, line 3214 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
Context 'Get

We should start context-blocks with 'When...'

Done.


Tests/Unit/SqlServerDsc.Common.Tests.ps1, line 3291 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
Context 'Get

We should start context-blocks with 'When...'

Done.


Tests/Unit/SqlServerDsc.Common.Tests.ps1, line 3354 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
            $mockThrowLocalizedMessage = {
                throw $Message
            }

See a previous comment

Done.


Tests/Unit/SqlServerDsc.Common.Tests.ps1, line 3372 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
Mock -CommandName New-InvalidOperationException -MockWith $mockThrowLocalizedMessage -Verifiable

See a previous comment

Done.


Tests/Unit/SqlServerDsc.Common.Tests.ps1, line 3377 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
Context 'Get

We should start context-blocks with 'When...'

Done.


Tests/Unit/SqlServerDsc.Common.Tests.ps1, line 3385 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
                $mockSMK                = [System.Text.Encoding]::UTF8.GetBytes('ABCDEFGHIJKLMNOP')
                $cryptoProvider         = New-Object -TypeName System.Security.Cryptography.TripleDESCryptoServiceProvider
                $cryptoProvider.Padding = 'PKCS7'
                $cryptoProvider.Mode    = 'CBC'
                $cryptoProvider.Key     = $mockSMK
                $mockIV                 = $cryptoProvider.IV
                $encryptor              = $cryptoProvider.CreateEncryptor()
                $memoryStream           = New-Object System.IO.MemoryStream
                $cryptoStream           = New-Object System.Security.Cryptography.CryptoStream ($memoryStream, $encryptor, [System.Security.Cryptography.CryptoStreamMode]::Write)
                $innerMessage           = [byte[]]('0x0D','0xF0', '0xAD', '0xBA') + [BitConverter]::GetBytes([int16]0) + [BitConverter]::GetBytes([int16]$mockUnicodePassword.Length) + $mockUnicodePassword
                $cryptoStream.Write($innerMessage, 0, $innerMessage.Length)
                $cryptoStream.Close()
                $memoryStream.Close()
                $cryptoProvider.Clear()
                $mockEnc_message = $memoryStream.ToArray()

What is all this doing? It can't be used in the mock, since the mock has already been added prior to this It-block?

Moved to the mock. This part is an encryption of current credential and preparation of the encrypted message so that function Get-SqlPSCredential() can work with its decryption the same way it works on Sql. The current line generates 128 bit 3DES key which will be used for password encryption.


Tests/Unit/SqlServerDsc.Common.Tests.ps1, line 3411 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
                $mockSMK                = [System.Text.Encoding]::UTF8.GetBytes('ABCDEFGHIJKLMNOPQRSTUVWXYZ012345')
                $cryptoProvider         = New-Object System.Security.Cryptography.AESCryptoServiceProvider
                $cryptoProvider.Padding = 'PKCS7'
                $cryptoProvider.Mode    = 'CBC'
                $cryptoProvider.Key     = $mockSMK
                $mockIV                 = $cryptoProvider.IV
                $encryptor              = $cryptoProvider.CreateEncryptor()
                $memoryStream           = New-Object System.IO.MemoryStream
                $cryptoStream           = New-Object System.Security.Cryptography.CryptoStream ($memoryStream, $encryptor, [System.Security.Cryptography.CryptoStreamMode]::Write)
                $innerMessage           = [byte[]]('0x0D','0xF0', '0xAD', '0xBA') + [BitConverter]::GetBytes([int16]0) + [BitConverter]::GetBytes([int16]$mockUnicodePassword.Length) + $mockUnicodePassword
                $cryptoStream.Write($innerMessage, 0, $innerMessage.Length)
                $cryptoStream.Close()
                $memoryStream.Close()
                $cryptoProvider.Clear()
                $mockEnc_message = $memoryStream.ToArray()

Same question as the previous comment.

The current line generates 256 bit AES key.

@t3mi
Copy link
Author

t3mi commented Jun 11, 2019

You mention closing the admin-connection, I wonder if we should add that to another helper function so that other contributors close the connection the same way.

It won't work the same way in current state. Right now all connections to SQL are done using connection pool so if you run Disconnect(), connection will not be closed right away but will change its status to sleeping until new request comes in. If you want to have disposable connection than it needs to be non-pooled. In that case Disconnect() will close connection immediately. This was done for DAC connection as there is a limitation of only one DAC connection is supported.

@johlju
Copy link
Member

johlju commented Jun 12, 2019

This was done for DAC connection as there is a limitation of only one DAC connection is supported.

I meant for other contributors calling for a DAC connection in the future. That there are a Disconnect-DACConnection (or whatever is a suitable name).
But that can be added in another PR.

@johlju johlju added needs review The pull request needs a code review. and removed waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. labels Jun 12, 2019
Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work! Just tiny things left! 😃

Reviewed 8 of 9 files at r3, 2 of 2 files at r4.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @t3mi)


DSCResources/MSFT_SqlServerDatabaseMail/MSFT_SqlServerDatabaseMail.psm1, line 170 at r2 (raw file):

Previously, t3mi wrote…

I've also thinking into LCM protection way but can't find the proof. Get() needs to return it because it's used in the Test() function to compare desired state with current one.

Could we add comment for that in the tests so the next contributor knows this?


DSCResources/MSFT_SqlServerDatabaseMail/MSFT_SqlServerDatabaseMail.psm1, line 445 at r2 (raw file):

Previously, t3mi wrote…

Done. Sorry for this.

No worries, you did such an amazing job for the rest of the code so don't feel sorry for a few of my nitpick comments 😃


Examples/Resources/SqlServerDatabaseMail/2-EnableDatabaseMailWithSsl.ps1, line 3 at r4 (raw file):

 enable Database Mail

Maybe we should add '...using SSL'?


Examples/Resources/SqlServerDatabaseMail/3-EnableDatabaseMailWithAuthentication.ps1, line 3 at r4 (raw file):

 enable Database Mail

Maybe we should add '...with SMTP authentication'?


Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1, line 2460 at r2 (raw file):

Previously, t3mi wrote…

I've tried but the logic is the same. Any chance you can verify please if its enough or the notice should be added anyway?

It great work rewriting this, but yeah, there are similarities in the logic that looks like it would be difficult to rework, it all depends how you would interpret the license. Also, personally I would feel bad to not give credit where credit is due. Though with the original code and these changes a lot of credit should go to you too. 🙂

I think you have to decide how this complies to "Originality of Work" since you are the author of this PR.

Should you decide to add back a notice, then do it according to the CLA (suggested prefix text) and to the BSD 3 license (copyright notice, and link to license), and a link to the source. I think the source author would appreciate that.


Modules/SqlServerDsc.Common/en-US/SqlServerDsc.Common.strings.psd1, line 57 at r2 (raw file):

Previously, t3mi wrote…

I can create a separate string for that but in the output its properly saying that Disconnect was done from the ADMIN session.

VERBOSE: [WIN-G10ASDOAENJ]: [[SqlServerDatabaseMail]EnableDatabaseMail] Connected to SQL instance 'ADMIN:WIN-G10ASDOAENJ'. (SQLCOMMON0018)
VERBOSE: [WIN-G10ASDOAENJ]: [[SqlServerDatabaseMail]EnableDatabaseMail] Disconnected from 'ADMIN:WIN-G10ASDOAENJ' instance. (SQLCOMMON0054)

Let's leave it as-is.


Tests/Unit/SqlServerDsc.Common.Tests.ps1, line 3385 at r2 (raw file):

Previously, t3mi wrote…

Moved to the mock. This part is an encryption of current credential and preparation of the encrypted message so that function Get-SqlPSCredential() can work with its decryption the same way it works on Sql. The current line generates 128 bit 3DES key which will be used for password encryption.

Could we add that as a comment where applicable? So all we other contributors know/remember this? 🙂


Tests/Unit/SqlServerDsc.Common.Tests.ps1, line 3411 at r2 (raw file):

Previously, t3mi wrote…

The current line generates 256 bit AES key.

Same as previous, add comment?

Copy link
Author

@t3mi t3mi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 9 of 16 files reviewed, 6 unresolved discussions (waiting on @johlju)


DSCResources/MSFT_SqlServerDatabaseMail/MSFT_SqlServerDatabaseMail.psm1, line 170 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we add comment for that in the tests so the next contributor knows this?

Added.


Examples/Resources/SqlServerDatabaseMail/2-EnableDatabaseMailWithSsl.ps1, line 3 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
 enable Database Mail

Maybe we should add '...using SSL'?

Corrected.


Examples/Resources/SqlServerDatabaseMail/3-EnableDatabaseMailWithAuthentication.ps1, line 3 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…
 enable Database Mail

Maybe we should add '...with SMTP authentication'?

Corrected.


Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1, line 2460 at r2 (raw file):

Also, personally I would feel bad to not give credit where credit is due.
Me too :) If it's not OK how I done can you point please to some example as I cannot find any. I've updated description of the PR itself and added notice to the function.


Tests/Unit/SqlServerDsc.Common.Tests.ps1, line 3385 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we add that as a comment where applicable? So all we other contributors know/remember this? 🙂

Added.


Tests/Unit/SqlServerDsc.Common.Tests.ps1, line 3411 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Same as previous, add comment?

Added.

Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 9 of 9 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @t3mi)


Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1, line 2460 at r2 (raw file):

Previously, t3mi wrote…

Also, personally I would feel bad to not give credit where credit is due.
Me too :) If it's not OK how I done can you point please to some example as I cannot find any. I've updated description of the PR itself and added notice to the function.

I looks okay to me, but this is the first time we added code with another license so I need to confirm this with the MS Open Source team so we do not add something that backfires later. 🙂 I will also send a question to Antti and see if it is possible for him to re-release his code with the MIT License (we can still credit him). That would simplify things.

@johlju johlju added needs investigation The issue needs to be investigated by the maintainers or/and the community. and removed needs review The pull request needs a code review. labels Jun 23, 2019
Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@t3mi sorry it took so long, but after this change I will merge this one. Also please rebase since there are merge conflicts.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @t3mi)


Modules/SqlServerDsc.Common/SqlServerDsc.Common.psm1, line 2460 at r5 (raw file):

    .DESCRIPTION
        Submission containing materials of a third party: Antti Rantasaari 2014, NetSPI

So I got feedback from a representative from NetSPI, they are happy as long as we provide the name and source url.
So I suggest we replace the .DESCRIPTION block with the following:

"Inspired by the work of Antti Rantasaari 2014, NetSPI (https://github.com/NetSPI/Powershell-Modules/blob/master/Get-MSSQLCredentialPasswords.psm1) that is under BSD-3 license"

@johlju johlju added waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. and removed needs investigation The issue needs to be investigated by the maintainers or/and the community. labels Jun 30, 2019
Copy link

github-actions bot commented Mar 2, 2024

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

@github-actions github-actions bot added abandoned The pull request has been abandoned. and removed abandoned The pull request has been abandoned. labels Mar 2, 2024
Copy link

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

@github-actions github-actions bot added abandoned The pull request has been abandoned. and removed abandoned The pull request has been abandoned. labels Mar 18, 2024
Copy link

github-actions bot commented Apr 2, 2024

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

@github-actions github-actions bot added abandoned The pull request has been abandoned. and removed abandoned The pull request has been abandoned. labels Apr 2, 2024
Copy link

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

@github-actions github-actions bot added abandoned The pull request has been abandoned. and removed abandoned The pull request has been abandoned. labels Apr 17, 2024
Copy link

github-actions bot commented May 2, 2024

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

@github-actions github-actions bot added abandoned The pull request has been abandoned. and removed abandoned The pull request has been abandoned. labels May 2, 2024
Copy link

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

@github-actions github-actions bot added abandoned The pull request has been abandoned. and removed abandoned The pull request has been abandoned. labels May 18, 2024
Copy link

github-actions bot commented Jun 2, 2024

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

@github-actions github-actions bot added abandoned The pull request has been abandoned. and removed abandoned The pull request has been abandoned. labels Jun 2, 2024
Copy link

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

@github-actions github-actions bot added abandoned The pull request has been abandoned. and removed abandoned The pull request has been abandoned. labels Jun 17, 2024
Copy link

github-actions bot commented Jul 2, 2024

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

@github-actions github-actions bot added the abandoned The pull request has been abandoned. label Jul 2, 2024
@johlju johlju removed the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label Jul 2, 2024
@github-actions github-actions bot removed the abandoned The pull request has been abandoned. label Jul 3, 2024
Copy link

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

@github-actions github-actions bot added the abandoned The pull request has been abandoned. label Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abandoned The pull request has been abandoned.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SqlDatabaseMail: Support for SMTP server credentials
5 participants