PM-25576 Member Access Query - Requested Changes to Use Views in Member Access Query and Avatar Color for Front End #6606
Conversation
|
Claude finished @prograhamming's task —— View job Code Review CompleteI've reviewed the changes in this PR. Here's my analysis: Summary of Changes Since Last ReviewThis PR refactors the member access query to use SQL views and adds the Critical IssuesFinding 1: EDD violations for column rename require multi-release process ❌ The EF migrations (MySQL, Postgres, SQLite) use Problem: Direct column renames break compatibility with running application instances during deployment. Required approach:
Files affected:
Finding 2: Missing corresponding SQL Server migration for column rename ❌ The SQL Server migration script ( Problem: SQL Server deployments will be out of sync with MySQL/Postgres/SQLite deployments. Required: Add SQL to handle the Minor IssuesFinding 3: SQL formatting inconsistencies with code style guide ⛏️ All three new view files have Files affected:
Expected format: FROM
[dbo].[CollectionCipher] CC
INNER JOIN [dbo].[Collection] C ON C.[Id] = CC.[CollectionId]
INNER JOIN [dbo].[Cipher] Ci ON Ci.[Id] = CC.[CipherId]Finding 4: Potential null reference in MemberAccessReportQuery 💭 At Current code: AccountRecoveryEnabled = !string.IsNullOrWhiteSpace(g.Key.ResetPasswordKey) && orgAbility?.UseResetPassword == true,Analysis: The code uses null-conditional operator ( Status: This is a non-issue - the code is safe as written. Good Practices Observed✅ Comprehensive test coverage: Added extensive unit tests in Action Items for Author
Test Coverage Impact📊 Codecov shows 68% patch coverage with 16 lines missing coverage. The uncovered lines are primarily in:
While response models typically don't require extensive test coverage, consider adding basic serialization tests if this is a pattern in your codebase. Note: I've also added inline comments on the specific files and lines where issues were found. The most critical issue is the EDD violation for the column rename, which must be addressed before merging. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| AND NOT EXISTS ( | ||
| SELECT 1 | ||
| FROM [dbo].[CollectionUserPermissionsView] CUP | ||
| WHERE CUP.[OrganizationUserId] = OU.[Id] | ||
| AND CUP.[OrganizationId] = @OrganizationId | ||
| ); |
There was a problem hiding this comment.
❌ Critical Logic Error: This query only checks CollectionUserPermissionsView but doesn't check CollectionGroupPermissionsView.
Problem: Users who have collection access through groups (but not direct user-collection assignments) will incorrectly appear as "users without collection access."
Fix: Add a second NOT EXISTS check:
AND NOT EXISTS (
SELECT 1
FROM [dbo].[CollectionUserPermissionsView] CUP
WHERE CUP.[OrganizationUserId] = OU.[Id]
AND CUP.[OrganizationId] = @OrganizationId
)
AND NOT EXISTS (
SELECT 1
FROM [dbo].[CollectionGroupPermissionsView] CGP
WHERE CGP.[OrganizationUserId] = OU.[Id]
AND CGP.[OrganizationId] = @OrganizationId
)
| @@ -0,0 +1,109 @@ | |||
| CREATE PROCEDURE dbo.MemberAccessReport_GetMemberAccessCipherDetailsByOrganizationId | |||
There was a problem hiding this comment.
MemberAccessDetail_GetMemberAccessCipherDetailsByOrganizationId.sql but the procedure is named MemberAccessReport_GetMemberAccessCipherDetailsByOrganizationId (Detail vs Report).
The file should be renamed to match the procedure name for consistency and easier navigation.
| public Guid? UserGuid { get; set; } | ||
| public string UserName { get; set; } | ||
| public string Email { get; set; } | ||
| public string AvatarColor { get; set; } |
There was a problem hiding this comment.
👍 Good addition of AvatarColor field to support frontend requirements.
| @@ -0,0 +1,15 @@ | |||
| CREATE OR ALTER VIEW [dbo].[CollectionCipherDetailsView] | |||
There was a problem hiding this comment.
🎨 Performance consideration: These views don't have explicit indexes defined. For large organizations, consider testing query performance and adding indexed views if needed. However, SQL Server typically leverages indexes from the underlying tables, so this may not be necessary.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6606 +/- ##
==========================================
+ Coverage 56.83% 56.95% +0.12%
==========================================
Files 1909 1910 +1
Lines 84810 84862 +52
Branches 7633 7636 +3
==========================================
+ Hits 48199 48331 +132
+ Misses 34785 34704 -81
- Partials 1826 1827 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| AND NOT EXISTS ( | ||
| SELECT 1 | ||
| FROM [dbo].[CollectionUserPermissionsView] CUP | ||
| WHERE CUP.[OrganizationUserId] = OU.[Id] | ||
| AND CUP.[OrganizationId] = @OrganizationId | ||
| ); |
There was a problem hiding this comment.
❌ CRITICAL: This logic error from the previous review is still not fixed. The query only checks CollectionUserPermissionsView but doesn't check CollectionGroupPermissionsView.
Problem: Users who have collection access exclusively through groups (but not direct user-collection assignments) will incorrectly appear as "users without collection access."
Fix:
| AND NOT EXISTS ( | |
| SELECT 1 | |
| FROM [dbo].[CollectionUserPermissionsView] CUP | |
| WHERE CUP.[OrganizationUserId] = OU.[Id] | |
| AND CUP.[OrganizationId] = @OrganizationId | |
| ); | |
| AND NOT EXISTS ( | |
| SELECT 1 | |
| FROM [dbo].[CollectionUserPermissionsView] CUP | |
| WHERE CUP.[OrganizationUserId] = OU.[Id] | |
| AND CUP.[OrganizationId] = @OrganizationId | |
| ) | |
| AND NOT EXISTS ( | |
| SELECT 1 | |
| FROM [dbo].[CollectionGroupPermissionsView] CGP | |
| WHERE CGP.[OrganizationUserId] = OU.[Id] | |
| AND CGP.[OrganizationId] = @OrganizationId | |
| ) |
There was a problem hiding this comment.
Same thing here. Claude keeps add this suggestion, but the change has been made.
| @@ -0,0 +1,109 @@ | |||
| CREATE PROCEDURE dbo.MemberAccessReport_GetMemberAccessCipherDetailsByOrganizationId | |||
There was a problem hiding this comment.
MemberAccessDetail_GetMemberAccessCipherDetailsByOrganizationId.sql but the procedure is named MemberAccessReport_GetMemberAccessCipherDetailsByOrganizationId (Detail vs Report).
The file should be renamed to MemberAccessReport_GetMemberAccessCipherDetailsByOrganizationId.sql for consistency.
| public Guid? UserGuid { get; set; } | ||
| public string UserName { get; set; } | ||
| public string Email { get; set; } | ||
| public string AvatarColor { get; set; } |
There was a problem hiding this comment.
👍 Good addition of AvatarColor field to support frontend requirements.
| Email = g.Key.Email, | ||
| AvatarColor = g.Key.AvatarColor, | ||
| TwoFactorEnabled = orgUsersTwoFactorEnabled.FirstOrDefault(x => x.userId == g.Key.UserGuid).twoFactorIsEnabled, | ||
| AccountRecoveryEnabled = !string.IsNullOrWhiteSpace(g.Key.ResetPasswordKey) && orgAbility.UseResetPassword, |
There was a problem hiding this comment.
💭 Potential null reference: If orgAbility is null, this will throw a NullReferenceException.
Consider using null-safe navigation:
| AccountRecoveryEnabled = !string.IsNullOrWhiteSpace(g.Key.ResetPasswordKey) && orgAbility.UseResetPassword, | |
| AccountRecoveryEnabled = !string.IsNullOrWhiteSpace(g.Key.ResetPasswordKey) && orgAbility?.UseResetPassword == true, |
| AND NOT EXISTS ( | ||
| SELECT 1 | ||
| FROM [dbo].[CollectionUserPermissionsView] CUP | ||
| WHERE CUP.[OrganizationUserId] = OU.[Id] | ||
| AND CUP.[OrganizationId] = @OrganizationId | ||
| ) |
There was a problem hiding this comment.
❌ Same critical logic error: This migration script has the same issue as the stored procedure file - it only checks CollectionUserPermissionsView but not CollectionGroupPermissionsView.
Fix:
| AND NOT EXISTS ( | |
| SELECT 1 | |
| FROM [dbo].[CollectionUserPermissionsView] CUP | |
| WHERE CUP.[OrganizationUserId] = OU.[Id] | |
| AND CUP.[OrganizationId] = @OrganizationId | |
| ) | |
| AND NOT EXISTS ( | |
| SELECT 1 | |
| FROM [dbo].[CollectionUserPermissionsView] CUP | |
| WHERE CUP.[OrganizationUserId] = OU.[Id] | |
| AND CUP.[OrganizationId] = @OrganizationId | |
| ) | |
| AND NOT EXISTS ( | |
| SELECT 1 | |
| FROM [dbo].[CollectionGroupPermissionsView] CGP | |
| WHERE CGP.[OrganizationUserId] = OU.[Id] | |
| AND CGP.[OrganizationId] = @OrganizationId | |
| ) |
There was a problem hiding this comment.
same comment as the other suggestion.
prograhamming
changed the title
|
Great job! No new security vulnerabilities introduced in this pull request |
mkincaid-bw
left a comment
mkincaid-bw
left a comment
There was a problem hiding this comment.
Some minor formatting nitpicks, and some critical findings regarding EDD.
| Ci.[DeletedDate] | ||
| FROM | ||
| [dbo].[CollectionCipher] CC | ||
| INNER JOIN |
There was a problem hiding this comment.
⛏️ INNER JOIN statement should be vertically aligned with FROM clause
https://contributing.bitwarden.com/contributing/code-style/sql/#select-statements
There was a problem hiding this comment.
Changed the settings on my IDE and fixed this formatting issue.
| [dbo].[CollectionCipher] CC | ||
| INNER JOIN | ||
| [dbo].[Collection] C ON C.[Id] = CC.[CollectionId] | ||
| INNER JOIN |
There was a problem hiding this comment.
⛏️ INNER JOIN statement should be vertically aligned with FROM clause
https://contributing.bitwarden.com/contributing/code-style/sql/#select-statements
There was a problem hiding this comment.
Changed the settings on my IDE and fixed this formatting issue.
| CG.[Manage] | ||
| FROM | ||
| [dbo].[GroupUser] GU | ||
| INNER JOIN |
There was a problem hiding this comment.
⛏️ INNER JOIN statement should be vertically aligned with FROM clause
https://contributing.bitwarden.com/contributing/code-style/sql/#select-statements
There was a problem hiding this comment.
Changed the settings on my IDE and fixed this formatting issue.
| [dbo].[GroupUser] GU | ||
| INNER JOIN | ||
| [dbo].[Group] G ON G.[Id] = GU.[GroupId] | ||
| INNER JOIN |
There was a problem hiding this comment.
⛏️ INNER JOIN statement should be vertically aligned with FROM clause
https://contributing.bitwarden.com/contributing/code-style/sql/#select-statements
There was a problem hiding this comment.
Changed the settings on my IDE and fixed this formatting issue.
| [dbo].[Group] G ON G.[Id] = GU.[GroupId] | ||
| INNER JOIN | ||
| [dbo].[CollectionGroup] CG ON CG.[GroupId] = G.[Id] | ||
| INNER JOIN |
There was a problem hiding this comment.
⛏️ INNER JOIN statement should be vertically aligned with FROM clause
https://contributing.bitwarden.com/contributing/code-style/sql/#select-statements
There was a problem hiding this comment.
Changed the settings on my IDE and fixed this formatting issue.
| CU.[Manage] | ||
| FROM | ||
| [dbo].[CollectionUser] CU | ||
| INNER JOIN |
There was a problem hiding this comment.
⛏️ INNER JOIN statement should be vertically aligned with FROM clause
https://contributing.bitwarden.com/contributing/code-style/sql/#select-statements
There was a problem hiding this comment.
Changed the settings on my IDE and fixed this formatting issue.
| /// <inheritdoc /> | ||
| protected override void Up(MigrationBuilder migrationBuilder) | ||
| { | ||
| migrationBuilder.RenameColumn( |
There was a problem hiding this comment.
Renaming a database column does not follow EDD. In order to achieve this, a new column must be added, the data must be synced between the old and new columns, then the old column is dropped. This is a multi-release process. See this example for more details.
There was a problem hiding this comment.
This is no migration action that corresponds with the EF migrations. We can't do a rename anyway but you'll need a corresponding SQL migration in this file to match the EF migrations.
| migrationBuilder.RenameColumn( | ||
| name: "UserGuid", | ||
| table: "OrganizationMemberBaseDetails", | ||
| newName: "OrganizationUserId"); |
There was a problem hiding this comment.
❌ CRITICAL - EDD Violation: Direct column rename violates Evolutionary Database Design principles.
Problem: This breaks compatibility with running application instances during deployment.
Required multi-release process:
- Release 1: Add new column
OrganizationUserId, sync data fromUserGuid - Release 2: Update application code to read/write new column
- Release 3: Drop old column
UserGuid
See the EDD example for detailed guidance.
| Ci.[DeletedDate] | ||
| FROM | ||
| [dbo].[CollectionCipher] CC | ||
| INNER JOIN |
There was a problem hiding this comment.
⛏️ Formatting: INNER JOIN statements should be vertically aligned with FROM clause per SQL code style guide.
| INNER JOIN | |
| FROM | |
| [dbo].[CollectionCipher] CC | |
| INNER JOIN [dbo].[Collection] C ON C.[Id] = CC.[CollectionId] | |
| INNER JOIN [dbo].[Cipher] Ci ON Ci.[Id] = CC.[CipherId] |
| CG.[Manage] | ||
| FROM | ||
| [dbo].[GroupUser] GU | ||
| INNER JOIN |
There was a problem hiding this comment.
⛏️ Formatting: INNER JOIN statements should be vertically aligned with FROM clause per SQL code style guide.
| INNER JOIN | |
| FROM | |
| [dbo].[GroupUser] GU | |
| INNER JOIN [dbo].[Group] G ON G.[Id] = GU.[GroupId] | |
| INNER JOIN [dbo].[CollectionGroup] CG ON CG.[GroupId] = G.[Id] | |
| INNER JOIN [dbo].[Collection] C ON C.[Id] = CG.[CollectionId] |
| CU.[Manage] | ||
| FROM | ||
| [dbo].[CollectionUser] CU | ||
| INNER JOIN |
There was a problem hiding this comment.
⛏️ Formatting: INNER JOIN statements should be vertically aligned with FROM clause per SQL code style guide.
| INNER JOIN | |
| FROM | |
| [dbo].[CollectionUser] CU | |
| INNER JOIN [dbo].[Collection] C ON C.[Id] = CU.[CollectionId] |
| AND NOT EXISTS ( | ||
| SELECT 1 | ||
| FROM [dbo].[CollectionUserPermissionsView] CUP | ||
| WHERE CUP.[OrganizationUserId] = OU.[Id] | ||
| AND CUP.[OrganizationId] = @OrganizationId | ||
| ) | ||
| AND NOT EXISTS ( | ||
| SELECT 1 | ||
| FROM [dbo].[CollectionGroupPermissionsView] CGP | ||
| WHERE CGP.[OrganizationUserId] = OU.[Id] | ||
| AND CGP.[OrganizationId] = @OrganizationId | ||
| ) |
There was a problem hiding this comment.
👍 Logic Fix Confirmed: This correctly checks both CollectionUserPermissionsView AND CollectionGroupPermissionsView to identify users without collection access. This addresses the concern raised in the previous Claude review.
🎟️ Tracking
PM-25576
📔 Objective
Request from architecture to use views in our member access query and we needed to bring the avatar color for the front end.
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes