[DISCUSS] Embeddables and feature controls

[kobelb]

Currently, if a user only has access to Canvas and not Maps they are unable to add a map to a Canvas workpad or view the map which has already been added to a Canvas workpad.

This differs from the approach which was taken with Dashboards. When a user only has access to Dashboards and not Visualize, Maps, etc. they are able to add existing visualizations, maps, etc. to a Dashboard and are able to view a Dashboard and the referenced visualizations, maps, etc.

While application teams are given the freedom to decide how their privileges and integrations with other applications/features will behave, it'd be good for us to standardize on an approach.

@stacey-gammon what are your thoughts on this? Is there a way to "codify" the approach within the "embeddables framework"?

/cc @elastic/kibana-security

[stacey-gammon]

Are you sure? Maybe it's a bug with your specific setup? I just created a user that has access to a space with Maps turned off and they are able to see maps embedded in Canvas.

Agree it should be the same experience as with dashboard. Within the embeddable framework there is a isEditable function on every factory type, this is supposed to use the capabilities framework to decide the return val.

Here's a screenshot of my testing:

[kobelb]

I just double checked by creating a role which only has access to Canvas, and they aren't able to view the Maps. In your tests, are you also giving the role access to Dashboards? When the user has the Dashboards feature, they're able to read maps per:

kibana/x-pack/plugins/features/server/oss_features.ts

Line 88 in ec2134d

'map',

There was another recent regression I found here. My fear is that as we make Kibana features work better together, we run the risk of introducing similar issues. I'd like to find a way to make this easy for application developers to not "trip on".

[stacey-gammon]

Ah, interesting. So a short term fix is that Canvas permissions adds all the embeddable types like dashboard does, but that won't solve the situation when we have third party developers adding embeddable types that we don't know about.

Can plugins modify the permissions of other plugins? I'm not sure how we could solve this. Have an embeddable permission? I think we'd still need a way for plugins to dynamically add their embeddable types to this embeddable permission type? Hmmmmm 🤔

[kobelb]

Ah, interesting. So a short term fix is that Canvas permissions adds all the embeddable types like dashboard does

Yeah, it's a potential option. The one risk we have here is that before Canvas supported the integration with maps, users who just had access to Canvas weren't able to view maps in any way. If we were to make this change, we might surprise users by now allowing these users to view the maps. It's a rather pedantic concern, that we can potentially just ignore. If we had introduced maps in the same minor version which added the canvas integration, I likely wouldn't be bringing this up.

but that won't solve the situation when we have third party developers adding embeddable types that we don't know about.

Can plugins modify the permissions of other plugins? I'm not sure how we could solve this. Have an embeddable permission? I think we'd still need a way for plugins to dynamically add their embeddable types to this embeddable permission type? Hmmmmm 🤔

Agreed, we'd likely need to change the way that features are registered to fully address this for third-party embeddables. I believe we discussed this briefly a while ago, and then dropped the ball on pushing it forward. We absolutely should do this long-term, but we won't have it available for 7.6 which adds the Canvas and Maps integrations...

[kobelb]

Apologies for the serial replies... The reason I brought up this discuss issue in the first place is because we have a decision that we can make here. We don't necessarily have to make it so when you have access to a feature, you automatically get access to all of the saved-object types which can be embedded in that feature. It's the approach we took with Dashboards, which made sense at the time. The other option we have is to continue to model it the way it's currently done with Canvas, where you need access to both Canvas and the Maps feature to be able to embed Maps in Canvas. I'd just like for us to figure out the guidelines we have for which approach to choose, so we can have some consistency.

[stacey-gammon]

cc @clintandrewhall @majagrubic - your thoughts here? I feel like giving read access to maps in Canvas permissions sounds like a better UX. If we consider it a bug, it's not a BWC change, right? :)

cc @timductive

[legrego]

I'm torn on what to do here. The security practitioner in me likes the Canvas approach, where we can support fine-grained controls over the various saved object types without any unexpected authorization surprises. (i.e. we won't get asked "Why does Canvas grant me access to Maps?").

On the other hand, the rest of me likes the Dashboard approach, because it allows for "one-click" access to everything that a feature can do. It also substantially reduces the complexity required for each app that supports embeddables. If we offer fine-grained control, then each app will have to account for an ever growing combination of privileges, and decide how to offer their experiences accordingly.

Regardless of the approach, I agree with Brandon that we need to change the way features are registered to accommodate embeddables

[jportner]

+1 for the Dashboard approach. I think the current situation doesn't make sense, and if security is complicated then we're doing it wrong and people are more prone to mistakes. It follows that if I want to give a user access to Canvas, they should have access to anything that Canvas embeds or directly depends on.

[kobelb]

Once Larry's awesome sub-features are available with #35616, we have the option to add a sub-feature for embedding Maps within Canvas. Whether or not this should be automatically included within the Canvas "all" privilege in a minor is debatable, and we could potentially add the ability for sub-features to be excluded from the primary feature privileges...

[clintandrewhall]

My gut reaction wrt Canvas would be to treat an embeddable as read-only/static if the viewer does not have access to the feature that created the embedded asset.

It would stand to reason that if the viewer didn't have access to the feature, then someone else prepared the embed... so they should have access to the end-state that was prepared, but not the preparation. For maps, that would mean some subset of features, and the embed couldn't be fundamentally altered, (e.g. parameters, perhaps even the expression itself). Maps would provide that limited rendered state, e.g. zoom in/out, but not pan; click for detail but not filter, whatever they decide is appropriate for a read-only embed.

The comparison might be shared workpads in Canvas: you have the final rendered state at the moment it was prepared, but you can't alter the expressions or other logic that produced that workpad that was shared at that point in time, (e.g. the data doesn't update).

Does that make sense? In short, I'm leaning much more toward the source of the embed determining that read-only set of features, and owns that read-only embed render.

This has implications for Canvas, as well. At the moment, external shared workpads can't handle embeddables, as they require a live data connection. A read-only rendered state would allow us to share workpads with embedded assets easily.

[stacey-gammon]

A read-only rendered state would allow us to share workpads with embedded assets easily.

Not necessarily. Ready only in this sense just means that the user can't edit or create map saved objects, but the embeddable might still go grab the saved object, then query elasticsearch, so there are direct connections to Kibana/ES.

I'm leaning much more toward the source of the embed determining that read-only set of features, and owns that read-only embed render.

If the user has read only permissions of the embeddable type in question, this works out fine. The problem is when the user does not have read only permissions of Maps. I think you are saying that, for example, there should be logic in Maps application that says, even if this user does not have read only permissions to maps, render embeddables anyway? I'm not sure how that logic would work but I have a feeling feature controls handles that logic at the saved object client level so maps wouldn't be able to override it?

It would stand to reason that if the viewer didn't have access to the feature, then someone else prepared the embed... so they should have access to the end-state that was prepared, but not the preparation.

I don't know. Should they have access to that end state if the viewer explicitly does not have maps read access? Why would they not have read access?

Now that I'm thinking about it... if someone went out of their way to say this user shouldn't even be able to read/view maps, why should we show it to them? Write access is one thing, hiding the app from the nav is another, but no read access at all? I'm trying to think of the situation where someone would set up permissions like this, but still want to allow the user to see maps.

[clintandrewhall]

if someone went out of their way to say this user shouldn't even be able to read/view maps, why should we show it to them?

That's a compelling statement. Perhaps we should consider an "Access Denied" state for embeddables.

[stacey-gammon]

We don't actually have to do anything special in embeddables if this is the flow we want. Canvas works this way now. We'd just have to remove maps read capabilities which dashboard permissions add implicitly. Maybe that is the simplest path forward, but it could be a breaking change. Maybe in 8.0?

[kobelb]

Using Canvas and Maps as an example, I think we long-term want to support the following options:

1. Create Canvas workpads, embed maps, create new maps
2. Create Canvas workpads, embed maps, can't create new maps
3. Create Canvas workpads, can't embed maps
4. View Canvas workpads, view embedded maps, create new maps
5. View Canvas workpads, can't view embedded maps, create new maps

Using the current Dashboard approach to privileges with the addition of sub-features, we can model all of these situations, and it seems rather intuitive:

1. Create Canvas workpads, embed maps, create new maps

2. Create Canvas workpads, embed maps, can't create new maps

3. Create Canvas workpads, can't embed maps

4. View Canvas workpads, view embedded maps, create new maps

5. View Canvas workpads, can't view embedded maps, create new maps

In my opinion, this is where we should be aiming to get, and then figure out how we get there.

[legrego]

@kobelb should we also consider:

Create Canvas workpads, embed maps, can't create maps

In other words, users with "All" access to Canvas and "Read" access to Maps should likely be able to embed existing maps into a Canvas workpad.

[kobelb]

@legrego, yup! I hit "enter" and accidentally posted this too early, fleshing out what I said above at the moment :)

[kobelb]

@peterschretlen I know you've given sub-feature privileges some thoughts before, do you have any opinions on their use within the context of embeddables?

[peterschretlen]

The question that comes to mind is - do we consider a map something special to the maps app? Or just another type of visualization content that could be used in a viz composition/authoring/editing tool (maps, dashboard, canvas, lens) The same question could eventually apply to something like graph.

I think the dashboard has the right idea: feature controls limit the composition/authoring/editing/views (what you can do in the app), but don't limit your access to the visualizations created in your space. I think that should extend to embeddables in Canvas too.

For example, I can imagine at some point creating a primitive map using Lens directly in a dashboard. Same with graph at some point. Now these visualizations can be created and edited across multiple apps each with their own feature controls. An "embed maps" sub-feature in Canvas feels kind of odd - controlling the overall ability to embed in Canvas might make more sense as a sub-feature?

[kobelb]

The question that comes to mind is - do we consider a map something special to the maps app? Or just another type of visualization content that could be used in a viz composition/authoring/editing tool (maps, dashboard, canvas, lens) The same question could eventually apply to something like graph.

Currently, Dashboards can contain visualizations, lens visualizations, maps and saved-searches. Each of these are created by an application separate than Dashboards. Even if a user only has all access to the Dashboard feature, they're able to add existing "embeddables" (visualizations, lens visualizations, maps and saved-searches) that are already created. In this sense, maps themselves aren't necessarily special. However, maps are the first saved-object from an external application which can be embedded in Canvas. All of Canvas' other elements are created from within Canvas itself.

An "embed maps" sub-feature in Canvas feels kind of odd - controlling the overall ability to embed in Canvas might make more sense as a sub-feature?

Are you proposing instead of adding a sub-feature to Canvas to embed maps that we'd add a sub-feature to Maps for whether or not these can be embedded in Canvas?

[peterschretlen]

However, maps are the first saved-object from an external application which can be embedded in Canvas. All of Canvas' other elements are created from within Canvas itself.

Yes, my question (is there something special about maps?) seems like the wrong one then. Instead perhaps it should be: is there something special about Canvas that we would not want to share objects with it the way we do with dashboards.

Short term there's the fact Canvas hasn't embedded maps before - so a potential for surprise on upgrade to unexpectedly expose maps. Practically though I don't think it's a big concern.

Long term / guideline-wise I can't think of a counter-example where Canvas should differ in this way from Dashboard. I think provided these 'presentation' apps can support rendering an embeddable, there's no reason to add controls around whether they can access a type of visualization.

Are you proposing instead of adding a sub-feature to Canvas to embed maps that we'd add a sub-feature to Maps for whether or not these can be embedded in Canvas?

No, I was more questioning whether we need a sub-feature for embedding maps at all. I don't see a need for it, and there's no real downside to making it part of Canvas' all privilege? If it is requested/needed at some point, it shouldn't be an issue adding it as a sub-privilege later?

Though I do see the problem you're getting at - if you have a bunch of apps defining embeddable types (some from 3rd parties), and a bunch of apps consuming them, how do you manage this when defining feature privileges? The consuming app lists the saved object types it can access, so it's king-of-the-hill today, and it won't know about 3rd party types.

[kobelb]

No, I was more questioning whether we need a sub-feature for embedding maps at all. I don't see a need for it, and there's no real downside to making it part of Canvas' all privilege? If it is requested/needed at some point, it shouldn't be an issue adding it as a sub-privilege later?

Yeah, we can definitely postpone adding it until later after it's deemed necessary.

Though I do see the problem you're getting at - if you have a bunch of apps defining embeddable types (some from 3rd parties), and a bunch of apps consuming them, how do you manage this when defining feature privileges? The consuming app lists the saved object types it can access, so it's king-of-the-hill today, and it won't know about 3rd party types.

We can make some changes to accommodate features producing the embeddables registering themselves with the features consuming the embeddable. Right now, we just have a static list that is controlled by the application that consumes all of the embeddables. It'd then be up to the consuming application to determine whether or not to have sub-feature privileges for each of these or just make them part of the all privilege.

[arisonl]

if someone went out of their way to say this user shouldn't even be able to read/view maps, why should we show it to them?

I feel that this remains unanswered? With new applications, types of objects and the requirement to be able to embed any type, the question becomes even more interesting (previously it was more of a 1:1 thing, dashboards:vizes). For the same reason, enforcing a consistent adding/embedding behaviour, rather than leaving it up to each app, becomes even more important. We could however, leave it up to the admin: Maybe not immediately but down the road, the ability to add an embeddable of any type to a consuming app becomes an "add-all" privilege on top of "all/read" (available to consuming apps only). If it is selected, we have the current dashboards behaviour ("implicit" access to objects that you may not have access to outside the consuming app). If not, the privileges remain as defined for each of the embeddable types (if you do not have access to the type outside the consuming app, then you cannot add it).

[stacey-gammon]

@arisonl - I was thinking about the same thing but I think the reason is based on how feature controls is shown to the user. "All | Read | None", which means that if they want to hide the app from the user, they select "None", so I think that is probably why users select "none", not because they are explicitly saying "do not allow reading". Like I think it'd be different if "Read" was an on/off toggle. Then that would be explicitly saying "do not allow reading".

[kobelb]

if someone went out of their way to say this user shouldn't even be able to read/view maps, why should we show it to them?

I feel that this remains unanswered?

I think the phrasing "someone went out of their way to say this user shouldn't even be able to read/view maps" is just one way to look at this situation. When a user selects "Custom" on the role management page, all features default to "None". They then select which features the user should have access to:

So, it's possible that the user was approaching this differently and thinking "I only want this user to be able to create Canvas workpads". Whether or not they're then surprised when the user is then able to view all existing maps and add them to workpads is still debatable. However, I think this can be addressed with the introduction of sub-feature privileges, because they'll be able to expand the section per these mockups to get a better overview of what the "All" privilege actually includes:

For the same reason, enforcing a consistent adding/embedding behaviour, rather than leaving it up to each app, becomes even more important.

I completely agree.