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

Export-DbaUser - Refactor T-SQL User-Role Scripting to Eliminate Dependencies #9232

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

Conversation

0x7FFFFFFFFFFFFFFF
Copy link
Contributor

Refactored the T-SQL scripting for user-role assignments to enforce independence between scripts. This change removes the need to track which roles have been scripted, preventing dependency issues when scripts are run out of order. Below is a detailed introduction to the issue and the solution.

In this new approach of scripting, we do not maintain a variable to track the roles that have been scripted. Our method involves a consistent verification process for each user against the complete list of roles. This ensures that we dynamically include only the roles to which a user belongs. For example, consider two users: user1 is associated with role1 and role2, while user2 is associated with role1 and role3.

Attempting to memorize the scripted roles could result in Transact-SQL (T-SQL) statements such as:

IF NOT EXISTS (role1)
  CREATE ROLE role1
IF NOT EXISTS (role2)
  CREATE ROLE role2
IF NOT EXISTS (user1)
  CREATE USER user1
ADD user1 TO role1
ADD user1 TO role2

-- And for another user:

IF NOT EXISTS (role3)
  CREATE ROLE role3
IF NOT EXISTS (user2)
  CREATE USER user2
ADD user2 TO role1
ADD user2 TO role3

`

However, this script inadvertently introduces a dependency issue. To ensure user2 is properly configured, the script segment for user1 must be executed first due to the shared role1. To circumvent this issue and remove interdependencies, we opt to match each user against all potential roles. Consequently, roles are scripted per user membership, resulting in T-SQL like:

IF NOT EXISTS (role1)
  CREATE ROLE role1
IF NOT EXISTS (role2)
  CREATE ROLE role2
IF NOT EXISTS (user1)
  CREATE USER user1
ADD user1 TO role1
ADD user1 TO role2

-- And for another user:

IF NOT EXISTS (role1)
  CREATE ROLE role1
IF NOT EXISTS (role3)
  CREATE ROLE role3
IF NOT EXISTS (user2)
  CREATE USER user2
ADD user2 TO role1
ADD user2 TO role3

While this method may produce some redundant code (e.g., checking and creating role1 twice), it guarantees that each portion of the script is self-sufficient and can be executed independently of others. Therefore, users can selectively execute any segment of the script without concern for execution order or dependencies.

Please read -- recent changes to our repo

On November 10, 2022, we removed some bloat from our repository (for the second and final time). This change requires that all contributors reclone or refork their repo.

PRs from repos that have not been recently reforked or recloned will be closed and @potatoqualitee will cherry-pick your commits and open a new PR with your changes.

  • Please confirm you have the smaller repo (85MB .git directory vs 275MB or 110MB or 185MB .git directory)

Type of Change

  • Bug fix (non-breaking change, fixes Export-DbaUser includes unrelated database roles in export #9231 )
  • New feature (non-breaking change, adds functionality, fixes # )
  • Breaking change (affects multiple commands or functionality, fixes # )
  • Ran manual Pester test and has passed (.\tests\manual.pester.ps1)
  • Adding code coverage to existing functionality
  • Pester test is included
  • If new file reference added for test, has is been added to github.com/dataplat/appveyor-lab ?
  • Unit test is included
  • Documentation
  • Build system

Purpose

Approach

Commands to test

Screenshots

Learning

Refactored the T-SQL scripting for user-role assignments to enforce independence between scripts.
This change removes the need to track which roles have been scripted, preventing dependency issues when scripts are
run out of order. Below is a detailed introduction to the issue and the solution.

In this new approach of scripting, we do not maintain a variable to track the roles that have been scripted. Our
method involves a consistent verification process for each user against the complete list of roles. This ensures that
we dynamically include only the roles to which a user belongs. For example, consider two users: user1 is associated
with role1 and role2, while user2 is associated with role1 and role3.

Attempting to memorize the scripted roles could result in Transact-SQL (T-SQL) statements such as:

```
IF NOT EXISTS (role1)
  CREATE ROLE role1
IF NOT EXISTS (role2)
  CREATE ROLE role2
IF NOT EXISTS (user1)
  CREATE USER user1
ADD user1 TO role1
ADD user1 TO role2

-- And for another user:

IF NOT EXISTS (role3)
  CREATE ROLE role3
IF NOT EXISTS (user2)
  CREATE USER user2
ADD user2 TO role1
ADD user2 TO role3
```
`

However, this script inadvertently introduces a dependency issue. To ensure user2 is properly configured, the script
segment for user1 must be executed first due to the shared role1. To circumvent this issue and remove interdependencies,
we opt to match each user against all potential roles. Consequently, roles are scripted per user membership, resulting
in T-SQL like:

```
IF NOT EXISTS (role1)
  CREATE ROLE role1
IF NOT EXISTS (role2)
  CREATE ROLE role2
IF NOT EXISTS (user1)
  CREATE USER user1
ADD user1 TO role1
ADD user1 TO role2

-- And for another user:

IF NOT EXISTS (role1)
  CREATE ROLE role1
IF NOT EXISTS (role3)
  CREATE ROLE role3
IF NOT EXISTS (user2)
  CREATE USER user2
ADD user2 TO role1
ADD user2 TO role3
```

While this method may produce some redundant code (e.g., checking and creating role1 twice), it guarantees that each
portion of the script is self-sufficient and can be executed independently of others. Therefore, users can selectively
execute any segment of the script without concern for execution order or dependencies.
@niphlod
Copy link
Contributor

niphlod commented Jan 29, 2024

hum, the single exported file before had issues ?
if not, is this just a reorder of the statements (and an enlargement if counting lines and statements) to make sure that you can execute not only the exported files, but also pieces of them ?

@0x7FFFFFFFFFFFFFFF
Copy link
Contributor Author

hum, the single exported file before had issues ? if not, is this just a reorder of the statements (and an enlargement if counting lines and statements) to make sure that you can execute not only the exported files, but also pieces of them ?

The issue with the single exported file including roles not pertinent to the user. The pull request has made two justments:

  1. Exclude any roles that are unrelated to the user.
  2. It now generates all the roles a user is a member of within their individual script. This means that for each user, the generated script will include the necessary statements to create roles, create the user, and add the user to their respective roles. This negates the need to reference role creation statements from other users' scripts, as each user's script is self-contained and fully executable on its own.

@niphlod
Copy link
Contributor

niphlod commented Jan 30, 2024

okay, then maybe it's good to put a regression test into tests/Export-DbaUser.Tests.ps1 to avoid this "way" getting reverted by a next edit.

@wsmelton
Copy link
Member

wsmelton commented Feb 1, 2024

I'd agree, this seems to be adding more T-SQL code to something that could be done via PowerShell methods in less code (possibly).

A preference for me would also be to put this logic in a Get function having it output the expected results as PS objects. Then all Export does is call that and output it to a T-SQL file. Logic shouldn't have to have exist in an Export function by most standards in PowerShell...it should only export.

@wsmelton wsmelton changed the title Refactor User-Role Scripting to Eliminate Dependencies Export-DbaUser - Refactor T-SQL User-Role Scripting to Eliminate Dependencies Feb 1, 2024
@0x7FFFFFFFFFFFFFFF
Copy link
Contributor Author

  • Exclude any roles that are unrelated to the user.
  • It now generates all the roles a user is a member of within their individual script. This means that for each user, the generated script will include the necessary statements to create roles, create the user, and add the user to their respective roles. This negates the need to reference role creation statements from other users' scripts, as each user's script is self-contained and fully executable on its own.

Can we change it like this:

  1. Exclude any roles that are unrelated to the user. So that the exported script is more clean and relevant.
  2. Maybe we can add a switch parameter, something like -NoRoleDependency. When the user specified this parameter, we export in a no dependency way (so that there will be some duplicate CREATE ROLE statements). If not, keep current logic.

@potatoqualitee
Copy link
Member

Thank you all for y our feedback. I will merge once the PR is approved.

@potatoqualitee
Copy link
Member

Any updates?

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

Successfully merging this pull request may close these issues.

Export-DbaUser includes unrelated database roles in export
4 participants