[PM-13693] Fixed null warnings #4896

MJebran · 2024-10-15T18:22:39Z

🎟️ Tracking

github issue link:
#2013

link to 14 specified issues mentioned in the above link:
https://pvs-studio.com/en/blog/posts/csharp/0947/

📔 Objective

We fixed 4 issues that could have cased null error, issues were warnings themselves

Changed a method that was not Undernet test, looking for conformation to add tests.

Collaborators (mob programmed)
https://github.com/luris26
https://github.com/BensonB12

🦮 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

…le fixed\n\nCo-authored-by: Luris Solis <[email protected]>\nCo-authored-by: Benson Bird <[email protected]>

…D parameter and rename methods\n\nCo-authored-by: Luris Solis <[email protected]>\nCo-authored-by: Benson Bird <[email protected]>

…d of the null checks because it already assumes that x.u is not null\n\nCo-authored-by: Luris Solis <[email protected]>\nCo-authored-by: Benson Bird <[email protected]>

bitwarden-bot · 2024-10-15T18:22:56Z

Thank you for your contribution! We've added this to our internal Community PR board for review.
ID: PM-13693

CLAassistant · 2024-10-15T18:23:12Z

All committers have signed the CLA.

justindbaur

Just one note, thanks for the contribution!

justindbaur · 2024-10-30T20:41:16Z

src/Billing/Controllers/FreshdeskController.cs

@@ -166,7 +166,7 @@ private async Task<HttpResponseMessage> CallFreshdeskApiAsync(HttpRequestMessage
            }
        }
        await Task.Delay(30000 * (retriedCount + 1));
-        return await CallFreshdeskApiAsync(request, retriedCount++);
+        return await CallFreshdeskApiAsync(request, retriedCount + 1);


We should probably update this to just increment retriedCount a single time. GitHub isn't letting me to an actual suggestion but it would look like this:

retriedCount++; await Task.Delay(30000 * retriedCount); return await CallFreshdeskApiAsync(request, retriedCount);

justindbaur · 2024-10-30T20:41:56Z

src/Core/Vault/Services/Implementations/CipherService.cs

+        IEnumerable<KeyValuePair<int, int>> folderRelationships,
+        Guid importingUserId)


Love this change, I really dislike trying to grab info from the first item in a list. Much nicer this way.

justindbaur · 2024-10-30T21:06:00Z

...structure.EntityFramework/AdminConsole/Repositories/Queries/OrganizationUserUserViewQuery.cs

+            UsesKeyConnector = x.u.UsesKeyConnector,
            AccessSecretsManager = x.ou.AccessSecretsManager,
-            HasMasterPassword = x.u != null && !string.IsNullOrWhiteSpace(x.u.MasterPassword)
+            HasMasterPassword = !string.IsNullOrWhiteSpace(x.u.MasterPassword)


Generated SQLite from before change

SELECT "o"."Id", "o"."UserId", "o"."OrganizationId", "u"."Name", COALESCE("u"."Email", "o"."Email") AS "Email", "u"."AvatarColor", "u"."TwoFactorProviders", "u"."Premium", "o"."Status", "o"."Type", "o"."ExternalId", "s"."ExternalId" AS "SsoExternalId", "o"."Permissions", "o"."ResetPasswordKey", "u"."Id" IS NOT NULL AND "u"."UsesKeyConnector" AS "UsesKeyConnector", "o"."AccessSecretsManager", "u"."Id" IS NOT NULL AND "u"."MasterPassword" IS NOT NULL AND trim("u"."MasterPassword") <> 'P|CfDJ8KUSA3v4Y1dFg8Rnf6FjA5fdvqaglKtClSQ1I3diP2vxzTdwq2_dCgDHF-chiyxdEEkJZ_XZnvZFhIpQGoqQL4f35sOzasRWeL8HXy0O59nSBVpX2WOp565OeY4fYRwuNg' AS "HasMasterPassword" FROM "OrganizationUser" AS "o" LEFT JOIN "User" AS "u" ON "o"."UserId" = "u"."Id" LEFT JOIN "SsoUser" AS "s" ON "o"."UserId" = "s"."UserId" AND "o"."OrganizationId" = "s"."OrganizationId"

After change:

SELECT "o"."Id", "o"."UserId", "o"."OrganizationId", "u"."Name", COALESCE("u"."Email", "o"."Email") AS "Email", "u"."AvatarColor", "u"."TwoFactorProviders", "u"."Premium", "o"."Status", "o"."Type", "o"."ExternalId", "s"."ExternalId" AS "SsoExternalId", "o"."Permissions", "o"."ResetPasswordKey", "u"."UsesKeyConnector", "o"."AccessSecretsManager", "u"."MasterPassword" IS NOT NULL AND trim("u"."MasterPassword") <> 'P|CfDJ8KUSA3v4Y1dFg8Rnf6FjA5dJYW3Rjd8aosV-K01yVWmZVkVbItCW4oOuqXMvKO2PD9DIw0XKjy8sqgWtIKF1q7cT1JKgc3F2mwImAtRqP5WfrlURovVo9WdKmv2sI9lHUg' AS "HasMasterPassword" FROM "OrganizationUser" AS "o" LEFT JOIN "User" AS "u" ON "o"."UserId" = "u"."Id" LEFT JOIN "SsoUser" AS "s" ON "o"."UserId" = "s"."UserId" AND "o"."OrganizationId" = "s"."OrganizationId"

And the equivalent hand wrote SQL:

SELECT OU.[Id], OU.[UserId], OU.[OrganizationId], U.[Name], ISNULL(U.[Email], OU.[Email]) Email, U.[AvatarColor], U.[TwoFactorProviders], U.[Premium], OU.[Status], OU.[Type], OU.[AccessSecretsManager], OU.[ExternalId], SU.[ExternalId] SsoExternalId, OU.[Permissions], OU.[ResetPasswordKey], U.[UsesKeyConnector], CASE WHEN U.[MasterPassword] IS NOT NULL THEN 1 ELSE 0 END AS HasMasterPassword FROM [dbo].[OrganizationUser] OU LEFT JOIN [dbo].[User] U ON U.[Id] = OU.[UserId] LEFT JOIN [dbo].[SsoUser] SU ON SU.[UserId] = OU.[UserId] AND SU.[OrganizationId] = OU.[OrganizationId]

So this seems like a good change to me.

I also double checked this because I was concerned about orgUsers in the invited state that are not linked to a user, but they're excluded by the inner join on User.Id. So we should always have a User available. And as has been pointed out, we access x.u before this anyway, so clearly we do.

eliykat

Thanks for your contribution @MJebran ! The code owned by team-admin-console-dev looks good to me.

eliykat · 2024-11-01T03:52:10Z

...structure.EntityFramework/AdminConsole/Repositories/Queries/OrganizationUserUserViewQuery.cs

+            UsesKeyConnector = x.u.UsesKeyConnector,
            AccessSecretsManager = x.ou.AccessSecretsManager,
-            HasMasterPassword = x.u != null && !string.IsNullOrWhiteSpace(x.u.MasterPassword)
+            HasMasterPassword = !string.IsNullOrWhiteSpace(x.u.MasterPassword)


I also double checked this because I was concerned about orgUsers in the invited state that are not linked to a user, but they're excluded by the inner join on User.Id. So we should always have a User available. And as has been pointed out, we access x.u before this anyway, so clearly we do.

djsmith85 · 2024-11-05T16:06:34Z

src/Api/Tools/Controllers/ImportCiphersController.cs

@@ -57,7 +57,7 @@ public async Task PostImport([FromBody] ImportCiphersRequestModel model)
        var userId = _userService.GetProperUserId(User).Value;
        var folders = model.Folders.Select(f => f.ToFolder(userId)).ToList();
        var ciphers = model.Ciphers.Select(c => c.ToCipherDetails(userId, false)).ToList();
-        await _cipherService.ImportCiphersAsync(folders, ciphers, model.FolderRelationships);
+        await _cipherService.ImportFolderCiphersAsync(folders, ciphers, model.FolderRelationships, userId);


Initially these were likely meant to be method overloads. If we are changing the name I'd prefer:

Suggested change

await _cipherService.ImportFolderCiphersAsync(folders, ciphers, model.FolderRelationships, userId);

await _cipherService.ImportIntoIndividualVaultAsync(folders, ciphers, model.FolderRelationships, userId);

djsmith85 · 2024-11-05T16:06:37Z

src/Api/Tools/Controllers/ImportCiphersController.cs

@@ -85,7 +85,7 @@ public async Task PostImport([FromQuery] string organizationId,

        var userId = _userService.GetProperUserId(User).Value;
        var ciphers = model.Ciphers.Select(l => l.ToOrganizationCipherDetails(orgId)).ToList();
-        await _cipherService.ImportCiphersAsync(collections, ciphers, model.CollectionRelationships, userId);
+        await _cipherService.ImportCollectionCiphersAsync(collections, ciphers, model.CollectionRelationships, userId);


Initially these were likely meant to be method overloads. If we are changing the name I'd prefer:

Suggested change

await _cipherService.ImportCollectionCiphersAsync(collections, ciphers, model.CollectionRelationships, userId);

await _cipherService.ImportIntoOrganizationalVaultAsync(collections, ciphers, model.CollectionRelationships, userId);

djsmith85 · 2024-11-05T16:08:57Z

src/Core/Vault/Services/Implementations/CipherService.cs

@@ -678,15 +678,14 @@ await _collectionCipherRepository.UpdateCollectionsForAdminAsync(cipher.Id,
        await _pushService.PushSyncCipherUpdateAsync(cipher, collectionIds);
    }

-    public async Task ImportCiphersAsync(
+    public async Task ImportFolderCiphersAsync(


Note to self: These should likely be moved into a tools-owned import service leveraging methods from the CipherService/Repository

Co-authored-by: Benson Bird <[email protected]> Co-authored-by: Luris Solis <[email protected]>

djsmith85

Approving for Tools

cturnbull-bitwarden

Billing changes look good. Thank you for your contribution!

eliykat

There are some CI failures that are blocking merge.

formatting - please run dotnet format from the root of the repository and then commit the result.
tests failures - for some reason, removing the null check on UsesKeyConnector in OrganizationUserUserViewQuery has broken the SCIM integration tests. It throws a null reference exception even though when I debug everything seems to have a value. However, this also made me notice that we're using it in the SCIM PostUserCommand to return an invited user's details, however I think the inner join in this query would exclude invited users. I will find some time to look into this further.

Co-authored-by: Luris Solis <[email protected]>\Co-authored-by: Benson Bird <[email protected]>

github-actions · 2024-12-20T12:46:49Z

Checkmarx One – Scan Summary & Details – ecec235d-a0dc-4d07-b8b4-cb2a2aba76a5

No New Or Fixed Issues Found

MJebran and others added 5 commits October 2, 2024 15:23

fixed potential buge bitwarden#14 on the artical

5360cba

Merge branch 'bitwarden:main' into main

09e9adf

issue 1 and 2 are not longer applicable or fixed. issue 3,4 in artica…

11972ca

…le fixed\n\nCo-authored-by: Luris Solis <[email protected]>\nCo-authored-by: Benson Bird <[email protected]>

issue 5 was not found, issue 6 in the article is now fixed -added GUI…

8a42c87

…D parameter and rename methods\n\nCo-authored-by: Luris Solis <[email protected]>\nCo-authored-by: Benson Bird <[email protected]>

issues 7-12 where not applicable anymore, issue 13 is done. We got ri…

097eafb

…d of the null checks because it already assumes that x.u is not null\n\nCo-authored-by: Luris Solis <[email protected]>\nCo-authored-by: Benson Bird <[email protected]>

MJebran requested review from a team as code owners October 15, 2024 18:22

MJebran requested review from eliykat and cd-bitwarden October 15, 2024 18:22

bitwarden-bot changed the title ~~Fixed these issues https://pvs-studio.com/en/blog/posts/csharp/0947/~~ [PM-13693] Fixed these issues https://pvs-studio.com/en/blog/posts/csharp/0947/ Oct 15, 2024

bitwarden-bot added the community-pr label Oct 15, 2024

MJebran changed the title ~~[PM-13693] Fixed these issues https://pvs-studio.com/en/blog/posts/csharp/0947/~~ [PM-13693] Fixed null warnings Oct 15, 2024

Merge branch 'bitwarden:main' into main

f6b0234

justindbaur reviewed Oct 30, 2024

View reviewed changes

eliykat previously approved these changes Nov 1, 2024

View reviewed changes

djsmith85 requested changes Nov 5, 2024

View reviewed changes

djsmith85 reviewed Nov 5, 2024

View reviewed changes

MJebran and others added 2 commits November 6, 2024 14:43

updated method names

4b47e62

Co-authored-by: Benson Bird <[email protected]> Co-authored-by: Luris Solis <[email protected]>

Merge branch 'main' of https://github.com/MJebran/server

9a759ed

MJebran dismissed eliykat’s stale review via 9a759ed November 6, 2024 21:47

Merge branch 'bitwarden:main' into main

0cbbc84

MJebran requested a review from djsmith85 November 6, 2024 21:55

djsmith85 assigned djsmith85, justindbaur and eliykat Nov 6, 2024

djsmith85 previously approved these changes Nov 7, 2024

View reviewed changes

eliykat previously approved these changes Nov 11, 2024

View reviewed changes

djsmith85 requested a review from justindbaur November 11, 2024 09:04

cturnbull-bitwarden previously approved these changes Nov 11, 2024

View reviewed changes

eliykat reviewed Nov 11, 2024

View reviewed changes

MJebran and others added 2 commits November 13, 2024 14:29

Merge branch 'bitwarden:main' into main

2fd6ea3

we did dotnet format

218e417

Co-authored-by: Luris Solis <[email protected]>\Co-authored-by: Benson Bird <[email protected]>

MJebran dismissed stale reviews from eliykat, cturnbull-bitwarden, and djsmith85 via 218e417 November 13, 2024 21:46

djsmith85 requested review from djsmith85, cturnbull-bitwarden and eliykat November 19, 2024 07:17

djsmith85 approved these changes Nov 19, 2024

View reviewed changes

amorask-bitwarden approved these changes Nov 21, 2024

View reviewed changes

shane-melton approved these changes Dec 4, 2024

View reviewed changes

jonashendrickx approved these changes Dec 6, 2024

View reviewed changes

Merge branch 'main' into main

3c785aa

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-13693] Fixed null warnings #4896

[PM-13693] Fixed null warnings #4896

MJebran commented Oct 15, 2024 •

edited

Loading

bitwarden-bot commented Oct 15, 2024

CLAassistant commented Oct 15, 2024 •

edited

Loading

justindbaur left a comment

justindbaur Oct 30, 2024

justindbaur Oct 30, 2024

justindbaur Oct 30, 2024

eliykat Nov 1, 2024

eliykat left a comment

eliykat Nov 1, 2024

djsmith85 Nov 5, 2024

djsmith85 Nov 5, 2024

djsmith85 Nov 5, 2024

djsmith85 left a comment

cturnbull-bitwarden left a comment

eliykat left a comment

github-actions bot commented Dec 20, 2024

		IEnumerable<KeyValuePair<int, int>> folderRelationships,
		Guid importingUserId)

	await _cipherService.ImportFolderCiphersAsync(folders, ciphers, model.FolderRelationships, userId);
	await _cipherService.ImportIntoIndividualVaultAsync(folders, ciphers, model.FolderRelationships, userId);

	await _cipherService.ImportCollectionCiphersAsync(collections, ciphers, model.CollectionRelationships, userId);
	await _cipherService.ImportIntoOrganizationalVaultAsync(collections, ciphers, model.CollectionRelationships, userId);

[PM-13693] Fixed null warnings #4896

Are you sure you want to change the base?

[PM-13693] Fixed null warnings #4896

Conversation

MJebran commented Oct 15, 2024 • edited Loading

🎟️ Tracking

📔 Objective

🦮 Reviewer guidelines

bitwarden-bot commented Oct 15, 2024

CLAassistant commented Oct 15, 2024 • edited Loading

justindbaur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliykat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djsmith85 left a comment

Choose a reason for hiding this comment

cturnbull-bitwarden left a comment

Choose a reason for hiding this comment

eliykat left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 20, 2024

No New Or Fixed Issues Found

MJebran commented Oct 15, 2024 •

edited

Loading

CLAassistant commented Oct 15, 2024 •

edited

Loading