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

0x7FFFFFFFFFFFFFFF commented 5 months ago

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.

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

Type of Change

[x] Bug fix (non-breaking change, fixes #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

niphlod commented 5 months ago

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 commented 5 months ago

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:

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.

niphlod commented 5 months ago

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 commented 5 months ago

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.

0x7FFFFFFFFFFFFFFF commented 5 months ago

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:

Exclude any roles that are unrelated to the user. So that the exported script is more clean and relevant.
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 commented 4 months ago

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

potatoqualitee commented 3 months ago

Any updates?

potatoqualitee commented 1 month ago

@niphlod @wsmelton can i merge? I do not have an opinion on this particular topic other than it seems useful.

Oh but also, if the work is already done in T-SQL then we don't need it to move to PS? That'd be more of a preference than a requiement.

niphlod commented 1 month ago

I don't have any preference in the matter of "PS vs T-SQL" but I'd add a regr test to make sure this behaviour doesn't get reverted at a later stage.

potatoqualitee commented 1 month ago

Thank you -- @0x7FFFFFFFFFFFFFFF is that something you can do? add in a test that would fail if your PR's functionality gets impacted?

0x7FFFFFFFFFFFFFFF commented 1 month ago

Thanks for reviewing the pull request and for the suggestions. I'm pretty busy recently, which has impacted my ability to respond promptly. I appreciate your understanding and patience.

Regarding the test for the Export-DbaUser refactoring, I must admit that I am not very familiar with it. I'll give it a try and will reach out if I need specific guidance or feedback.

niphlod commented 1 month ago

we can definitely help, just need to prop up two user with roles exposing the behaviour you want to fix and then make sure everything stays in the same way "forever" ;-)

0x7FFFFFFFFFFFFFFF commented 1 month ago

I've added a test.

potatoqualitee commented 2 weeks ago

Thank you very much! Apologies for the delay. Merging finally 🙏🏼

dataplat / dbatools