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.

Sign up for GitHub

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

[PM-13693] Fixed null warnings #4896

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
+19 −23
Open

[PM-13693] Fixed null warnings #4896

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5360cba
fixed potential buge #14 on the artical
MJebran Oct 2, 2024
09e9adf
Merge branch 'bitwarden:main' into main
MJebran Oct 15, 2024
11972ca
issue 1 and 2 are not longer applicable or fixed. issue 3,4 in artica…
MJebran Oct 15, 2024
8a42c87
issue 5 was not found, issue 6 in the article is now fixed -added GUI…
MJebran Oct 15, 2024
097eafb
issues 7-12 where not applicable anymore, issue 13 is done. We got ri…
MJebran Oct 15, 2024
f6b0234
Merge branch 'bitwarden:main' into main
MJebran Oct 21, 2024
4b47e62
updated method names
MJebran Nov 6, 2024
9a759ed
Merge branch 'main' of https://github.com/MJebran/server
MJebran Nov 6, 2024
0cbbc84
Merge branch 'bitwarden:main' into main
MJebran Nov 6, 2024
2fd6ea3
Merge branch 'bitwarden:main' into main
MJebran Nov 13, 2024
218e417
we did dotnet format
MJebran Nov 13, 2024
3c785aa
Merge branch 'main' into main
djsmith85 Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/Api/Models/Request/Accounts/PremiumRequestModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@ public class PremiumRequestModel : IValidatableObject

public bool Validate(GlobalSettings globalSettings)
{
if (!(License == null && !globalSettings.SelfHosted) ||
(License != null && globalSettings.SelfHosted))
if (!(License == null && !globalSettings.SelfHosted))
{
return false;
}
Expand Down
4 changes: 2 additions & 2 deletions src/Api/Tools/Controllers/ImportCiphersController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.ImportIntoIndividualVaultAsync(folders, ciphers, model.FolderRelationships, userId);
}

[HttpPost("import-organization")]
Expand Down Expand Up @@ -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.ImportIntoOrganizationalVaultAsync(collections, ciphers, model.CollectionRelationships, userId);
}

private async Task<bool> CheckOrgImportPermission(List<Collection> collections, Guid orgId)
Expand Down
5 changes: 3 additions & 2 deletions src/Billing/Controllers/FreshdeskController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,9 @@ private async Task<HttpResponseMessage> CallFreshdeskApiAsync(HttpRequestMessage
throw;
}
}
await Task.Delay(30000 * (retriedCount + 1));
return await CallFreshdeskApiAsync(request, retriedCount++);
retriedCount ++;
await Task.Delay(30000 * retriedCount);
return await CallFreshdeskApiAsync(request, retriedCount);
}

private TAttribute GetAttribute<TAttribute>(Enum enumValue) where TAttribute : Attribute
Expand Down
6 changes: 3 additions & 3 deletions src/Core/Vault/Services/ICipherService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@ Task ShareAsync(Cipher originalCipher, Cipher cipher, Guid organizationId, IEnum
Task ShareManyAsync(IEnumerable<(Cipher cipher, DateTime? lastKnownRevisionDate)> ciphers, Guid organizationId,
IEnumerable<Guid> collectionIds, Guid sharingUserId);
Task SaveCollectionsAsync(Cipher cipher, IEnumerable<Guid> collectionIds, Guid savingUserId, bool orgAdmin);
Task ImportCiphersAsync(List<Folder> folders, List<CipherDetails> ciphers,
IEnumerable<KeyValuePair<int, int>> folderRelationships);
Task ImportCiphersAsync(List<Collection> collections, List<CipherDetails> ciphers,
Task ImportIntoIndividualVaultAsync(List<Folder> folders, List<CipherDetails> ciphers,
IEnumerable<KeyValuePair<int, int>> folderRelationships, Guid importingUserId);
Task ImportIntoOrganizationalVaultAsync(List<Collection> collections, List<CipherDetails> ciphers,
IEnumerable<KeyValuePair<int, int>> collectionRelationships, Guid importingUserId);
Task SoftDeleteAsync(Cipher cipher, Guid deletingUserId, bool orgAdmin = false);
Task SoftDeleteManyAsync(IEnumerable<Guid> cipherIds, Guid deletingUserId, Guid? organizationId = null, bool orgAdmin = false);
Expand Down
18 changes: 7 additions & 11 deletions src/Core/Vault/Services/Implementations/CipherService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -678,15 +678,14 @@ await _collectionCipherRepository.UpdateCollectionsForAdminAsync(cipher.Id,
await _pushService.PushSyncCipherUpdateAsync(cipher, collectionIds);
}

public async Task ImportCiphersAsync(
public async Task ImportIntoIndividualVaultAsync(
List<Folder> folders,
List<CipherDetails> ciphers,
IEnumerable<KeyValuePair<int, int>> folderRelationships)
IEnumerable<KeyValuePair<int, int>> folderRelationships,
Guid importingUserId)
Comment on lines +684 to +685
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
var userId = folders.FirstOrDefault()?.UserId ?? ciphers.FirstOrDefault()?.UserId;

// Make sure the user can save new ciphers to their personal vault
var anyPersonalOwnershipPolicies = await _policyService.AnyPoliciesApplicableToUserAsync(userId.Value, PolicyType.PersonalOwnership);
var anyPersonalOwnershipPolicies = await _policyService.AnyPoliciesApplicableToUserAsync(importingUserId, PolicyType.PersonalOwnership);
if (anyPersonalOwnershipPolicies)
{
throw new BadRequestException("You cannot import items into your personal vault because you are " +
Expand All @@ -703,7 +702,7 @@ public async Task ImportCiphersAsync(
}
}

var userfoldersIds = (await _folderRepository.GetManyByUserIdAsync(userId ?? Guid.Empty)).Select(f => f.Id).ToList();
var userfoldersIds = (await _folderRepository.GetManyByUserIdAsync(importingUserId)).Select(f => f.Id).ToList();

//Assign id to the ones that don't exist in DB
//Need to keep the list order to create the relationships
Expand Down Expand Up @@ -736,13 +735,10 @@ public async Task ImportCiphersAsync(
await _cipherRepository.CreateAsync(ciphers, newFolders);

// push
if (userId.HasValue)
{
await _pushService.PushSyncVaultAsync(userId.Value);
}
await _pushService.PushSyncVaultAsync(importingUserId);
}

public async Task ImportCiphersAsync(
public async Task ImportIntoOrganizationalVaultAsync(
List<Collection> collections,
List<CipherDetails> ciphers,
IEnumerable<KeyValuePair<int, int>> collectionRelationships,
Expand Down
4 changes: 2 additions & 2 deletions ...ucture.EntityFramework/AdminConsole/Repositories/Queries/OrganizationUserUserViewQuery.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@ from su in su_g.DefaultIfEmpty()
SsoExternalId = x.su.ExternalId,
Permissions = x.ou.Permissions,
ResetPasswordKey = x.ou.ResetPasswordKey,
UsesKeyConnector = x.u != null && x.u.UsesKeyConnector,
UsesKeyConnector = x.u.UsesKeyConnector,
AccessSecretsManager = x.ou.AccessSecretsManager,
HasMasterPassword = x.u != null && !string.IsNullOrWhiteSpace(x.u.MasterPassword)
HasMasterPassword = !string.IsNullOrWhiteSpace(x.u.MasterPassword)
Comment on lines +31 to +33
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});
}
}
2 changes: 1 addition & 1 deletion test/Core.Test/Vault/Services/CipherServiceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public async Task ImportCiphersAsync_IntoOrganization_Success(
.GetManyByOrganizationIdAsync(organization.Id)
.Returns(new List<Collection> { collections[0] });

await sutProvider.Sut.ImportCiphersAsync(collections, ciphers, collectionRelationships, importingUserId);
await sutProvider.Sut.ImportIntoOrganizationalVaultAsync(collections, ciphers, collectionRelationships, importingUserId);

await sutProvider.GetDependency<ICipherRepository>().Received(1).CreateAsync(
ciphers,
Expand Down
Loading