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

Set-SqlDscServerPermission: Added support for assigning permissions to a server role #2061

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

IAMJDA
Copy link

@IAMJDA IAMJDA commented Feb 23, 2025

Pull Request (PR) description

Updated Set-SqlDscServerPermission to be able to set permission on server roles. Adapted unit tests so they should continue to work but missing support for the new logic.

This Pull Request (PR) fixes the following issues

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 Reviewable

@IAMJDA IAMJDA requested review from johlju and a team as code owners February 23, 2025 16:48
…ts for new logic bug haven't added explicit tests for roles
Copy link

codecov bot commented Feb 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93%. Comparing base (85ea1ed) to head (57aecc3).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff           @@
##           main   #2061     +/-   ##
======================================
- Coverage    94%     93%     -1%     
======================================
  Files        94      36     -58     
  Lines      7930    6341   -1589     
======================================
- Hits       7500    5953   -1547     
+ Misses      430     388     -42     
Flag Coverage Δ
unit 93% <ø> (-1%) ⬇️

see 58 files with indirect coverage changes

@johlju johlju added the needs review The pull request needs a code review. label Feb 28, 2025
@johlju johlju changed the title SqlServerPermission: Added support for assigning permissions to a server role. Set-SqlServerPermission: Added support for assigning permissions to a server role Mar 1, 2025
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 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @IAMJDA)


source/Classes/020.SqlPermission.ps1 line 373 at r1 (raw file):

            New-InvalidOperationException -Message $missingPrincipalMessage
        }

We should not need this check at all since it is already evaluated in the Set-SqlDscServerPermission command. I rather see this change for the resource SqlPermission in a separate PR as we also need integration tests. Please revert the changes for this file and move it to a separate PR. 🙂

Code quote:

        $testSqlDscIsPrincipalParameters = @{
            ServerObject      = $serverObject
            Name              = $this.Name
        }

        # This will test wether the principal exist.
        $isLogin = Test-SqlDscIsLogin @testSqlDscIsPrincipalParameters
        $isRole = Test-SqlDscIsRole @testSqlDscIsPrincipalParameters

        if (-not $isLogin -and -not $isRole)
        {
            $missingPrincipalMessage = $this.localizedData.NameIsMissing -f @(
                $this.Name,
                $this.InstanceName
            )

            New-InvalidOperationException -Message $missingPrincipalMessage
        }

source/Public/Set-SqlDscServerPermission.ps1 line 107 at r1 (raw file):

        if (-not $isLogin -and -not $isRole)
        {
            $missingPrincipalMessage = $script:localizedData.ServerPermission_MissingPrincipal -f $Name, $ServerObject.InstanceName

We need to update the localized string

ServerPermission_MissingPrincipal = The principal '{0}' is not a login on the instance '{1}'.

Code quote:

ServerPermission_MissingPrincipal

source/Public/Set-SqlDscServerPermission.ps1 line 117 at r1 (raw file):

                )
            )
            return

We should not need return here as we throw a terminating error?

Code quote:

return

tests/Unit/Public/Set-SqlDscServerPermission.Tests.ps1 line 430 at r1 (raw file):

                Mock -CommandName Test-SqlDscIsRole -MockWith {
                    return $false
                }

We should add a test that actually test the it is a role and not a login.

Code quote:

                Mock -CommandName Test-SqlDscIsRole -MockWith {
                    return $false
                }

@johlju johlju changed the title Set-SqlServerPermission: Added support for assigning permissions to a server role Set-SqlDscServerPermission: Added support for assigning permissions to a server role Mar 1, 2025
@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 Mar 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set-SqlDscServerPermission: Not possible to set server permissions for a server role
2 participants