-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[AC-1682] Flexible collections: data migrations for deprecated permissions #3437
[AC-1682] Flexible collections: data migrations for deprecated permissions #3437
Conversation
… = 1 for all users with Manager role or 'EditAssignedCollections' permission
Fixed Issues
|
EF migrations are missing at this moment, I will add them soon. |
Data migrations should be in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
For visibility - I'm working through your other PRs first which I think are higher priority, so I probably won't get to this until next week. |
Started looking at this today, but would like a second review from another Flexible Collections expert. |
FROM [dbo].[CollectionGroup] CG | ||
INNER JOIN [dbo].[Collection] C ON CG.[CollectionId] = C.[Id] | ||
INNER JOIN #TempGroup TG ON CG.[GroupId] = TG.[GroupId] | ||
WHERE C.[OrganizationId] = TG.[OrganizationId]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure, but is this WHERE
clause doing anything?
The INNER JOIN
ensures that records exist in both tables, so if we INNER JOIN
CollectionGroup
, Collection
and TempGroup
, we should only end up with CollectionGroup
records for Groups where AccessAll = 1
. Then we can update the values without reference to the orgId. I think.
That said, I'm happy to leave it if that's safer or if there's another reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, it also seems redundant to me, but perhaps its for performance reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No performance gain with where clause, if does not add anything then it can be removed
util/Migrator/DbScripts_transition/2023-11-10_00_AccessAllCollectionGroups.sql
Outdated
Show resolved
Hide resolved
util/Migrator/DbScripts_transition/2023-11-10_00_AccessAllCollectionUsers.sql
Outdated
Show resolved
Hide resolved
util/Migrator/DbScripts_transition/2023-11-10_00_AccessAllCollectionGroups.sql
Outdated
Show resolved
Hide resolved
util/Migrator/DbScripts_transition/2023-11-10_00_AccessAllCollectionUsers.sql
Outdated
Show resolved
Hide resolved
util/Migrator/DbScripts_transition/2023-11-10_00_AccessAllCollectionGroups.sql
Outdated
Show resolved
Hide resolved
util/Migrator/DbScripts_transition/2023-11-10_00_AccessAllCollectionUsers.sql
Outdated
Show resolved
Hide resolved
util/Migrator/DbScripts_transition/2023-11-10_00_ManagersEditAssignedCollectionUsers.sql
Outdated
Show resolved
Hide resolved
…lection assignments Co-authored-by: Thomas Rittson <[email protected]>
…signedCollectionUsers sql script
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on this.
@rkac-bw could you please check the latest changes in response to my feedback. Thanks
util/Migrator/DbScripts_transition/2023-12-06_02_ManagersEditAssignedCollectionUsers.sql
Outdated
Show resolved
Hide resolved
…EditAssignedCollections permission assigned to groups with collection access
…ion dates at the same time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this has been a lot of work and revisions, thanks for putting the time in to be the expert on these changes. I have a few questions below.
src/Sql/dbo/Stored Procedures/Organization_EnableCollectionEnhancements.sql
Outdated
Show resolved
Hide resolved
src/Sql/dbo/Stored Procedures/Organization_EnableCollectionEnhancements.sql
Outdated
Show resolved
Hide resolved
-- Step 2 | ||
-- Update existing rows in [dbo].[CollectionUser] | ||
UPDATE target | ||
SET | ||
target.[ReadOnly] = 0, | ||
target.[HidePasswords] = 0, | ||
target.[Manage] = 0 | ||
FROM [dbo].[CollectionUser] AS target | ||
INNER JOIN ( | ||
SELECT C.[Id] AS [CollectionId], T.[OrganizationUserId] | ||
FROM [dbo].[Collection] C | ||
INNER JOIN #TempStep2 T ON C.[OrganizationId] = T.[OrganizationId] | ||
) AS source | ||
ON target.[CollectionId] = source.[CollectionId] AND target.[OrganizationUserId] = source.[OrganizationUserId]; | ||
|
||
-- Insert new rows into [dbo].[CollectionUser] | ||
INSERT INTO [dbo].[CollectionUser] ([CollectionId], [OrganizationUserId], [ReadOnly], [HidePasswords], [Manage]) | ||
SELECT source.[CollectionId], source.[OrganizationUserId], 0, 0, 0 | ||
FROM ( | ||
SELECT C.[Id] AS [CollectionId], T.[OrganizationUserId] | ||
FROM [dbo].[Collection] C | ||
INNER JOIN #TempStep2 T ON C.[OrganizationId] = T.[OrganizationId] | ||
) AS source | ||
LEFT JOIN [dbo].[CollectionUser] AS target | ||
ON target.[CollectionId] = source.[CollectionId] AND target.[OrganizationUserId] = source.[OrganizationUserId] | ||
WHERE target.[CollectionId] IS NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 Why doesn't this follow the same pattern/structure as Step 1? I thought it would be pretty similar because it's the same thing but for orgUsers instead of Groups. However, this uses target/source naming and has nested SELECT queries. Is there some difference here that I'm missing or is this just a result of removing the batching logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense and its easier to read. I have rewritten those parts here, what do you think?
UPDATE target
SET
target.[ReadOnly] = 0,
target.[HidePasswords] = 0,
target.[Manage] = 0
FROM [dbo].[CollectionUser] AS target
INNER JOIN [dbo].[Collection] AS C ON target.[CollectionId] = C.[Id]
INNER JOIN #TempUsersAccessAll AS TU ON C.[OrganizationId] = TU.[OrganizationId] AND target.[OrganizationUserId] = TU.[OrganizationUserId];
INSERT INTO [dbo].[CollectionUser] ([CollectionId], [OrganizationUserId], [ReadOnly], [HidePasswords], [Manage])
SELECT C.[Id] AS [CollectionId], TU.[OrganizationUserId], 0, 0, 0
FROM [dbo].[Collection] C
INNER JOIN #TempUsersAccessAll TU ON C.[OrganizationId] = TU.[OrganizationId]
LEFT JOIN [dbo].[CollectionUser] target
ON target.[CollectionId] = C.[Id] AND target.[OrganizationUserId] = TU.[OrganizationUserId]
WHERE target.[CollectionId] IS NULL;
@rkac-bw will perform some testing to see if there isn't any performance loss.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
src/Sql/dbo/Stored Procedures/Organization_EnableCollectionEnhancements.sql
Show resolved
Hide resolved
…ancements.sql Co-authored-by: Thomas Rittson <[email protected]>
https://github.com/bitwarden/server into ac/ac-1682/data-migrations-for-deprecated-permissions
…Only] = 0 and [HidePasswords] = 0
…th [ReadOnly] = 0 and [HidePasswords] = 0" This reverts commit 086c88f.
src/Sql/dbo/Stored Procedures/Organization_EnableCollectionEnhancements.sql
Show resolved
Hide resolved
src/Sql/dbo/Stored Procedures/Organization_EnableCollectionEnhancements.sql
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me
Type of change
Objective
With Flexible Collections, the Manager role and 'EditAssignedCollections' permission are deprecated. Therefore, we must explicitly grant access to those users for the collections they previously had access to, but now with the new Manage permission. Additionally, users and groups with the AccessAll permission will now have access to all collections within their organizations.
Code changes
Before you submit
dotnet format --verify-no-changes
) (required)