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.

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
3 changes: 1 addition & 2 deletions src/Api/Models/Request/Accounts/PremiumRequestModel.cs
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
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
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -678,15 +678,14 @@
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 @@
}
}

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 @@
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 All @@ -750,7 +746,7 @@
{
var org = collections.Count > 0 ?
await _organizationRepository.GetByIdAsync(collections[0].OrganizationId) :
await _organizationRepository.GetByIdAsync(ciphers.FirstOrDefault(c => c.OrganizationId.HasValue).OrganizationId.Value);

Check warning on line 749 in src/Core/Vault/Services/Implementations/CipherService.cs

View workflow job for this annotation

GitHub Actions / Quality scan

'ciphers.FirstOrDefault(c => c.OrganizationId.HasValue)' is null on at least one execution path. (https://rules.sonarsource.com/csharp/RSPEC-2259)
var importingOrgUser = await _organizationUserRepository.GetByOrganizationAsync(org.Id, importingUserId);

if (collections.Count > 0 && org != null && org.MaxCollections.HasValue)
Expand Down
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
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