Show controls as read only based on tenant permissions

[jakubp-eliatra]

Description

Adds a proper read only mode support for read only tenants.

A new capability hide_for_read_only as array is utilized to recognize the capabilities which should be disable in read only mode. If a read only tenant is recognized the capabilities associated with writing access such as 'createNew' and 'saveQuery' are then set to false.

Category.

New feature

Why these changes are required?

• Resolves [Bug] Fix issues with read-only role security#2701.

What is the old behavior before changes and new behavior after changes?

For example re-arranging visualisations is not possible in such mode now.

[davidlago]

@jakubp-eliatra , the DCO check is failing on this PR. I'm going to hold off on approving the CI run since we'll have to re-run anyway.

[jakubp-eliatra]

Just updated the PR. Two remarks:

1. Whenever the capabilities route is called, we perform a request to authinfo in order to get information about the user's tenant. This request requires an authorization header.
   We did run into the problem that we don't have the authorization header set on the request. The reason for this is that the capabilities route is ignored by the auth handler, which means that the auth header is never added here.
   We solved this by always passing the auth type to the capabilities switcher logic, and using the auth type and the cookie to build the authorization header here.

Another possible solution, and maybe something to address either way in the future, would be to stop ignoring the capabilities route.
That route is defined as auth being optional, and that case doesn't seem to be handled by the authentication logic right now. To handle that case, we could check the route's auth settings before returning a 401 Unauthorized. For example like this:

if (request.route.options?.authRequired === 'optional') {  
  return toolkit.notHandled(); // Maybe toolkit.authorized()?
}

That way, the request will not throw an error if it is not authenticated, but if it is authenticated, the regular process would continue.

2. As discussed, this PR assumes that a complementary PR for Dashboards would get created that adds hide_for_read_only property to capabilities' providers like so:
   export const capabilitiesProvider = () => ({ dashboard: { createNew: true, show: true, showWriteControls: true, saveQuery: true, hide_for_read_only: ['createNew', 'saveQuery'], }, });

The new property is an array that contains the names of capabilities that will get toggled to false (for read only tenants) using the logic from this PR. So basically we would need to add this one line to each capabilities_provider.ts for all affected plugins.

The plugins that need to be modified include:

• advanced_settings
• dashboard
• index_patterns
• discover
• saved_objects_management
• visualize

PS. One idea that also came up was to make sure that all apps register capability 'showWriteControls', just as the Dashboards app does. That one capability would fit well for the read only use case, if it takes precedence over other capabilities that are used for write controls. But since we already had a discussed solution, we went that path.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d5a6626) 66.63% compared to head (e1d41d4) 66.63%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1472   +/-   ##
=======================================
  Coverage   66.63%   66.63%           
=======================================
  Files          94       94           
  Lines        2395     2395           
  Branches      319      319           
=======================================
  Hits         1596     1596           
  Misses        728      728           
  Partials       71       71           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jakubp-eliatra]

We took our time to estimate the efforts of adjusting internal and external plugins to read only mode. Here are the results:

a) internal plugins that need modification:

1. advanced_settings* (save)
2. dashboard* (createNew, showWriteControls)
3. data*
4. discover* (save)
5. home*
6. management*
7. opensearch_dashboards_legacy (showWriteControls)
8. opensearch_dashboards_react (advancedSettings, navLinks)
9. saved_objects_management* (delete)
10. timeline* (save)
11. vis_builder
12. visualize* (show)
13. console* + dev_tools (!) (show, save)

asterisk (*) - registers capabilities but doesn't do anything with them OR uses capabilities in a way unrelated with logic that should be updated for read only mode; they might need little adjustments or not at all, but they should at least be tested
(x, y) - known capabilities used within the plugin

b) external plugins that need modification:

• dashboards-maps - https://github.com/opensearch-project/dashboards-maps/blob/main/server/saved_objects/map_saved_object.ts
• dashboards-observability - https://github.com/opensearch-project/dashboards-observability/blob/main/server/saved_objects/observability_saved_object.ts

Those are the only two out of external plugins that we found they have something to do with capabilities at all. There doesn't seem to be any logic related with read only mode inside of those plugins. By that I mean that I checked the codebase in general, wandering from file to file and searched for buzzwords such as capabilities + readonly/disabled (obviously with excluding cases where it's just UI logic, such as disabling a form field somewhere).

Due to the fact that for example maps plugin contains customizable layers of maps (and user can set zoom etc.), I believe it would be best for these two to incorporate some logic related with read only mode for those two plugins. In other words, a support for read only mode would make for a better UX in case of those plugins.

c) my opinion on the research that's been done:

Luckily, most of changes are limited to main Dashboards repo, however in the future more and more plugins might need to be aware of how the read only mode works and what property should be registered (hide_for_read_only array if that's what we're going with) to make it work properly.

Therefore documentation updates should be & will be included in the read only PR.

Summing up, it seems that there is no ideal solution. We either:

1. have a list of capabilities are related with read only logic to toggle them (like showWriteControls, show etc.) - heuristic solution but there's the guessing factor (we eliminated this approach here: [Bug] Fix issues with read-only role security#2701)
2. we have them explicitly listed within the hide_for_read_only_property and we adjust some logic within internal/external plugins described above.

[cwperks]

@jakubp-eliatra Are there any tests that can be written for this change? Unit, integration of functional tests?

[stephen-crawford]

Hi @jakubp-eliatra, thank you for putting this together. Overall, everything makes sense to me. That being said, we will want to add some tests to verify the functionality is expected. Since we are making a pretty significant UI change when the tenant is read-only, you should be able to use Cypress to check that an element which is present with not read-only is no longer present for a read-only tenant.

[stephen-crawford]

Looks good, just one comment about an error message

[kajetan-nobel]

@DarshitChanpura @cwperks Regarding unauthorized routes, I found that we've got a similar implementation here

security-dashboards-plugin/server/auth/types/authentication_type.ts

Lines 218 to 233 in 31c5a72

	authNotRequired(request: OpenSearchDashboardsRequest): boolean {
	const pathname = request.url.pathname;
	if (!pathname) {
	return false;
	}
	// allow requests to ignored routes
	if (AuthenticationType.ROUTES_TO_IGNORE.includes(pathname!)) {
	return true;
	}
	// allow requests to routes that doesn't require authentication
	if (this.config.auth.unauthenticated_routes.indexOf(pathname!) > -1) {
	// TODO: use opensearch-dashboards server user
	return true;
	}
	return false;
	}

which already contains not exhaustive list. Maybe it would be good to refactor there and reuse here.

There is also a possibility to use request.route.options.authRequired to check if the requested route requires to be auth (lots of contains value optional)

[DarshitChanpura]

I agree. We should refactor to re-use.

There is also a possibility to use request.route.options.authRequired to check if the requested route requires to be auth (lots of contains value optional)

i'm not sure about this one as this seems a change in design or the way API works. This could be done in a follow-up PR

[cwperks]

There is also a possibility to use request.route.options.authRequired to check if the requested route requires to be auth (lots of contains value optional)

This would be my preferred approach. I think it would be better for maintenance to have an annotation/option where the route is declared that can signal whether the route is protected (auth required) or not. Most instances of routes not requiring auth are going to be login endpoints.

[kajetan-nobel]

@DarshitChanpura @cwperks I tried to base it on authNotRequired, but it's on a completely different use level, and it would make it much more troublesome. To explain it more:

• authNotRequired entirely wants to ignore the capabilities route (especially for the login page) while most of isReadonly are calls to capabilities
• authNotRequired is based on the current pathname, not the referrer which is kinda important in case of again - capabilities

I'm going rather to keep the current implementation and use there request.route.options.authRequired

[kajetan-nobel]

@DarshitChanpura @cwperks Basically while the hideForReadonly method is used only in core.capabilities.registerSwitcher all calls are to capabilities. I'm leaving isAnonymousPage with the additional request.route.options.authRequired as a backup when someone would like to call it outside the method.

[kajetan-nobel]

@cwperks Regarding #1472 (comment) I've added a fix to it.

[peternied]

I've reviewed the outstanding comments as well - thanks!