Follow OrgRules to forbid backups keys by ekievsky · Pull Request #376 · FlowCrypt/flowcrypt-ios

ekievsky · 2021-07-03T15:48:41Z

This PR:
Skipping backups searching on Setup and Settings if OrgRules has canBackupKeys false;
Handling unknown ClientConfigurationFlag;
Added tests for unknown ClientConfigurationFlag;
Waiting for OrgRules to be fetched on Startup;

close #277

Tests:

Tests added or updated

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

addresses the issue it closes (if any)
code is readable and understandable
is accompanied with tests, or tests are not needed
is free of vulnerabilities
is documented clearly and usefully, or doesn't need documentation

…ackupKeys false; Handling unknown ClientConfigurationFlag; Added tests for unknown ClientConfigurationFlag; Waiting for OrgRules to be fetched on Startup;

ekievsky · 2021-07-03T15:51:59Z

@tomholub The build fails on CI and if I open SemaphoreCI it shows me empty screen with Secret flowcrypt-ios-ci-secrets not found message, is it internal Semaphore issue?

Kharchevskyi · 2021-07-03T16:06:26Z

same for https://flowcrypt.semaphoreci.com/workflows/183fcbb7-7dec-4c55-aa82-680e9f0beb39

Kharchevskyi · 2021-07-03T16:09:35Z

@tomholub we do not run ui tests on CI. Maybe we can remove this step from CI flow?

ekievsky · 2021-07-06T14:36:52Z

Hi @tomholub , which task should I take next?

tomholub · 2021-07-06T15:17:16Z

issue 359 please - thanks. later i’ll assign more

…

On Tuesday, July 6, 2021, Evgenii Kievsky ***@***.***> wrote: Hi @tomholub <https://github.com/tomholub> , which task should I take next? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#376 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABQDZEJBKWLQCIPI3VEV75TTWMIJBANCNFSM47YIDXJQ> .

-- -- Tom James Holub <http://holub.me/>

…d-backups

tomholub

Thank you for the PR - see my comments.

Some of the naming choices were not ideal already in the code before your PR, but got highlighted now - so we'll do a bit of refactoring per my comments.

FlowCrypt/Controllers/Settings/Settings List/SettingsViewController.swift

FlowCrypt/Controllers/Setup/SetupInitialViewController.swift

FlowCrypt/Functionality/Services/Organisational Rules Service/OrganisationalRulesService.swift

FlowCrypt/Models/ClientConfiguration.swift

…d-backups

ekievsky · 2021-07-08T20:09:48Z

@tomholub there could be error with JSON parsing and wrong user domain in EnterpriseServerApi should we show error on startup with error description or just create OrgRules with empty ClientConfig?

tomholub · 2021-07-08T20:26:30Z

@tomholub there could be error with JSON parsing and wrong user domain in EnterpriseServerApi should we show error on startup with error description or just create OrgRules with empty ClientConfig?

Thank you for asking. Absolutely have to show error to the user, never default to empty ClientConfig to resolve errors.

Additionally:

If this error happened when we already had OrgRules saved before (we are refreshing them), then leave already saved OrgRules as they are
if this error happens during setup flow - before any OrgRules were saved before - we cannot proceed setup at all.

Only default to empty ClientConfig on gmail.com domain and similar where we know there are no OrgRules to fetch.

In general, I want errors to be handled "harshly" - either crash the app if it's never expected (like a value that should never be nil but somehow is), or show the error to the user if it's some errors that could conceivably happen (like a network error, or badly formatted external json like here).

We do a lot of "soft" error handling in the app with something ?? "" and similar and I dislike it a lot, because errors don't get properly discovered where they happen, and instead they either blow up somewhere else (best case) or go completely unnoticed (worst case), so I'd like to move away from doing that.

Made OrganisationalRules not optional; Fixed PR comments;

tomholub · 2021-07-09T13:52:21Z

FlowCrypt/Functionality/DataManager/Encrypted Storage/EncryptedStorage.swift

                storage.delete(userToDelete)
+                storage.delete(clientConfigurations)


@Kharchevskyi does the order of items matter here? If there are some foreign keys then we probably need to delete the user last?

I think in this case - yes.
ClientConfigurationObject has userObject as a reference.
To be sure please check if this cause a crash on log out when there are 2 accounts logged in.

tomholub · 2021-07-09T13:53:37Z

FlowCrypt/Functionality/Services/EnterpriseServerApi.swift

@@ -12,8 +12,21 @@ protocol EnterpriseServerApiType {
    func getActiveFesUrl(for email: String) -> Promise<String?>


Changes in this file are very good, thanks

tomholub · 2021-07-09T13:55:07Z

FlowCrypt/Functionality/Services/Organisational Rules Service/OrganisationalRulesService.swift

-            return nil
+    func getSavedOrganisationalRulesForCurrentUser() -> OrganisationalRules {
+        guard let configuration = self.clientConfigurationProvider.fetch() else {
+            assertionFailure("There should not be a user without OrganisationalRules")


Does assertionFailure also work for production builds?

…d-backups

Skipping backups searching on Setup and Settings if OrgRules has canB…

85b4d29

…ackupKeys false; Handling unknown ClientConfigurationFlag; Added tests for unknown ClientConfigurationFlag; Waiting for OrgRules to be fetched on Startup;

Kharchevskyi mentioned this pull request Jul 6, 2021

Secret flowcrypt-ios-ci-secrets not found #378

Closed

Merge branch 'master' into feature/issue-277-follow-orgrules-to-forbi…

dec96d6

…d-backups

tomholub reviewed Jul 8, 2021

View reviewed changes

Merge branch 'master' into feature/issue-277-follow-orgrules-to-forbi…

d08dd2a

…d-backups

tomholub marked this pull request as draft July 8, 2021 18:43

tomholub and others added 2 commits July 9, 2021 02:43

Merge branch 'master' into feature/issue-277-follow-orgrules-to-forbi…

f1bba06

…d-backups

Fixed PR comments;

474ca42

Changed error handling for EnterpriseServerApiType;

58ea373

Made OrganisationalRules not optional; Fixed PR comments;

ekievsky marked this pull request as ready for review July 9, 2021 09:09

tomholub approved these changes Jul 9, 2021

View reviewed changes

tomholub added 2 commits July 9, 2021 21:55

Merge branch 'master' into feature/issue-277-follow-orgrules-to-forbi…

a3c6a6c

…d-backups

Merge branch 'master' into feature/issue-277-follow-orgrules-to-forbi…

20b1ec9

…d-backups

This was referenced Jul 9, 2021

fix order of db deletions during logout #386

Closed

replace all fatalError with throw #387

Closed

tomholub enabled auto-merge (squash) July 9, 2021 14:52

tomholub merged commit d737ff6 into master Jul 9, 2021

tomholub deleted the feature/issue-277-follow-orgrules-to-forbid-backups branch July 9, 2021 15:05

		storage.delete(userToDelete)
		storage.delete(clientConfigurations)

		@@ -12,8 +12,21 @@ protocol EnterpriseServerApiType {
		func getActiveFesUrl(for email: String) -> Promise<String?>

Conversation

ekievsky commented Jul 3, 2021 • edited by tomholub Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To be filled by reviewers

Uh oh!

ekievsky commented Jul 3, 2021

Uh oh!

Kharchevskyi commented Jul 3, 2021

Uh oh!

Kharchevskyi commented Jul 3, 2021

Uh oh!

ekievsky commented Jul 6, 2021

Uh oh!

tomholub commented Jul 6, 2021 via email

Uh oh!

tomholub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ekievsky commented Jul 8, 2021

Uh oh!

tomholub commented Jul 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomholub Jul 9, 2021

Choose a reason for hiding this comment

Uh oh!

Kharchevskyi Jul 9, 2021

Choose a reason for hiding this comment

Uh oh!

tomholub Jul 9, 2021

Choose a reason for hiding this comment

Uh oh!

tomholub Jul 9, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ekievsky commented Jul 3, 2021 •

edited by tomholub

Loading

tomholub commented Jul 8, 2021 •

edited

Loading