UI for blacklisting unverified devices per-room & globally #636

ara4n · 2017-01-22T00:34:14Z

This should solve element-hq/element-web#2313, modulo hitting up against the regression in element-hq/element-web#3020.

It builds on top of #635, which should land first for sensible review.

Does not work until Async dialog loader breaks ability to verify devices element-hq/element-web#3020 is fixed.

(written blind; untested as yet)

ara4n · 2017-01-23T18:31:57Z

(hm, even once #3020 is fixed, which I guess it now is, it'll probably need Dialog to support stacks of dialogs so that the user can jump in & out of verifying/blocking devices nicely)

richvdh · 2017-01-23T18:41:11Z

Ok. I have that on an unmerged branch, so I can land that.

…

On 23 January 2017 18:31:57 GMT+00:00, Matthew Hodgson ***@***.***> wrote: (hm, even once #3020 is fixed, which I guess it now is, it'll probably need Dialog to support stacks of dialogs so that the user can jump in & out of verifying/blocking devices nicely) -- You are receiving this because you were assigned. Reply to this email directly or view it on GitHub: #636 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

…verified

richvdh · 2017-01-26T14:33:42Z

Ok, some thoughts on this:

Firstly this PR mixes two concepts. Let's tale them separately.

Firstly, it adds the verify/blacklist buttons to the UnknownDeviceDialog.

This seems a good idea
I landed Allow modals to stack up #649, which means we get to go back to the old dialog - but by then the scroll offset has been reset. I guess we need to keep the old dialog open.
We need to fix Big modal dialogs randomly scroll to the bottom when loaded element-hq/element-web#3018
We need to update the list as people blacklist/verify the devices; there is no feedback that you have been successful. Indeed, we probably want to regenerate the device list instead of taking the list from the UnknownDeviceException.

Second, it includes the UI for twiddling the global blacklist setting. This is ok - though presumably in this case we will never see the UnknownDeviceDialog, so there's no point handling it specially?

ara4n · 2017-01-26T18:56:47Z

sounds plausible on all counts. sorry for mixing the two issues.

richvdh · 2017-01-31T20:53:59Z

currently blocked on #635, which is with @ara4n

ara4n · 2017-02-02T21:45:37Z

no longer blocked on #635. @richvdh, PTAL as the UnkDeviceDialog is pretty crap without this..

ara4n · 2017-02-02T21:56:08Z

(just merging)

ara4n · 2017-02-02T22:09:49Z

having just tried this, there are some minor problems:

Blacklist button doesn't work
Actually verifying (or blacklisting) devices doesn't seem to update the device list
it really feels like there should be some kind of way to bring up this dialog at any point, to help manage en-masse all the devices in the room (and show the known devices to, so you have a full dashboard), otherwise you are just a bit blind when going and verifying/blacklisting the listed devices.

Hum.

richvdh · 2017-02-02T22:50:04Z

it really feels like there should be some kind of way to bring up this dialog at any point, to help manage en-masse all the devices in the room (and show the known devices to, so you have a full dashboard), otherwise you are just a bit blind when going and verifying/blacklisting the listed devices.

That would imply we need to rethink what it means to be "known".

ara4n · 2017-02-03T01:52:59Z

i've fixed the verify/blacklist UI updating problem in the above commit.
in terms of whether the dialog should be displayable on demand... yes. probably by clicking on the padlock or something. but i think that can be a separate PR and isn't so critical.

@richvdh PTAL.

richvdh · 2017-02-03T10:29:30Z

src/UserSettingsStore.js

@@ -149,6 +149,23 @@ module.exports = {
        return MatrixClientPeg.get().setAccountData("im.vector.web.settings", settings);
    },

+    getLocalSettings: function() {


it would be good to explain wtf a 'local setting' is. Turns out, it's a setting which is persisted in localstorage.

richvdh · 2017-02-03T10:30:02Z

src/UserSettingsStore.js

+        return JSON.parse(localSettingsString);
+    },
+
+    getLocalSetting: function(type, defaultValue = null) {


type? why type?

richvdh · 2017-02-03T10:30:27Z

src/UserSettingsStore.js

+        return JSON.parse(localSettingsString);
+    },
+
+    getLocalSetting: function(type, defaultValue = null) {


comments on what the behaviour is if the setting is absent would be nice.

richvdh · 2017-02-03T10:31:07Z

src/UserSettingsStore.js

@@ -149,6 +149,23 @@ module.exports = {
        return MatrixClientPeg.get().setAccountData("im.vector.web.settings", settings);
    },

+    getLocalSettings: function() {
+        var localSettingsString = localStorage.getItem('mx_local_settings') || '{}';


ought to handle localStorage === undefined here

richvdh · 2017-02-03T10:38:57Z

src/components/structures/UserSettings.js

+    _renderLocalSetting: function(setting) {
+        const client = MatrixClientPeg.get();
+        return <div className="mx_UserSettings_toggle" key={ setting.id }>
+            <input id={ setting.id }


this is crying out to be made a separate react component.

Don't enable encryption unless the box is checked!

richvdh · 2017-02-03T11:22:03Z

src/components/views/rooms/RoomSettings.js

+        var isGlobalBlacklistUnverified = UserSettingsStore.getLocalSettings().blacklistUnverifiedDevices;
+        var isRoomBlacklistUnverified = this._isRoomBlacklistUnverified();
+
+        var settings =


please can we put parens around multi-line jsx?

richvdh

A bunch of stylistic nits which I'd love to see fixed, but realistically we want this in a release today. I've fixed the visible problems, so will land this

ara4n added 7 commits January 21, 2017 17:39

UI for blacklisting unverified devices per-room & globally

3071fc0

(written blind; untested as yet)

oops

c618880

improve the verify warning if blacklisting is unabled

071e364

literally blindly add verification buttons

532f4e5

fix local storage idiocy

39c122f

make it work

d9c0513

make UnknownDeviceDialog work again, other than the mess of element-h…

7bc3fc8

…q/element-web#3020

ara4n assigned richvdh Jan 22, 2017

This was referenced Jan 22, 2017

Support for blacklisting unverified devices, both per-room and globally matrix-org/matrix-js-sdk#336

Merged

Consider changing block/blacklist device to be "reject device", with associated UX improvements element-hq/element-web#3022

Open

richvdh added 2 commits January 26, 2017 14:09

Merge branch 'matthew/warn-unknown-devices' into matthew/blacklist-un…

67bd2cf

…verified

Fix lint

0a3d9fc

richvdh assigned ara4n and unassigned richvdh Jan 31, 2017

ara4n assigned richvdh and unassigned ara4n Feb 2, 2017

merge

be41462

ara4n added 2 commits February 3, 2017 01:49

tweak verbiage

5de84f8

correctly reflect verify/blacklist state in UI

28d28b5

richvdh reviewed Feb 3, 2017

View reviewed changes

richvdh added 2 commits February 3, 2017 10:50

UnknownDeviceDialog.js: tweak warning

e018eef

Fix RoomSettings

3ea746d

Don't enable encryption unless the box is checked!

richvdh reviewed Feb 3, 2017

View reviewed changes

richvdh approved these changes Feb 3, 2017

View reviewed changes

richvdh merged commit 2b67c12 into develop Feb 3, 2017

turt2live mentioned this pull request Nov 4, 2017

"Never send to unverified devices" option does not persist page reloads element-hq/element-web#5516

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI for blacklisting unverified devices per-room & globally #636

UI for blacklisting unverified devices per-room & globally #636

ara4n commented Jan 22, 2017 •

edited by richvdh

Loading

ara4n commented Jan 23, 2017

richvdh commented Jan 23, 2017 via email

richvdh commented Jan 26, 2017

ara4n commented Jan 26, 2017

richvdh commented Jan 31, 2017

ara4n commented Feb 2, 2017

ara4n commented Feb 2, 2017

ara4n commented Feb 2, 2017 •

edited

Loading

richvdh commented Feb 2, 2017

ara4n commented Feb 3, 2017

richvdh Feb 3, 2017

richvdh Feb 3, 2017

richvdh Feb 3, 2017

richvdh Feb 3, 2017

richvdh Feb 3, 2017

richvdh Feb 3, 2017

richvdh left a comment

UI for blacklisting unverified devices per-room & globally #636

UI for blacklisting unverified devices per-room & globally #636

Conversation

ara4n commented Jan 22, 2017 • edited by richvdh Loading

ara4n commented Jan 23, 2017

richvdh commented Jan 23, 2017 via email

richvdh commented Jan 26, 2017

ara4n commented Jan 26, 2017

richvdh commented Jan 31, 2017

ara4n commented Feb 2, 2017

ara4n commented Feb 2, 2017

ara4n commented Feb 2, 2017 • edited Loading

richvdh commented Feb 2, 2017

ara4n commented Feb 3, 2017

richvdh Feb 3, 2017

Choose a reason for hiding this comment

richvdh Feb 3, 2017

Choose a reason for hiding this comment

richvdh Feb 3, 2017

Choose a reason for hiding this comment

richvdh Feb 3, 2017

Choose a reason for hiding this comment

richvdh Feb 3, 2017

Choose a reason for hiding this comment

richvdh Feb 3, 2017

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

ara4n commented Jan 22, 2017 •

edited by richvdh

Loading

ara4n commented Feb 2, 2017 •

edited

Loading