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-7407] Implemented check for organizations with unassigned items #3150

Merged
merged 2 commits into from
Apr 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions src/Core/Abstractions/IApiService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ public interface IApiService
Task<CipherResponse> PutShareCipherAsync(string id, CipherShareRequest request);
Task PutDeleteCipherAsync(string id);
Task<CipherResponse> PutRestoreCipherAsync(string id);
Task<bool> HasUnassignedCiphersAsync();
Task RefreshIdentityTokenAsync();
Task<SsoPrevalidateResponse> PreValidateSsoAsync(string identifier);
Task<TResponse> SendAsync<TRequest, TResponse>(HttpMethod method, string path,
Expand Down
1 change: 1 addition & 0 deletions src/Core/Abstractions/ICipherService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -37,5 +37,6 @@ Task<Tuple<List<CipherView>, List<CipherView>, List<CipherView>>> GetAllDecrypte
Task<byte[]> DownloadAndDecryptAttachmentAsync(string cipherId, AttachmentView attachment, string organizationId);
Task SoftDeleteWithServerAsync(string id);
Task RestoreWithServerAsync(string id);
Task<bool> VerifyOrganizationHasUnassignedItemsAsync();
}
}
2 changes: 2 additions & 0 deletions src/Core/Abstractions/IStateService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,8 @@ public interface IStateService
Task<BwRegion?> GetActiveUserRegionAsync();
Task<BwRegion?> GetPreAuthRegionAsync();
Task SetPreAuthRegionAsync(BwRegion value);
Task<bool> GetShouldCheckOrganizationUnassignedItemsAsync(string userId = null);
Copy link
Member

Choose a reason for hiding this comment

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

🤔 Naming wise, following a similar pattern to the other prompts (e.g. Get/Set UnassignedItemsPromptShownAsync) would make it easier to find this in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that this getter doesn't return if the prompt has been shown or not, it returns whether the check (and potentially the prompt) should be done or not, i.e. if the user chose "Remind me later" or not.
Also there are also two other that have Should:

  • GetShouldTrustDeviceAsync
  • GetShouldConnectToWatchAsync

Even though I agree having everything with the same name structure is better, but on the other hand I like having specific naming so one can exactly know what the method/property does without having to look much in other places.
What do you think?

Task SetShouldCheckOrganizationUnassignedItemsAsync(bool shouldCheck, string userId = null);
[Obsolete("Use GetPinKeyEncryptedUserKeyAsync instead, left for migration purposes")]
Task<string> GetPinProtectedAsync(string userId = null);
[Obsolete("Use SetPinKeyEncryptedUserKeyAsync instead, left for migration purposes")]
Expand Down
2 changes: 2 additions & 0 deletions src/Core/Constants.cs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ public static class Constants
public const string PreLoginEmailKey = "preLoginEmailKey";
public const string ConfigsKey = "configsKey";
public const string DisplayEuEnvironmentFlag = "display-eu-environment";
public const string UnassignedItemsBannerFlag = "unassigned-items-banner";
public const string RegionEnvironment = "regionEnvironment";
public const string DuoCallback = "bitwarden://duo-callback";

Expand Down Expand Up @@ -136,6 +137,7 @@ public static string AccountBiometricIntegrityValidKey(string userId, string sys
public static string ShouldConnectToWatchKey(string userId) => $"shouldConnectToWatch_{userId}";
public static string ScreenCaptureAllowedKey(string userId) => $"screenCaptureAllowed_{userId}";
public static string PendingAdminAuthRequest(string userId) => $"pendingAdminAuthRequest_{userId}";
public static string ShouldCheckOrganizationUnassignedItemsKey(string userId) => $"shouldCheckOrganizationUnassignedItems_{userId}";
Copy link
Member

Choose a reason for hiding this comment

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

⛏️ (not really relevant but worth mentioning) other similar constants don't have verbiage like "ShouldCheck".

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO without the "should check" one doesn't know what the flag is for, only that it has to do with organization unassigned items. However, if having the verbiage alike others is more important I can change it.

[Obsolete]
public static string KeyKey(string userId) => $"key_{userId}";
[Obsolete]
Expand Down
1 change: 1 addition & 0 deletions src/Core/Models/AppOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ public class AppOptions
public bool CopyInsteadOfShareAfterSaving { get; set; }
public bool HideAccountSwitcher { get; set; }
public OtpData? OtpData { get; set; }
public bool HasJustLoggedInOrUnlocked { get; set; }

public void SetAllFrom(AppOptions o)
{
Expand Down
1 change: 1 addition & 0 deletions src/Core/Pages/Accounts/LockPage.xaml.cs
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,7 @@ private async Task UnlockedAsync()
}
var previousPage = await AppHelpers.ClearPreviousPage();

_appOptions.HasJustLoggedInOrUnlocked = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

💭 Instead of having this variable scattered around and risk missing a path to the vault, can't we just check once on TabsPage.OnCreate with a control variable HasRunCheckOrganizationUnassignedItems?

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that TabsPage.OnCreate can be called when the vault has timeout never, then it's not technically after login/unlock as it will be called when opening the app and given timeout never the vault would be already unlocked.

App.MainPage = new TabsPage(_appOptions, previousPage);
}
}
Expand Down
2 changes: 2 additions & 0 deletions src/Core/Pages/Accounts/LoginApproveDevicePage.xaml.cs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ private async Task ContinueToVaultAsync()
{
return;
}

_appOptions.HasJustLoggedInOrUnlocked = true;
var previousPage = await AppHelpers.ClearPreviousPage();
App.MainPage = new TabsPage(_appOptions, previousPage);
}
Expand Down
2 changes: 2 additions & 0 deletions src/Core/Pages/Accounts/LoginPage.xaml.cs
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,8 @@ private async Task LogInSuccessAsync()
{
return;
}

_appOptions.HasJustLoggedInOrUnlocked = true;
var previousPage = await AppHelpers.ClearPreviousPage();
App.MainPage = new TabsPage(_appOptions, previousPage);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ private async Task LogInSuccessAsync()
{
return;
}

_appOptions.HasJustLoggedInOrUnlocked = true;
var previousPage = await AppHelpers.ClearPreviousPage();
App.MainPage = new TabsPage(_appOptions, previousPage);
}
Expand Down
2 changes: 2 additions & 0 deletions src/Core/Pages/Accounts/SetPasswordPage.xaml.cs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ private async Task SetPasswordSuccessAsync()
{
return;
}

_appOptions.HasJustLoggedInOrUnlocked = true;
var previousPage = await AppHelpers.ClearPreviousPage();
App.MainPage = new TabsPage(_appOptions, previousPage);
}
Expand Down
2 changes: 2 additions & 0 deletions src/Core/Pages/Accounts/TwoFactorPage.xaml.cs
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,8 @@ private async Task TwoFactorAuthSuccessToMainAsync()
{
return;
}

_appOptions.HasJustLoggedInOrUnlocked = true;
var previousPage = await AppHelpers.ClearPreviousPage();
App.MainPage = new TabsPage(_appOptions, previousPage);
}
Expand Down
2 changes: 1 addition & 1 deletion src/Core/Pages/TabsPage.cs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public TabsPage(AppOptions appOptions = null, PreviousPageInfo previousPage = nu
_keyConnectorService = ServiceContainer.Resolve<IKeyConnectorService>("keyConnectorService");
_stateService = ServiceContainer.Resolve<IStateService>();

_groupingsPage = new NavigationPage(new GroupingsPage(true, previousPage: previousPage))
_groupingsPage = new NavigationPage(new GroupingsPage(true, previousPage: previousPage, appOptions: appOptions))
{
Title = AppResources.MyVault,
IconImageSource = "lock.png"
Expand Down
6 changes: 5 additions & 1 deletion src/Core/Pages/Vault/GroupingsPage/GroupingsPage.xaml.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using Bit.App.Abstractions;
using Bit.App.Controls;
using Bit.App.Models;
using Bit.Core.Abstractions;
using Bit.Core.Enums;
using Bit.Core.Models.Data;
Expand Down Expand Up @@ -27,7 +28,7 @@ public partial class GroupingsPage : BaseContentPage

public GroupingsPage(bool mainPage, CipherType? type = null, string folderId = null,
string collectionId = null, string pageTitle = null, string vaultFilterSelection = null,
PreviousPageInfo previousPage = null, bool deleted = false, bool showTotp = false)
PreviousPageInfo previousPage = null, bool deleted = false, bool showTotp = false, AppOptions appOptions = null)
{
_pageName = string.Concat(nameof(GroupingsPage), "_", DateTime.UtcNow.Ticks);
InitializeComponent();
Expand All @@ -50,6 +51,7 @@ public GroupingsPage(bool mainPage, CipherType? type = null, string folderId = n
_vm.CollectionId = collectionId;
_vm.Deleted = deleted;
_vm.ShowTotp = showTotp;
_vm.AppOptions = appOptions;
_previousPage = previousPage;
if (pageTitle != null)
{
Expand Down Expand Up @@ -160,6 +162,8 @@ await LoadOnAppearedAsync(_mainLayout, false, async () =>
return;
}

await _vm.CheckOrganizationUnassignedItemsAsync();

// Push registration
var lastPushRegistration = await _stateService.GetPushLastRegistrationDateAsync();
lastPushRegistration = lastPushRegistration.GetValueOrDefault(DateTime.MinValue);
Expand Down
60 changes: 60 additions & 0 deletions src/Core/Pages/Vault/GroupingsPage/GroupingsPageViewModel.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System.Windows.Input;
using Bit.App.Abstractions;
using Bit.App.Controls;
using Bit.App.Models;
using Bit.App.Utilities;
using Bit.Core.Abstractions;
using Bit.Core.Enums;
Expand Down Expand Up @@ -45,6 +46,8 @@ public class GroupingsPageViewModel : VaultFilterViewModel
private readonly IPasswordRepromptService _passwordRepromptService;
private readonly IOrganizationService _organizationService;
private readonly IPolicyService _policyService;
private readonly IConfigService _configService;
private readonly IEnvironmentService _environmentService;
private readonly ILogger _logger;

public GroupingsPageViewModel()
Expand All @@ -61,6 +64,8 @@ public GroupingsPageViewModel()
_passwordRepromptService = ServiceContainer.Resolve<IPasswordRepromptService>("passwordRepromptService");
_organizationService = ServiceContainer.Resolve<IOrganizationService>("organizationService");
_policyService = ServiceContainer.Resolve<IPolicyService>("policyService");
_configService = ServiceContainer.Resolve<IConfigService>();
_environmentService = ServiceContainer.Resolve<IEnvironmentService>();
_logger = ServiceContainer.Resolve<ILogger>("logger");

Loading = true;
Expand Down Expand Up @@ -104,6 +109,7 @@ public GroupingsPageViewModel()
public List<Core.Models.View.CollectionView> Collections { get; set; }
public List<TreeNode<Core.Models.View.CollectionView>> NestedCollections { get; set; }

public AppOptions AppOptions { get; internal set; }
protected override ICipherService cipherService => _cipherService;
protected override IPolicyService policyService => _policyService;
protected override IOrganizationService organizationService => _organizationService;
Expand Down Expand Up @@ -699,5 +705,59 @@ private List<FolderView> BuildFolders(List<FolderView> decFolders)
var folders = decFolders.Where(f => _allCiphers.Any(c => c.FolderId == f.Id)).ToList();
return folders.Any() ? folders : null;
}

internal async Task CheckOrganizationUnassignedItemsAsync()
{
try
{
if (AppOptions?.HasJustLoggedInOrUnlocked != true)
{
return;
}

AppOptions.HasJustLoggedInOrUnlocked = false;

if (!await _configService.GetFeatureFlagBoolAsync(Core.Constants.UnassignedItemsBannerFlag)
||
!await _stateService.GetShouldCheckOrganizationUnassignedItemsAsync())
{
return;
}

var waitSyncTask = Task.Run(async () =>
{
while (_syncService.SyncInProgress)
{
await Task.Delay(100);
}
});
await waitSyncTask.WaitAsync(TimeSpan.FromMinutes(5));

if (!await _cipherService.VerifyOrganizationHasUnassignedItemsAsync())
{
return;
}

var message = _environmentService.SelectedRegion == Core.Enums.Region.SelfHosted
? AppResources.OrganizationUnassignedItemsMessageSelfHostDescriptionLong
: AppResources.OrganizationUnassignedItemsMessageUSEUDescriptionLong;

var response = await _deviceActionService.DisplayAlertAsync(AppResources.Notice,
message,
null,
AppResources.RemindMeLater,
AppResources.Ok);

if (response == AppResources.Ok)
{
await _stateService.SetShouldCheckOrganizationUnassignedItemsAsync(false);
}
}
catch (TimeoutException) { }
catch (Exception ex)
{
_logger.Exception(ex);
}
}
}
}
36 changes: 36 additions & 0 deletions src/Core/Resources/Localization/AppResources.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 12 additions & 0 deletions src/Core/Resources/Localization/AppResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -2886,4 +2886,16 @@ Do you want to switch to this account?</value>
<data name="LaunchDuo" xml:space="preserve">
<value>Launch Duo</value>
</data>
<data name="OrganizationUnassignedItemsMessageUSEUDescriptionLong" xml:space="preserve">
<value>Unassigned organization items are no longer visible in the All Vaults view and only accessible via the Admin Console. Assign these items to a collection from the Admin Console to make them visible.</value>
</data>
<data name="OrganizationUnassignedItemsMessageSelfHostDescriptionLong" xml:space="preserve">
<value>On May 2, 2024, unassigned organization items will no longer be visible in the All Vaults view and only accessible via the Admin Console. Assign these items to a collection from the Admin Console to make them visible.</value>
</data>
<data name="RemindMeLater" xml:space="preserve">
<value>Remind me later</value>
</data>
<data name="Notice" xml:space="preserve">
<value>Notice</value>
</data>
</root>
5 changes: 5 additions & 0 deletions src/Core/Services/ApiService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,11 @@ public Task<CipherResponse> PutRestoreCipherAsync(string id)
return SendAsync<object, CipherResponse>(HttpMethod.Put, string.Concat("/ciphers/", id, "/restore"), null, true, true);
}

public Task<bool> HasUnassignedCiphersAsync()
{
return SendAsync<object, bool>(HttpMethod.Get, "/ciphers/has-unassigned-ciphers", null, true, true);
}

#endregion

#region Attachments APIs
Expand Down
18 changes: 18 additions & 0 deletions src/Core/Services/CipherService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -829,6 +829,24 @@ public async Task RestoreWithServerAsync(string id)
await ClearCacheAsync();
}

public async Task<bool> VerifyOrganizationHasUnassignedItemsAsync()
{
var organizations = await _stateService.GetOrganizationsAsync();
Copy link
Member

Choose a reason for hiding this comment

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

🤔 I'm not sure if it's possible for this to return 0 orgs and the next call returning true, I just noticed other clients aren't checking this.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is exactly why I had to wait for synchronization to complete, otherwise we'd have 0 orgs right after logging in given the sync wouldn't have finished.
I could remove this alongside the sync wait but it will fire a request to the server for users that don't have organizations which IMO is a waste of resources and given that we're waiting for sync to finish we should have the latest organizations state when checking it.

if (organizations?.Any() != true)
{
return false;
}

try
{
return await _apiService.HasUnassignedCiphersAsync();
}
catch (ApiException ex) when (ex.Error?.StatusCode == System.Net.HttpStatusCode.BadRequest)
{
return false;
}
}

// Helpers

private async Task<Tuple<SymmetricCryptoKey, EncString, SymmetricCryptoKey>> MakeAttachmentKeyAsync(string organizationId, Cipher cipher = null, CipherView cipherView = null)
Expand Down
10 changes: 10 additions & 0 deletions src/Core/Services/StateService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1384,6 +1384,16 @@ public async Task SetPreAuthRegionAsync(BwRegion value)
await _storageMediatorService.SaveAsync(Constants.RegionEnvironment, value);
}

public async Task<bool> GetShouldCheckOrganizationUnassignedItemsAsync(string userId = null)
{
return await _storageMediatorService.GetAsync<bool?>(await ComposeKeyAsync(Constants.ShouldCheckOrganizationUnassignedItemsKey, userId)) ?? true;
}

public async Task SetShouldCheckOrganizationUnassignedItemsAsync(bool shouldCheck, string userId = null)
{
await _storageMediatorService.SaveAsync<bool?>(await ComposeKeyAsync(Constants.ShouldCheckOrganizationUnassignedItemsKey, userId), shouldCheck);
}

// Helpers

[Obsolete("Use IStorageMediatorService instead")]
Expand Down
2 changes: 1 addition & 1 deletion src/Core/Utilities/ServiceContainer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ public static void Init(string customUserAgent = null, string clearCipherCacheKe
var settingsService = new SettingsService(stateService);
var fileUploadService = new FileUploadService(apiService);
var configService = new ConfigService(apiService, stateService, logger);
var environmentService = new EnvironmentService(apiService, stateService, conditionedRunner);
var cipherService = new CipherService(cryptoService, stateService, settingsService, apiService,
fileUploadService, storageService, i18nService, () => searchService, configService, clearCipherCacheKey,
allClearCipherCacheKeys);
Expand Down Expand Up @@ -87,7 +88,6 @@ public static void Init(string customUserAgent = null, string clearCipherCacheKe
keyConnectorService, passwordGenerationService, policyService, deviceTrustCryptoService, passwordResetEnrollmentService);
var exportService = new ExportService(folderService, cipherService, cryptoService);
var auditService = new AuditService(cryptoFunctionService, apiService);
var environmentService = new EnvironmentService(apiService, stateService, conditionedRunner);
var eventService = new EventService(apiService, stateService, organizationService, cipherService);
var usernameGenerationService = new UsernameGenerationService(cryptoService, apiService, stateService);

Expand Down