sourcegraph / sourcegraph-public-snapshot

Code AI platform with Code Search & Cody
https://sourcegraph.com
Other
10.11k stars 1.29k forks source link

Insights: UX input for conflicting permissions between insight and dashboard #25697

Closed unclejustin closed 3 years ago

unclejustin commented 3 years ago

tldr 1. We need input on user flows for handling insight and dashboard permissions

tldr 2. Permissions are hard. I'm sorry for how long-winded this is

Hypothetical dashboard permissions

There are three basic permissions

  1. Global - everyone has access
  2. Organization - only members of this organization have access
  3. User - only this user has access

Note A: Dashboards and insights can both have multiple organizations and users.

Note B: User permissions supersede organization permissions.

Note C: Global supersedes everything else

Example permission sets

Let's get weird

Scenario A: Adding a less restrictive insight to a more restrictive dashboard

Given:

Do we alert the user that they are adding a Global insight to a dashboard with more restrictive permissions? I personally don't think it matters. I think this flow is, just add the insight and move on.

Scenario B: Adding a more restrictive insight to a less restrictive dashboard

Given:

This is the one with several options.

Updating insight & dashboard permissions

Changing permissions on an existing insight or dashboard will need to check if they are being used anywhere and prompt the user to take action accordingly.

For example:

Updating an insight from Global -> Org A will need to check if that insight is used on any dashboards. If so check that those dashboards' permissions are not overly permissive and....do something.

github-actions[bot] commented 3 years ago

Heads up @joelkw @felixfbecker @vovakulikov @unclejustin - the "team/code-insights" label was applied to this issue.

vovakulikov commented 3 years ago

I still don't give up that someone likes this idea as much as I do. @Joelkw @AlicjaSuska https://github.com/sourcegraph/sourcegraph/discussions/25201#discussioncomment-1374872

coury-clark commented 3 years ago

@vovakulikov to be clear, I like the idea. I just don't see how we solve the problems in that thread without (some kind of) insight-level permissions.

Joelkw commented 3 years ago

(First I will respond to @unclejustin 's great writeup specifically, then I will respond to permissions discussions happening in various places more generally back on that github discussions ticket.)

For Scenario B I would expect

Offer to update the Insight's permissions for them?

But phrased a bit differently than "would you like to update the permissions for this insight?" (which this wording implies) and instead phrase it like "Note: if you confirm moving an insight to this dashboard, it will be the first time this insight is visible outside [org A]." (wording imperfect but more a statement/FYI and doing it for them if they confirm.)

This is a slack comment from a DM with @AlicjaSuska that summarizes it as well: image

vovakulikov commented 3 years ago

@coury-clark I've captured this idea about insights without settings here https://docs.google.com/document/d/11jLcY3R7_KU6oeKePxehwJ7Y-v6GWYhnu-Uvtx0BPKc/edit?usp=sharing

Also I've added case for non-Sourcegraph HTML pages there https://www.figma.com/file/0BJQ82uvBRwZc6sHMNbKWD/Untitled?node-id=0%3A1

coury-clark commented 3 years ago

Follow up from a conversation with @unclejustin @vovakulikov @AlicjaSuska @CristinaBirkel and myself.

Do insights need individual permissions?

Some use cases that imply individual insight permissions:

  1. Viewer / editor permissions split
  2. Maintaining dashboard coherence across organizational change
    1. Users losing access to a team / org shouldn't lose access to insights they have created
    2. Organizations shouldn't lose access to insights if users leave

Example scenario:

The insight originally created should remain on the dashboards relevant to organization 1

Proposed Solution

  1. Remove user-specified insight permissions
    1. Insight specific permissions will be derived from specific events (creating a new insight, for example)
  2. The list of insights a user can access will be the union of:
    1. The insights that are associated with dashboards they can see
    2. Insights for which they have been granted user permissions
  3. Support two actions to move insights between dashboards:
    1. Attach - reference the same exact insight and do not modify permissions.
    2. Duplicate - make a detached copy of the insight and grant the initiating user permission
  4. Edit the creation UI to remove insight visibility selection, and replace it with optional dashboard associations
  5. Remove user and organization default dashboards (to be replaced by...)
  6. Add filter options to the "all insights" page to filter by:
    1. Insights available to me
    2. Insights I have user-level access to
    3. Insights available to a specific organization (future team) I am a member of
    4. Insights created by me (maybe?)
  7. Allow association of dashboards with user / organizations (teams) at creation time.

What does this accomplish?

Some Examples

Permissions in [brackets] Associations in {curly braces}

Scenario 1: moving org

Given

The associations and permissions do not change based on organizational change. User A can still see the insight as part of "all insights", but no longer can access "super great dashboard".

Notes:

What about deleting?

In the conversation we came up with the "google docs" analogy - if I share a document with someone I can revoke it or restrict it, but once they make a copy I am no longer able to influence the copy. This is the intentional trade-off between references and copies of which users should be aware of and make conscious decisions on how to build their dashboards.

Joelkw commented 3 years ago

This sounds good to me. My remaining large questions:

  1. What are the costs we foresee (technical or otherwise) that this solution would create for us (limitations or other)? This overall seems good – what are the biggest reasons to not do this?
  2. On this point:

    We substantially simplify the user experience - you will always have access to insights you create, everything is influenced by the visibility of the container (dashboard)

echoed here:

User A is moved from org 1 to org 5 [User A] {"super great dashboard"}

Does that make sense / is this necessary to keep User A on it? To borrow from the google docs analogy, when I add a google doc to the "code insights folder" I get an alert telling me "Sourcegraph is the owner of this folder, you will transfer ownership of this doc." That effectively means that I do not necessarily maintain access forever to things I create (imagine if "Sourcegraph code insights" was the owner, and you switched teams or something and we weren't an open company so you were not longer a part of "Sourcegraph code insights" and the doc became lost to you.)

We have multiple customers who expressed concern/dislike with associating any results/impact to "who created" the insight because they note people move around a lot (or could leave the company).

(Edit: insights "created by me" is a nice feature, I guess I'm just wondering if "insights created by me" should be limited to "insights created by me that I can still access" eg excludes insights that are in orgs you're not a part of anymore.)

Joelkw commented 3 years ago

And my remaining small/specific questions:

  1. Edit the creation UI to remove insight visibility selection, and replace it with optional dashboard associations
  2. Remove user and organization default dashboards (to be replaced by...)
  3. Add filter options to the "all insights" page to filter by: ...
  4. Allow association of dashboards with user / organizations (teams) at creation time.

^ this all makes sense but involves quite a bit of UI change. Will we have time for that before the 3.33 release, or do we have an iterative solution where we can still release some portion of this work in mind?

It feels like we could probably delay all of 4-7 in the UI for the next release (and when you select "visibility permission" on the insight, we just interpret that in the API as selecting the default org/personal/global dashboard directly since insights don't have permissions).

coury-clark commented 3 years ago

Does that make sense / is this necessary to keep User A on it? To borrow from the google docs analogy, when I add a google doc to the "code insights folder" I get an alert telling me "Sourcegraph is the owner of this folder, you will transfer ownership of this doc." That effectively means that I do not necessarily maintain access forever to things I create (imagine if "Sourcegraph code insights" was the owner, and you switched teams or something and we weren't an open company so you were not longer a part of "Sourcegraph code insights" and the doc became lost to you.)

This sounds to me more like an ownership model. In our sync today we seemed to come to the idea that there shouldn't be any strict reason why sharing an insight should transfer ownership, our take was that you have two options:

  1. attach a copy
  2. attach a reference

I suppose one of my first questions with an ownership model is: how can something have multiple owners? We can associate an insight with multiple dashboards, but how would something like that with ownership? Creating N copies each owned by their respective dashboard - but that removes the possibility of references.

Maybe the answer is that the event dictates whether or not a user gets permissions?

  1. If a user creates an insight through the creation UI, they will get user permission
  2. If a user "creates" an insight on an existing dashboard, they only get user permission if that dashboard is also a user dashboard. In other words, if a user makes a filtered view on an org dashboard, they don't also get user permission.

I think there is a fundamental trade-off here:

  1. References imply multiple associations - otherwise what is the point of having a referential insight?
  2. Ownership implies a single association

We have multiple customers who expressed concern/dislike with associating any results/impact to "who created" the insight because they note people move around a lot (or could leave the company).

We agree - that's one of the primary motivations behind this model, to preserve dashboard coherence through organizational change. Deliberate change made by users is another question.

(Edit: insights "created by me" is a nice feature, I guess I'm just wondering if "insights created by me" should be limited to "insights created by me that I can still access" eg excludes insights that are in orgs you're not a part of anymore.)

Yes, I should have been more clear. In the above model there are only two ways that a user can see an insight.

  1. The insights that are associated with dashboards they can see
  2. Insights for which they have been granted user permissions
coury-clark commented 3 years ago

What are the costs we foresee (technical or otherwise) that this solution would create for us (limitations or other)? This overall seems good – what are the biggest reasons to not do this?

We have to choose something. Probably the risk is that this may limit our ability to add some features down the road, but frankly, that is probably true of any permissions model (and why we are having this discussion right now - to unblock features based on the current settings model).

coury-clark commented 3 years ago

It feels like we could probably delay all of 4-7 in the UI for the next release (and when you select "visibility permission" on the insight, we just interpret that in the API as selecting the default org/personal/global dashboard directly since insights don't have permissions).

I'll defer to @vovakulikov and @unclejustin on specifics, but I don't see any reason this has to be all at once. The conversation was more around unifying the UX and permissions model, and this was just some of the conclusions around that.

coury-clark commented 3 years ago

Follow up thought: Our API would need to validate that the user initiating the request to add an insight view A to dashboard B should reasonably be able to do so. Are the below criteria sufficient (pre editor/viewer split)?

  1. The insight is viewable by the user (in the all insights list)
  2. The dashboard is viewable by the user

Does this same set of criteria apply to other operations (dropping dashboard if not relevant)?

  1. Remove from dashboard
  2. Duplicate view
  3. Edit view
CristinaBirkel commented 3 years ago

Follow up thought: Our API would need to validate that the user initiating the request to add an insight view A to dashboard B should reasonably be able to do so. Are the below criteria sufficient (pre editor/viewer split)?

  1. The insight is viewable by the user (in the all insights list)
  2. The dashboard is viewable by the user

I was just thinking about this! Would we run into issues based on the permissions of the dashboard? For example:

The dashboard is scoped to be viewable by team A. I'm part of teams A, B and C. Should I be able to reference an insight from one of the team B dashboards on the team A dashboard, just because I can see them? I think that I should be able to duplicate it, because that's just a matter of convenience, but it seems weird to create a reference this way.

coury-clark commented 3 years ago

The dashboard is scoped to be viewable by team A. I'm part of teams A, B and C. Should I be able to reference an insight from one of the team B dashboards on the team A dashboard, just because I can see them? I think that I should be able to duplicate it, because that's just a matter of convenience, but it seems weird to create a reference this way.

Yeah, one could imagine some pretty wild scenarios where expanding viewership allows for propagation quite wide. I don't necessarily think it's a problem (in that I don't immediately see why this would be a problem) as long as the behavior is consistent for all users (reference vs copy).

CristinaBirkel commented 3 years ago

Yeah, one could imagine some pretty wild scenarios where expanding viewership allows for propagation quite wide. I don't necessarily think it's a problem (in that I don't immediately see why this would be a problem) as long as the behavior is consistent for all users (reference vs copy).

Let's say I'm a member of teams A and B. I add a reference to an insight from a dashboard that only team B can see, to a dashboard that only team A can see. A month later, someone from team B edits that insight to include a query that should not be viewable by people on team A. I never would have referenced the insight if it had that query. But now it's there.

I was thinking we could add a constraint that you can only add a reference to an insight if everyone who can see the dashboard it's being added to can see the dashboard it's being added from. Does that make sense? And maybe if you change permissions on a dashboard it will (when applicable):

Joelkw commented 3 years ago

(All the higher-level questions above that Coury answered sound good to me)

Joelkw commented 3 years ago

On the question of "when should you be allowed to move an insight to another dashboard":

I agree with both of you! I think you should always be allowed to move insights around to any dashboards you have edit access to (and in the meantime will continue praying we never have a customer/user situation that would require us to split "viewer" and "editor" into different roles right now.

On the question of "when you move the insight to another dashboard is it a reference or a copy" (great question @CristinaBirkel ):

I think that the default behavior we should maintain is "when you add an insight to a new dashboard B [in addition to other places / dashboard A], it creates a reference" because:

(We can also consider features that show you "this insight lives on [z,a,b,c,] dashboards and your update will affect all of them [or] turn this insight from a reference to a copy".)

BUT if we maintain the idea that there is a (not present in the UI) concept of "orgs = folders" then moving an insight from one permission group to another permission group (so across two orgs) does seem like it needs to be a "break" from reference to copy instead. Because here we couldn't even safely show you "all the places this insight lives" when you go to update it, and the ability to "pass information through permissions layers" via shared insights seems dangerous.

What if you want to share references between orgs? Well, we will always support "global on this instance" as a permissions level (because it's simpler for smaller customers/quick setup, and goes with the Sourcegraph philosophy of transparency), so if you do want to create cross-org references, then I think should allow you to do so if the insight is globally visible. And if we really hear a need from customers, we can also expand this to the concept of "parent orgs" (sub-global but can hold multiple orgs), in this same pattern.

Note this paragraph ^ is a unique case where "global" can be used as references anywhere. I don't think it's generalizable to "copying from an org [bigger scope] to a personal dashboard [smaller scope] can also create a reference" –because you could get kicked out of the org and you should have to lose your personal reference (so this action should create a copy instead). But you can't get kicked out of "global" (at least not without losing your personal dashboards too obviously).

Joelkw commented 3 years ago

I was thinking we could add a constraint that you can only add a reference to an insight if everyone who can see the dashboard it's being added to can see the dashboard it's being added from. Does that make sense? And maybe if you change permissions on a dashboard it will (when applicable):

  • Let you know which insights will be removed by changing permissions on that dashboard
    • Result in "broken references" wherever that insight was referenced that is now invalid from a permissions standpoint

The user desires/wants behind this make sense to me! But I think that trying to track "if everyone who can see the dashboard it's being added to can see the dashboard it's being added from" gets very complicated, because to know this you'd have to break orgs into their specific sub-members. (I also think that is subject to changing a lot more often.)

Secondly, I'd be concerned with this option that it involves a lot of "broken references" being created, and I expect many people won't always fix those (or know to look for them). Instead, I think we should preserve references when possible and perhaps make the decision that it's not possible to preserve references across different orgs (but you can still duplicate/make a copy).

vovakulikov commented 3 years ago

@coury-clark @CristinaBirkel @unclejustin @AlicjaSuska @Joelkw I came up with some simple cases from the comments above and tried to solve them. Take a look whenever you got time. Page: User permissions examples https://www.figma.com/file/0BJQ82uvBRwZc6sHMNbKWD/Dashboard%2FInsights-permissions?node-id=17%3A5

Also, I just realized that we didn't discuss what are we gonna do with already existing insight at the org level for instance. I think there is no way to mark them and set user permissions. I also tried to cover that problem in figma.

coury-clark commented 3 years ago

Follow up from a sync with @Joelkw @CristinaBirkel and myself:

Some conclusions after going over the latest sync / async comments:

We’re comfortable if you can do problematic things with references, and these are:

1) You could accidentally leak something after you leave an org 2) you could accidentally delete things other people are looking at BUT: References won’t “break” so you don’t see broken insights

Customer feedback informs us that references enable users now, but we have no good signal on what kinds of permissions users need.

With some options that we can iterate on in the future:

  1. what if you lock down a dashboard? You could have the option to lock down references out – Some people might want it everywhere, we don’t have teams yet.
  2. Partial permissions - partial editor (change view but not insight metadata such as repos) This would allow references that cannot change the semantic of the insight
  3. More granular role-based permissions - full editor, viewer
  4. Event logs on dashboards that can be "undone" to repair accidentally broken dashboards

Therefore we will continue with the idea outlined above:

vovakulikov commented 3 years ago

Ok. Good so As I understood that We are generally ok with the system that originally was proposed by @coury-clark in this issue after we had a sync meeting about this system.

With that agreement in mind, I think these steps are possible to achieve during next week

  1. Edit the creation UI to remove insight visibility selection, and replace it with optional dashboard associations
  2. Remove user and organization default dashboards (to be replaced by...)
  3. Add filter options to the "all insights" page to filter by:
  4. Allow association of dashboards with user / organizations (teams) at creation time.

For 4. we implemented the first version of Combobox. I don't think it would be super complicated to add this in the creation with the right API. Here is the PR with comobobox component. https://github.com/sourcegraph/sourcegraph/pull/25790

For 5. It is also not so complicated but It requires a few decisions here, Should we preserve links to these dashboards in case if someone shared the URL of these dashboards? Are we ok with not having these dashboards and don't have created by me filter in all insights dashboard for now (I think next 1 release) @Joelkw @AlicjaSuska

If we ok with that then 6. can be postponed. About 7. this probably requires only BE effort right? I can't remember that we have to change something in the UI at least for this release @coury-clark is it about user permissions on the BE?

AlicjaSuska commented 3 years ago

Here is the list of the design needs for the nearest future based on the chosen approach (described in Coury's comment above):

  1. Creation UI

    • remove insight visibility
    • add dashboard assigning
    • inform about user keeping their access to the insight (?)
  2. 'Change dashboard visibility' popup

    • since we're not changing the visibility of particular insights, we need to change the messaging but the overall idea stays the same: checkbox for expanding visibility, information for limiting visibility (@Joelkw please confirm that it's ok with you)
  3. All insights dashboard

    • add filters for organizations and 'my insights' - not the most urgent but still needed soon-ish
  4. Assign insights to the dashboard

    • We should display all the insights accessible to the user (basically everything from 'all insights' dashboard
    • Inform them if they expand the visibility of the insight (ex. copy/reference from the other org dashboard). And here comes the following section:
  5. Copying / referencing the insights

    • @coury-clark is there any updated document that describes all the cases we've talked about: when to copy / when to reference the insight between dashboards owned by different orgs. This is still unclear to me from the comment above. My current understanding: 1. REFERENCE the org insight in the private dashboard 2. REFERENCE the insight between dashboards of the same org 3. COPY the insight between dashboards of different orgs.
    • Add 'shared' label to referenced insights so that user knows that changes made are going to be populated. Possibly add the information to the editing UI.

Please, let me know if there are any other design needs as of now that come to your mind.

Timeline -> @vovakulikov: It would be good to have some designs in a week (start or middle the week of Oct 18)

Joelkw commented 3 years ago

Thanks @AlicjaSuska ! Comments in order:

  1. LGTM. Not sure what you mean about the third bullet point, keeping access?
  2. Do we even need a checkbox? Insights don't have visibility anymore so – not sure we do, we just have to warn you about moving the dashboards around, right?
  3. Don't think this is urgent, agree. Given the migration is already a bit over the original time, I might suggest us holding off on this or figuring out the easiest way to "show you an all insights page without loading too many insights" in the interim.
  4. What is the "assign insights to the dashboard state"? Or is this functioning more as the "all insights you have" / replacing the "all insights" dashboard?
  5. I'll let Coury answer the first bullet but for the second instead of the "shared" could we just consider exposing what other dashboards the insight appears on, if any? "Shared" isn't much help to user because they don't have context on where, nor does it tell them how many (maybe I have an insight on 2 dashboards I own but someone else is also referencing it – but I don't know that and assume "shared" is just about my 2).
coury-clark commented 3 years ago
* @coury-clark is there any updated document that describes all the cases we've talked about: when to copy / when to reference the insight between dashboards owned by different orgs. This is still unclear to me from the comment above. My current understanding: 1. REFERENCE the org insight in the private dashboard 2. REFERENCE the insight between dashboards of the same org 3. COPY the insight between dashboards of different orgs.

I think the conclusions that @Joelkw and I came to were that the act of copy versus reference was entirely a user choice, and not restricted otherwise. The reason being we could always conceive of a known use-case that would be blocked by some form of those restrictions. So with the statement above We’re comfortable if you can do problematic things with references it's essentially to say that the UX should provide clear and obvious options (and understanding of what you're doing) but not otherwise restrict.

Along the lines of what @Joelkw said above, we could always add more information down the line to the insight to help users understand where it is being shown or how many times it is being shown.

AlicjaSuska commented 3 years ago

Thank you @Joelkw and @coury-clark for clarifying.

@Joelkw:

  1. by 'keeping access' I meant that user will be always able to edit and access insights created by them, no matter what happens with the dashboard they assign it to
  2. I'll prepare the design for it and we can move forward from there. I think that when changing the dashboard visibility, we need to inform that insights included in this dashboard will be shared with people from a different org. It's logical and we may not need the checkbox but an additional line of text that explain this would make the user more aware of what are the consequences of the action
  3. 'Assign insights to dashboard' is a popup where you choose the insights from (go to dashboard -> three dots menu -> add or remove insights'
Joelkw commented 3 years ago

@AlicjaSuska

  1. Got it, thanks. makes sense.
  2. Sounds good.
  3. For the "assign insights to dashboard state" perhaps it's at that moment we need to figure out how to allow users to choose between assigning a reference vs a copy (cc @coury-clark )?
coury-clark commented 3 years ago

@Joelkw We could also defer the problem at that moment and only allow references to be added to dashboard. To make a copy, one would create a copy, then add the copy to the dashboard. Perhaps this way it's always intuitive that adding to a dashboard is always referential.

Joelkw commented 3 years ago

Sounds fine to me for now, thanks @coury-clark

AlicjaSuska commented 3 years ago

Design work is available for review. Please, see the message on slack for context, thank you!

felixfbecker commented 3 years ago

Is this issue resolved?

coury-clark commented 3 years ago

Issue is no longer needed, the above has been implemented.