Feature/do not sync enc folders if e2ee is not setup #5258

allexzander · 2022-12-08T16:25:01Z

A screen recording of how this feature works now follows.

e2e_do_not_sync_encrypted_folders_without_mnemonic_entered.mp4

allexzander · 2022-12-08T17:26:59Z

@tobiasKaminsky See the video in the description

allexzander · 2022-12-08T17:30:02Z

@nimishavijay @jancborchardt What do you think? Qt widgets are not really that customizable compared to QML, so, I did what was possible without adding too much code. Any critical remarks?

src/gui/accountsettings.cpp

claucambra

A few comments but nothing major

src/gui/accountsettings.cpp

claucambra · 2022-12-15T10:58:32Z

src/gui/accountsettings.cpp

+    }
+}
+
+void AccountSettings::updateBlackListAndScheduleFolderSync(const QStringList &blackList, OCC::Folder *folder, const QStringList &foldersToRemoveFromBlacklist)


This method could be const

src/gui/accountsettings.cpp

src/gui/folderstatusmodel.cpp

src/libsync/discovery.cpp

src/gui/folderstatusmodel.cpp

codecov · 2022-12-15T11:50:37Z

Codecov Report

Merging #5258 (2cf877d) into master (a21df5b) will decrease coverage by 0.06%.
The diff coverage is 10.00%.

❗ Current head 2cf877d differs from pull request most recent head 867249f. Consider uploading reports for the commit 867249f to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5258      +/-   ##
==========================================
- Coverage   57.70%   57.64%   -0.07%     
==========================================
  Files         139      139              
  Lines       17577    17596      +19     
==========================================
  Hits        10143    10143              
- Misses       7434     7453      +19

Impacted Files	Coverage Δ
src/common/syncjournaldb.h	`66.66% <ø> (ø)`
src/csync/csync_exclude.h	`0.00% <ø> (ø)`
src/libsync/discovery.h	`59.25% <ø> (ø)`
src/libsync/discovery.cpp	`85.93% <10.00%> (-1.61%)`	⬇️

claucambra

Tiny comments :)

claucambra · 2022-12-15T12:10:38Z

src/gui/accountsettings.cpp

+        return;
+    }
+    // in case sync is already running - terminate it and start a new one
+    const QMetaObject::Connection syncTerminatedConnection = connect(folder, &Folder::syncFinished, this, [this, blackList, folder, foldersToRemoveFromBlacklist]() {


actually, const auto?

claucambra · 2022-12-15T12:11:05Z

src/gui/folderstatusmodel.cpp

@@ -153,41 +155,49 @@ QVariant FolderStatusModel::data(const QModelIndex &index, int role) const
        return QVariant();
    }
    case SubFolder: {
-        const auto &x = static_cast<SubFolderInfo *>(index.internalPointer())->_subs.at(index.row());
-        const auto supportsSelectiveSync = x._folder && x._folder->supportsSelectiveSync();
+        const auto &subfolderInfo = static_cast<SubFolderInfo *>(index.internalPointer())->_subs.at(index.row());


Awesome, so much better :D

nimishavijay · 2022-12-15T12:42:24Z

Looks really great!

I like the flow, it is really nice and intuitive! Some feedback about visuals:

It took me a second to find out that not all folders are synced as I immediately saw only the big green checkmark icon and missed the e2e infobox shown above it.
What do you think about moving the e2e infobox from above the sync information to inside the sync information, just above the folders list?
The background of the e2e infobox could be changed to yellow if e2e is not enabled to indicate a warning. We can also change the icon to the striked out lock that is used in the folders list.
I'm also thinking that it makes sense to change the big green checkmark icon to a warning or error icon if e2e is not enabled as not all folders that have been added are synced. Would that be a bit much?
If e2e is not enabled, the red striked out lock icon next to folder can be shown regardless of whether or not that folder is selected. And it can be green when e2e is enabled. I would also say that the "Could not decrypt" string can be removed because I missed it the first few times and I still understood the flow :)
"Display mnemonic" --> "Show passphrase"
"End-to-end encryption has been enabled for this account." (add full stop at the end of the sentence)

What do you think? :)

…tomatically blacklist them. Remove from blacklist once the E2EE mnemonic is provided. Signed-off-by: alex-z <blackslayer4@gmail.com>

nextcloud-desktop-bot · 2022-12-15T14:20:02Z

AppImage file: nextcloud-PR-5258-867249f339cc819a0f912acfa59fd02f74c8b407-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

sonarqubecloud · 2022-12-15T14:34:38Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
18 Code Smells

1.1% Coverage
0.0% Duplication

allexzander · 2022-12-15T14:48:00Z

@nimishavijay Apologies, I have only noticed your feedback after already merging it (we were in a bit of a rush to put it in). But, I guess most of your feedback can be implemented and is not very hard. Having said that, could you maybe create a new issue with your feedback in the description, we are going to implement it later as we will keep improving E2EE in the coming weeks.

allexzander force-pushed the feature/do-not-sync-enc-folders-if-e2ee-is-not-setup branch from 53185f8 to e4063a7 Compare December 8, 2022 17:21

allexzander marked this pull request as ready for review December 8, 2022 17:22

allexzander requested review from jancborchardt, nimishavijay, claucambra and mgallien December 8, 2022 17:26

claucambra reviewed Dec 8, 2022

View reviewed changes

src/gui/accountsettings.cpp Outdated Show resolved Hide resolved

claucambra reviewed Dec 9, 2022

View reviewed changes

src/gui/accountsettings.cpp Outdated Show resolved Hide resolved

src/gui/accountsettings.cpp Outdated Show resolved Hide resolved

src/gui/accountsettings.cpp Outdated Show resolved Hide resolved

allexzander force-pushed the feature/do-not-sync-enc-folders-if-e2ee-is-not-setup branch from 36ee635 to 33640b4 Compare December 12, 2022 17:40

allexzander requested a review from claucambra December 12, 2022 17:41

allexzander force-pushed the feature/do-not-sync-enc-folders-if-e2ee-is-not-setup branch 2 times, most recently from 540f7c2 to 6190d8d Compare December 15, 2022 09:37

claucambra reviewed Dec 15, 2022

View reviewed changes

allexzander force-pushed the feature/do-not-sync-enc-folders-if-e2ee-is-not-setup branch from 6190d8d to 022512e Compare December 15, 2022 11:15

allexzander requested a review from claucambra December 15, 2022 11:39

claucambra reviewed Dec 15, 2022

View reviewed changes

claucambra approved these changes Dec 15, 2022

View reviewed changes

allexzander force-pushed the feature/do-not-sync-enc-folders-if-e2ee-is-not-setup branch from 55a6819 to 2cf877d Compare December 15, 2022 13:58

Do not sync encrypted folders if the E2EE mnemonic is not entered. Au…

867249f

…tomatically blacklist them. Remove from blacklist once the E2EE mnemonic is provided. Signed-off-by: alex-z <blackslayer4@gmail.com>

allexzander force-pushed the feature/do-not-sync-enc-folders-if-e2ee-is-not-setup branch from 2cf877d to 867249f Compare December 15, 2022 14:09

allexzander merged commit f6cfb65 into master Dec 15, 2022

allexzander deleted the feature/do-not-sync-enc-folders-if-e2ee-is-not-setup branch December 15, 2022 14:44

nimishavijay mentioned this pull request Dec 15, 2022

Improve visuals in e2ee setup and usage #5276

Open

mgallien added this to the 3.7.0 milestone Jan 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/do not sync enc folders if e2ee is not setup #5258

Feature/do not sync enc folders if e2ee is not setup #5258

allexzander commented Dec 8, 2022 •

edited

Loading

allexzander commented Dec 8, 2022

allexzander commented Dec 8, 2022

claucambra left a comment

claucambra Dec 15, 2022

codecov bot commented Dec 15, 2022 •

edited

Loading

claucambra left a comment

claucambra Dec 15, 2022

claucambra Dec 15, 2022

nimishavijay commented Dec 15, 2022

nextcloud-desktop-bot commented Dec 15, 2022

sonarqubecloud bot commented Dec 15, 2022

allexzander commented Dec 15, 2022

Feature/do not sync enc folders if e2ee is not setup #5258

Feature/do not sync enc folders if e2ee is not setup #5258

Conversation

allexzander commented Dec 8, 2022 • edited Loading

allexzander commented Dec 8, 2022

allexzander commented Dec 8, 2022

claucambra left a comment

Choose a reason for hiding this comment

claucambra Dec 15, 2022

Choose a reason for hiding this comment

codecov bot commented Dec 15, 2022 • edited Loading

Codecov Report

claucambra left a comment

Choose a reason for hiding this comment

claucambra Dec 15, 2022

Choose a reason for hiding this comment

claucambra Dec 15, 2022

Choose a reason for hiding this comment

nimishavijay commented Dec 15, 2022

nextcloud-desktop-bot commented Dec 15, 2022

sonarqubecloud bot commented Dec 15, 2022

allexzander commented Dec 15, 2022

allexzander commented Dec 8, 2022 •

edited

Loading

codecov bot commented Dec 15, 2022 •

edited

Loading