SqlServerDatabaseMail: Added Authentication configuration #1374
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1374 +/- ##
=====================================
Coverage 98% 98%
=====================================
Files 36 36
Lines 5359 5515 +156
=====================================
+ Hits 5253 5406 +153
- Misses 106 109 +3 |
|
@t3mi Awesome work on this, I will review in a day or so. |
There was a problem hiding this comment.
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.
|
@t3mi Impressive work, clean coding! 😃 |
johlju
added
waiting for code fix
|
@johlju thanks for reviewing this mess :) I'll try to fix in a day or so. |
|
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? |
|
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. |
|
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. 😄 |
|
As-is = after the review comments been resolved I mean :) |
There was a problem hiding this comment.
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-SqlPSCredentialIn 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 = $trueCan we create new examples instead? Maybe
x-EnableDatabaseMailWithSslandx-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-ClauseThis 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=1374As 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 basedSame 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 'ExecuteWe 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 -VerifiableThis 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 'GetWe 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 'GetWe 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 -VerifiableSee a previous comment
Done.
Tests/Unit/SqlServerDsc.Common.Tests.ps1, line 3377 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Context 'GetWe 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.
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. |
I meant for other contributors calling for a DAC connection in the future. That there are a |
johlju
added
needs review
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 MailMaybe 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 MailMaybe 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.
There was a problem hiding this comment.
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
added
needs investigation
There was a problem hiding this comment.
@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
added
waiting for code fix
|
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. |
|
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. |
|
So, how does this get fixed? |
|
Suggest opening a new PR to continue to work. |
I only need one more value - UseDefaultCredentials. Abandon the work already done and just address that? |
|
Would like to see public commands that handle MailAccount (e.g. Get-/Add-/Remove-/Set-SqlDscMailAccount) and mailserver (e.g.g Get-/Add-/Remove-/Set-SqlDscMailServer) where the later commands takes the output from |
|
I have forked the repo and made the required updates to DSC_SqlDatabaseMail and the unit tests for UseDefaultCredentials. Running the unit tests now.... (The unit tests work and pointed out problems... fixing my errors now.) First fork. First module build. First use of unit tests. I'm a bit amazed I made it this far. Looking forward to my first pull request... exciting. |
|
Fixing a unit test is a lot harder than updating the resource. |
|
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
bot
added
abandoned
|
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
bot
added
abandoned
|
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
bot
added
abandoned
|
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
bot
added
abandoned
|
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
bot
added
abandoned
|
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
bot
added
abandoned
Pull Request (PR) description
Changes to SqlServerDatabaseMail:
EnableSslwhich controls encryption of communication using Secure Sockets Layer.AuthenticationandSMTPAccountfor configuration of SMTP authentication mode and credential used.Get-MailServerCredentialIdwhich gets credential Id used by mail server.Get-ServiceMasterKeywhich gets unencrypted Service Master Key for specified SQL Instance.Get-SqlPSCredentialwhich decrypts and returns PSCredential object of SQL Credential by Id.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
Entry should say what was changed, and how that affects users (if applicable).
and comment-based help.
This change is