element-hq / user-stories

A place to write user stories for planned cross-platform enhancements.
4 stars 2 forks source link

As a user I want to use widgets set up by other users if and only if I make an informed decision to give those widgets permission to load so that I can make use of widgets without losing control of my personal data. #7

Open lampholder opened 5 years ago

lampholder commented 5 years ago

IMPLEMENTATION THOUGHTS:

KNOWN ISSUES:

lampholder commented 5 years ago
lampholder commented 5 years ago

Flagging this needs-product-design until we've resolved the following outstanding questions

Outstanding Questions

lampholder commented 5 years ago

My thoughts:

@manuroe and @turt2live think users won't be happy to accept each widget, so would prefer a heuristic

I'm not convinced this is a problem (or rather, if it is, I'm not convinced it's one that's worth our trying to fix now). It's literally better than the behaviour we have on Riot Web today (which prompts once per widget instance per session), and Riot Web is the only platform on which people are using widgets.

If we did want to improve this, we could extend the option set with a couple of additional options:

What if the URL changes?

A change in URL should require the user to give permission afresh. A different URL, IIUC, can pull different data about the user, and can (obviously) share it with a totally different location, so this would require giving permission again.

How does revocation work?

As it does today IIUC - click a top-level UX element and you have revoked permission for the widget to load. @turt2live I know you're not wild about the UX for this in Riot Web, but I'd like us to treat that separately if we can.

Does any of this change if you created the widget? and What if you're not the creator, but the editor?

This is tricky - I'd been assuming that we would automatically load widgets we created (it's good to tease out all the assumptions), but I hadn't realised other users could edit them. Would it work to approve created widgets at creation time, then if somebody else should edit them you would be forced to give permission again? (I'm assuming we'd store a hash of the key identifying factors in place of a boolean permission value to achieve this). And likewise the act of editing could cause you to approve it. To round this out it would be good if the IM told the provisioner what permissions the app was going to have.

What if the widget disappears and comes back?

I guess this raises the question "is the event ID part of the identifying set of features which gets hashed to represent agreement to the loading of this widget instance". I'm kinda neutral - it seems like an unlikely vector for abuse and also very unlikely to occur often, so if the event id were part of the hash and you needed to reapprove the widget when it 'returned' I think that would be fine.

Is this allowed widget opening list signed by the user or could a malicious homeserver change it and opt you in to widgets?

It's not signed - your homeserver can do worse than opt you in to widgets if it is acting maliciously.

nadonomy commented 5 years ago
  • The load-permission UX will tell me:

    • who added the widget

I assume a combination of profile pic, display name & matrix id is what we want, to combat phishing & increase confidence in who set up a widget?

  • who provides the widget (might only be a URL)

👍

  • what data will be shared if I agree to open it

Do you have a list of the data that could potentially be shared? How broad this is will inform the best way to visualise it.

  • I only have to give each widget instance permission once (if the widget is removed and re-added, or if the same widget is in a different room, I will have to give permission again)

👍

  • My giving permission to the widget to load is persisted and honoured across all Riot clients/sessions

👍

  • I can revoke this permission if I change my mind by interacting with a top-level widget UX element

I think the best thing to do here is to keep the trash can icon & rename the tooltip to 'Remove for me'. Removing the widget should entirely remove its chrome from the room view, and users can re-add the rooms widget via room settings.

If it's the user that set up the widget, then the tooltip & interaction could simply change to 'Remove from this room'. I don't think it's wise to support the use case of "I set up a widget for a room, and want to personally remove it but have the widget persist in the room".

lampholder commented 5 years ago

I think the best thing to do here is to keep the trash can icon & rename the tooltip to 'Remove for me'. Removing the widget should entirely remove its chrome from the room view, and users can re-add the rooms widget via room settings.

I think we should leave this behaviour as it is today. We can improve later if we want to, but I just want us to achieve stable, consistent behaviour as quickly as possible without further scope creep (I don't want us to have to design "readd hidden widgets" UX in room settings, for example, not to mention the discovery problems I fear we'd introduce).

nadonomy commented 5 years ago

I think the best thing to do here is to keep the trash can icon & rename the tooltip to 'Remove for me'. Removing the widget should entirely remove its chrome from the room view, and users can re-add the rooms widget via room settings.

I think we should leave this behaviour as it is today. We can improve later if we want to, but I just want us to achieve stable, consistent behaviour as quickly as possible without further scope creep (I don't want us to have to design "readd hidden widgets" UX in room settings, for example, not to mention the discovery problems I fear we'd introduce).

Ok— on reflection, I agree with you. (I wrote that comment with a slightly different understanding of today's UX, but just benchmarked now to confirm.)

lampholder commented 5 years ago

I think the questions are answered (and the story has been updated with the conclusions), so I'm tentatively removing needs-product-design and re-adding needs-technical-design to flag that we need a technical design for the allowed widget load list that only grants permission to the specific widget instance in its specific configuration. @turt2live are you happy to take a look at this?

turt2live commented 5 years ago

@lampholder see https://github.com/vector-im/riot-meta/pull/311

dbkr commented 5 years ago

From https://github.com/vector-im/riot-meta/pull/311#issuecomment-549470524 - what information are we giving to the user for them to make a decision? It implies the user will have to understand the data being given to the widget, given that custom widgets will depends on what the curl value is as to what site they load.

turt2live commented 5 years ago

(note also that curl is non-standard - it should be url)

lampholder commented 5 years ago

Right, sounds like we're agreed on the UX flow and the technical decisions that back it (modulo the caveats now described in the first comment), so I'm going to remove the needs-technical-design blocker.

turt2live commented 5 years ago

Implementations: DO NOT use the creatorUserId in the widget's content. Use the event sender for the profile information instead.

turt2live commented 5 years ago

@lampholder some questions:

turt2live commented 5 years ago

for the revoke button:

we're going to use a context menu on riot-web because it's easier. Options are:

Copy can be argued.

lampholder commented 5 years ago

@lampholder some questions:

* The scope in the top post implies that the user who added the widget is going to get prompted. Is this intentional?

I've added a line at the bottom saying permission to load is implied if I add the widget - does that clarify sufficiently?

* Is the widget URL going to be the unwrapped or wrapped URL of the widget? If unwrapped, what are the conditions for unwrapping?

I discussed this with Nad and we agreed that, if the widget URL were wrapped and could be unwrapped (I believe this is only possible when the widget is wrapped by the integration manager your riot client has been configured to use), then the text would read something like:

(If it's not wrapped or is wrapped by an integration manager of which your riot instance isn't aware, it'll just say something like "Using this widget will share data with widget.com)

Hopefully this should be redundant and represented in a comp from @nadonomy somewhere :)