[Bug] Fix issues with read-only role

[stephen-crawford]

Currently there are numerous issues associated with the read-only role. This issue is meant to track the completion of these connected issues.

The specific issues in mind include:

• Despite Read Only Dashboard Role a user can rearrange visualizations on a dashboard security-dashboards-plugin#917
• Read Only Dashboard User Role causes Forbidden Errors on visualizations on a doashboard with tenants enabled security-dashboards-plugin#916
• [BUG] Read only user cannot create Short URL's security-dashboards-plugin#891
• Read Only Tenant Mode OpenSearch-Dashboards#4498
• Show controls as read only based on tenant permissions security-dashboards-plugin#1472
• Read-only tenant mode tests opensearch-dashboards-functional-test#792

Completion of this issue will look like a completed checklist of the linked issues where each issue has been addressed. Further details for the specific problems can be found on the linked issues.

[nibix]

Just to clarify the expectations:

• I would go for a separate PR for each of these issues, ok?
• [BUG] Read only user cannot create Short URL's security-dashboards-plugin#891 is an interesting and possibly tricky one. I guess, the vast majority of the work on this will go on in the Dashboards core project and not in the security plugin.

[stephen-crawford]

Hi @nibix, sounds great. Understood about the changes required for the short URL. I just made the issue here for convenience, but I know the linked issue is part of dashboards. Thank you for all your hard work.

[nibix]

We spent some time analyzing the code and the documentation:

There are two distinct functionalities bearing a name referencing to "read only" in Dashboards:

1. Read-only tenants: Users can be given read-only access to a Dashboards tenant via a role. This looks like this:

read_only_teant_role:
  tenant_permissions:
  - tenant_patterns:
    - "human_resources"
    allowed_actions:
    - "kibana_all_read"

This has the effect that the backend security plugin effectively blocks all write operations for the current user to saved objects. In Dashboards itself, write control widgets stay visible, however. Trying to use these will result in an error message along "Dashboard my_dashboard was not saved. Error: Forbidden".

This could (and should) be fixed by extending the Dashboards security plugin to use Dashboards capabilities to hide the write controls when it recognizes that the current user is accessing the current tenant via a role with only the kibana_all_read privilege. This has the limitation that each Dashboards plugin can freely define capabilities - so we kind of have to "guess" which capabilities refer to write operations. However, that should work in 99% of the cases.

2. Dashboards read only role: The Dashboards security plugin has a feature which hides all plugin nav links except Dashboards and Visualizations when the logged in user has a certain role. The role is called kibana_read_only by default, but the name can be changed using the dashboards config option opensearch_security.readonly_mode.roles. One big issue with this feature is that the backend security plugin is completely unaware of it. Thus, users in this mode still have write access to the Dashboards saved objects via the API. As the implementation effectively hides everything except the Dashboards and Visualization plugins, a better name might have been "dashboards only mode". At least readonly_mode is completely misleading as it creates the trust that users can safely view a dashboard without being able to fiddle around with it. I would believe that 50% of the confusion about "read only" Dashboards is coming from this situation.

To make things worse, the documentation about read only mode at https://opensearch.org/docs/2.7/security/access-control/users-roles/#set-up-a-read-only-user-in-opensearch-dashboards is wrong, as it names the wrong role names. The docs instruct the user to add the role opensearch_dashboards_read_only to a user. However, this role is still called kibana_dashboards_read_only (see the config/opensearch_dashboards.yml file in the default distribution. However, I am not sure atm where in the source repos this file is managed).

I am not sure if it would make sense to fix this read only mode, as it would stay kind of confusing due to the overlap with the read only tenant and the confusing name. As there is no real documentation on it (the existing docs are wrong), the apotion rate might be not too high as well. Thus, I would propose to deprecate this functionality in favor of read only tenants. The goal of hiding/selecitively showing only certain Dashboards plugins should be rather achieved by a universal access control solution for Dashboards plugins and capabilities as described in opensearch-project/security-dashboards-plugin#160 and also mentioned in opensearch-project/security-dashboards-plugin#917 .

@davidlago @scrawfor99 FYI

[davidlago]

Thanks @nibix. For 1), is there a way to make a variable available in a global enough context that other plugins and the dashboards application can leverage to show/hide components as they need? not sure I follow the having to guess part. Or were you implying that as part of this work we'll sweep each dashboard component/plugin and add the right toggles with some educated guesses? If that is the case, I would advise against it and have this issue/PR provide the "read only" context, and let other plugins/OSD use as they see fit.

For 2), I would not immediately do anything with this other than fixing the obviously incorrect documentation. I know there are larger plans to think about that capability access control in Dashboards, so happy to defer that deprecation until a plan for a successor is clear.

[nibix]

@davidlago

Each Dashboards plugin is supposed to expose a set of capabilities to Dashboards. Dashboards core or the security plugin can then choose to disable/enable the capabilities of the plugin individually for each user session.

The capabilities look like this:

• Dashboard plugin:

    createNew: true,
    show: true,
    showWriteControls: true,
    saveQuery: true,

• Discover plugin:

    show: true,
    createShortUrl: true,
    save: true,
    saveQuery: true,

• Visualize plugin:

    show: true,
    createShortUrl: true,
    delete: true,
    save: true,
    saveQuery: true,

• Index patterns plugin:

    save: true,

• Saved objects mgmt plugin:

    delete: true,
    edit: true,
    read: true,

• Advanced settings plugin:

    show: true,
    save: true,

Etc.

As you can see, there's a pattern, even though there is nothing which enforces the pattern other than the smartness of the developers.

So, for a user on a read only tenant, we could disable any capability which matches case insensitively the strings save, create, delete.

In this context it is important to keep in mind that it is not security critical if this heuristic gets a capability wrong. Still, the security backend plugin will block any writes to the saved objects. The capabilities only hide controls and thus avoid nasty error messages when the user clicks on a control which triggers an unauthorized action.

I would just recommend to document this and recommend plugin developers to take this into account when defining capabilities.

As already mentioned in my original comment, these capabilities could be also used to implement a universal access control solution for Dashboards plugins as described in opensearch-project/security-dashboards-plugin#160 and also mentioned in opensearch-project/security-dashboards-plugin#917 . This could replace the former "Read Only Role" - but that would be a second project.

For 2), I would not immediately do anything with this other than fixing the obviously incorrect documentation. I know there are larger plans to think about that capability access control in Dashboards, so happy to defer that deprecation until a plan for a successor is clear.

Okay. However, I would recommend to include a bold warning that just the role only has visual effects, but still allows API requests to write saved objects if these are manually crafted.

[davidlago]

Gotcha! that makes sense. I think it is a reasonable approach proceeding with those guesses.

[jakubp-eliatra]

Gotcha! that makes sense. I think it is a reasonable approach proceeding with those guesses.

Hi, @davidlago, according to your message, I'm proceeding with the implementation according to approach that @nibix suggested and I have a question. :)

So, for a user on a read only tenant, we could disable any capability which matches case insensitively the strings save, create, delete.

Is this the final say on a definite list of capabilities that should get disabled using this mode and that we are proceeding with? It would be great to have a verified list of capabilities that should be disabled, since as @nibix underlined here:

Etc.
As you can see, there's a pattern, even though there is nothing which enforces the pattern other than the smartness of the developers.

There might be some capabilities that we might miss (and we don't want to :)) using a general rule like this, so an agreed, thought-out list of capabilities would be very welcome here. Alternatively though, I will be proceeding with disabling anything matching save/create/delete as a general rule in the meantime.

[nibix]

There might be some capabilities that we might miss (and we don't want to :))

We already kind of have a strategy for this as outlined in #2701 (comment)

In this context it is important to keep in mind that it is not security critical if this heuristic gets a capability wrong. Still, the security backend plugin will block any writes to the saved objects. The capabilities only hide controls and thus avoid nasty error messages when the user clicks on a control which triggers an unauthorized action.
I would just recommend to document this and recommend plugin developers to take this into account when defining capabilities.

[davidlago]

Thinking out loud, would it be possible to update the capabilities object that each plugin registers to include a hide_for_read_only property for each capability registered that then can be used to conditionally render in the UI? And doing so in a backwards compatibility mode, i.e. it should work if the value of the capability field is true like today, but if the value is instead an object with the property hide_for_read_only, we take it into consideration?

That way we can sweep the current plugins/capabilities and hide for readonly as needed... new capabilities need to be explicitly hidden for readonly, vs. implicitly based on name heuristics that we need to document.

Thoughts?

[nibix]

Thinking out loud, would it be possible to update the capabilities object that each plugin registers to include a hide_for_read_only property for each capability registered that then can be used to conditionally render in the UI? And doing so in a backwards compatibility mode, i.e. it should work if the value of the capability field is true like today, but if the value is instead an object with the property hide_for_read_only, we take it into consideration?

That should be possible, but likely only with modifications of core Dashboards.

I would recommend to do it incrementally:

• First, let's do the security plugin only fix
• Then, let's refine with the additional dashboards support in separate PR (actually two separate PRs then)

[davidlago]

If the fix with a more explicit declaration of capabilities is possible, what is the benefit of doing the security plugin only fix (the one with the heuristics, right?) if it is to be followed by the second one with changes in dashboards?

If this were an urgent fix I'd say yeah, let's patch first the fastest way possible, but given that this has been an issue from day 1, we might as well spend our cycles in the longer term fix.

Unless you were thinking that the security plugin only fix is a building block for the other change? if so yeah, let's do it incrementally. If it is throwaway work, let's skip ahead.

[davidlago]

Linking opensearch-project/security-dashboards-plugin#1468 as the security plugin for dashboards is being asked to support this capabilities functionality as well (tangentially related)

[nibix]

@davidlago

If the fix with a more explicit declaration of capabilities is possible, what is the benefit of doing the security plugin only fix (the one with the heuristics, right?) if it is to be followed by the second one with changes in dashboards?

If we don't want each and every plugin developer having to touch their plugin, the heuristic is required anyway. The additional Kibana core API could only give additional hints on the character of the capabilities.

Thus, there will be virtually no throwaway work. For me, that makes this the ideal candidate for an incremental approach. Thus, we can get the main benefit quickly, without significant external risks. Afterwards, we can do the polishing - this allows us to move any discussions on how the extension to core should look like exactly to this phase.

The alternative would be just a breaking change which forces each Kibana plugin dev to add the required metadata.

[davidlago]

The premise on my suggestion was to do it in a backwards compatible way:

Thinking out loud, would it be possible to update the capabilities object that each plugin registers to include a hide_for_read_only property for each capability registered that then can be used to conditionally render in the UI? And doing so in a backwards compatibility mode, i.e. it should work if the value of the capability field is true like today, but if the value is instead an object with the property hide_for_read_only, we take it into consideration?

If not possible I agree, we will not be breaking all plugins because of this change... but is it not?

[nibix]

So, maybe we need to talk about this on a more concrete level. The proposal would be to kind of add a hide_for_ready_only property which is [] by default, right?

As far as I understand it, that means that a plugin defines by default no capabilities that should be hidden in read-only mode. In consequence that means that the code changes we're doing for the security plugin won't have any effect until a plugin developer goes ahead and defines the properties to be hidden.

So, while staying compatible on the source code level, we would not have an immediate benefit. That's why we were proposing the heuristic.

[nibix]

Talked with @davidlago :

We'll go this way:

• we won't use a heuristic
• add additional meta data to the Dashboards core capabilities interface
• we will go through the existing plugins and make the adaptions to these to report the proper meta data

[ashwin-pc]

Just jumping in here since this issue is whats linked to the dashboards doc updating the core api. How are we using the currently exposed capabilities and how does that differ from the new field we are adding to the api?

e.g.

Visualize

    show: true,
    createShortUrl: true,
    delete: true,
    save: true,
    saveQuery: true,

which now becomes

    show: true,
    createShortUrl: true,
    delete: true,
    save: true,
    saveQuery: true,
    hide_for_read_only: ['createShortUrl', 'delete', 'save', 'saveQuery'],

[nibix]

How are we using the currently exposed capabilities and how does that differ from the new field we are adding to the api?

To my knowledge, capabilities are not used at all by Dashboards otherwise. These stem from the Kibana times where they were used in non-free code.

[ashwin-pc]

If thats the case, why dont we just reuse them instead of creating anew property?

[nibix]

The existing properties do not follow a strictly defined semantic. Thus, it would be only possible to use a heuristic to determine whether a capability should be available in read only mode or not. This was discussed, but in the end there was a decision or go for a clearly definable approach. You can check the discussion in the first comments of this issue.

[ashwin-pc]

Oh got it. Sorry missed that in the long comment chain. There is a problem with this approach though. All the capabilities mentioned in the list above are custom capabilities and are scoped to a specific plugin. The list that we are adding is also technically a custom capability, but based on this thread its more metadata than a capability. e.g.

visualize: {
  show: true,
  ...
}

is accessed as coreStart.application.capabilities.visualize.show. So the new property will also behave as a capability like coreStart.application.capabilities.visualize.hide_for_read_only and isnt guaranteed to exist on each of the custom capabilities since their shape is pretty relaxed.

  /** Custom capabilities, registered by plugins. */
  [key: string]: Record<string, boolean | Record<string, boolean>>;

My recommendation is that we do one of two things:

1. keep a list in the security plugin that we disable for the read only tenants (quick but not scalable)
2. use the capabilitiesSwicher to disable the capabilities for a read only tenant or user. Currently we do not use this and is the correct way to update the capabilities.

To do this you will have to register a capabilitiesSwicher for each plugin that needs to have its capabilities restricted based on roles such that a request when the user authenticates or switches a tenant will update the capabilities using the switcher.

e.g.

core.capabilities.registerSwitcher((request, capabilities) => {
   // Request that comes after login or tenant switch
  if(myPluginApi.shouldRestrictSomePluginBecauseOf(request)) {
    return {
      somePlugin: {
        featureEnabledByDefault: false // `featureEnabledByDefault` will be disabled. All other capabilities will remain unchanged.
      }
    }
  }
  return {}; // All capabilities will remain unchanged.
});

I havent tried this so let me know if you have and what happened if you did.

[kajetan-nobel]

Hey @ashwin-pc, that's very on point! While working on a PoC, I looked into existing plugin solutions for a read-only behavior. This raised some concerns for me:

1. The aspect of determining where to put logic about whether the current tenant is in a read-only mode is a bit uncertain to me. However, I'm leaning towards the idea that this check should be somewhat integrated into the core. This would involve verifying within the registerSwitcher whether the current tenant is operating in that mode. At the same time, this approach seems to render some portions of the security plugin redundant, aside from the part responsible for hiding UI controls.

2. Certain plugins currently include a badge indicating whether you're in read-only mode. Likely, these modifications could also affect those plugins at some stage.
   e.g. https://github.com/opensearch-project/OpenSearch-Dashboards/blob/454fdfb391feec9301df9b6387a3a08d2f699097/src/plugins/visualize/public/application/index.tsx#L48

[DarshitChanpura]

Hey @kajetan-nobel jumping in here, correct me if I'm wrong or misunderstood something.

The aspect of determining where to put logic about whether the current tenant is in a read-only mode is a bit uncertain to me. However, I'm leaning towards the idea that this check should be somewhat integrated into the core. This would involve verifying within the registerSwitcher whether the current tenant is operating in that mode. At the same time, this approach seems to render some portions of the security plugin redundant, aside from the part responsible for hiding UI controls.

If this gets us to an acceptable end state, we should go with this route. We can deal with redundancy once implementation is complete as long as the performance doesn't take a hit.

Certain plugins currently include a badge indicating whether you're in read-only mode. Likely, these modifications could also affect those plugins at some stage.

I like the idea of badging. Would this sit next to tenant name to be displayed for read-only user?

[ashwin-pc]

@kajetan-nobel yes you are right, it does some of the work that is traditionally done using the security plugin. That being said, the security plugin being separate from core is an inherited burden and should ideally be a core functionality. I know there are plans to do that, so bringing this functionality into core is definitely moving the needle in the right direction.

[kajetan-nobel]

@ashwin-pc @DarshitChanpura
I'm sorry for leaving you for such a long time. Going back to the topic:

The only working way that I found was to add a SecurityService into a SetupCore

e.g. of integration

core.capabilities.registerSwitcher(async (request, capabilites) => {
  return await core.security.readonlyService().hideForReadonly(request, capabilites, {
    myAwesomePlugin: {
      save: false,
      delete: false,
    },
  });
});

Both (Dashboard PR and Security Plugin PR) are ready to review with PoC using registerSwitcher. In case of missing a Security plugin - it will not affect the capabilities.

Lifecycle of a SecurityService:

1. It registers to a SetupContext
2. It provides core's ReadonlyService which can be used to define how it should behave without a security plugin.
3. While the security plugin will be setup, it'll register its own ReadonlyService that extends a core's one.

Please let me know if this approach satisfies current needs.

[ashwin-pc]

@kajetan-nobel I like this approach a lot. I didnt have time to validate this locally yet, but based on the code i read, I dont see any issues with this approach. If there are no other objections to this, I think you can mark the PR's as ready to review and we can get this in :)

[kajetan-nobel]

Hey, the task is ready for review in the meaning of switching capabilities and hiding controls when a tenant is read-only:

PRs to review:

• Read Only Tenant Mode OpenSearch-Dashboards#4498
• Show controls as read only based on tenant permissions security-dashboards-plugin#1472
• Read-only tenant mode tests opensearch-dashboards-functional-test#792

Comment which explains how to test it:
opensearch-project/OpenSearch-Dashboards#4498 (comment)

cc: @ashwin-pc @peternied

[peternied]

Thanks @kajetan-nobel I've updated the description of this issue to show those outstanding PRs

[kajetan-nobel]

Update:

• Read Only Tenant Mode OpenSearch-Dashboards#4498 is waiting for additional approve
• Show controls as read only based on tenant permissions security-dashboards-plugin#1472 I've added requested tests and it's ready to review

[kajetan-nobel]

Hey, I'll need help with your reviews as opensearch-project/opensearch-dashboards-functional-test#792 is approved and waiting for merge.

• Read Only Tenant Mode OpenSearch-Dashboards#4498
• Show controls as read only based on tenant permissions security-dashboards-plugin#1472

[peternied]

I've reviewed both PRs [1][2] and added comments - I think you've arrived at a great place.

• [1] Read Only Tenant Mode OpenSearch-Dashboards#4498
• [2] Show controls as read only based on tenant permissions security-dashboards-plugin#1472

[kajetan-nobel]

@ashwin-pc I'll need your approval one more time in opensearch-project/OpenSearch-Dashboards#4498 as it got lost after merging the suggestion.

@peternied I've addressed all of your comments on opensearch-project/security-dashboards-plugin#1472 and I've improved some parts that required a bit of refactoring.

[kajetan-nobel]

@ashwin-pc @joshuarrrr @kavilla I need your help with review on opensearch-project/OpenSearch-Dashboards#4498

[stephen-crawford]

This is the still open PR: opensearch-project/security-dashboards-plugin#1472

[peternied]

This issue isn't fully resolved, as there are more checklist items to be completed.

[kajetan-nobel]

As an update:

• [BUG] Read only user cannot create Short URL's security-dashboards-plugin#891 it's completed as it was a part in Show controls as read only based on tenant permissions security-dashboards-plugin#1472
• Read-only tenant mode tests opensearch-dashboards-functional-test#792 waits for merge (as it waited for the last two PRs to be merged).

[kajetan-nobel]

I've updated functional tests and retested them with 2.11.1, they're on review one more time.

[kajetan-nobel]

@davidlago We can close it as all related issues/PRs are closed.