[BUG-2.7.0] feature flag opensearch_security.multitenancy.enabled was broken

[Flyingliuhub]

What is the bug?
the feature flag opensearch_security.multitenancy.enabled was broken when use it in the opensearchdashboard.yaml file, the recently change https://github.com/opensearch-project/security-dashboards-plugin/pull/1394/files will override this flag.

How can one reproduce the bug?
Steps to reproduce the behavior:

1. opensearch_security.multitenancy.enabled: false in the opensearchdasbhoard.yaml
2. start opensearch dasbhoards
3. it pop up the windows ask to choose tenant
4. cannot choose any tenant, please see the screenshot below

What is the expected behavior?
opensearch_security.multitenancy.enabled: false should functional

Do you have any screenshots?

Do you have any additional context?
N/A

[kavilla]

Part of the bug here is this boolean is accessed:

https://github.com/opensearch-project/security-dashboards-plugin/blob/main/public/apps/account/account-nav-button.tsx#L88

before https://github.com/opensearch-project/security-dashboards-plugin/blob/main/public/apps/account/account-nav-button.tsx#L73 is able to update the state. So it isMultiTenancyEnabled is always true at first when accessed since it's the default value. It's difficult to see this bug on local because if you disable multitenancy dynamically, you will likely not see the popup because getShouldShowTenantPopup will likely return false if you have previously ran the application. So to constantly see it, set the dynamic config and open in incognito.

[cwperks]

One solution could be to use the value in opensearch_dashboards.yml as the default value.

For this line

const [isMultiTenancyEnabled, setIsMultiTenancyEnabled] = React.useState<boolean>(true);

could be changed to:

const [isMultiTenancyEnabled, setIsMultiTenancyEnabled] = React.useState<boolean>(props.config.multitenancy.enabled || false);

@Flyingliuhub @kavilla Are you able to dynamically turn off the multitenancy enabled value and have dashboards run as expected?

[cwperks]

@abhivka7 What are your thoughts on this issue?

[cliu123]

@cwperks Good point! But the default value should be true, like the following as Multi-tenancy is enabled by default:

const [isMultiTenancyEnabled, setIsMultiTenancyEnabled] = React.useState<boolean>(props.config.multitenancy.enabled || true);

[cwperks]

@cliu123 Thank you for chiming in!

props.config.multitenancy.enabled || true would always return true, so this just needs to be updated to use props.config.multitenancy.enabled if its not undefined otherwise use the default of true.

props.config.multitenancy.enabled !== undefined ? props.config.multitenancy.enabled : true;

edit: The nullish coalescing operator should work too

props.config.multitenancy.enabled ?? true

[kavilla]

That would help as it would appear the static config currently does not seem to have much purpose within OpenSearch Dashboards as it would prioritize the OpenSearch dynamic setting over the OpenSearch Dashboards.

However, we will still have the issue with the state being accessed before it is updated from the async function. In this sense we should ensure that we wait for the response from dynamic config prior to accessing and executing logic.

[DarshitChanpura]

@cwperks Good point! But the default value should be true, like the following as Multi-tenancy is enabled by default:

const [isMultiTenancyEnabled, setIsMultiTenancyEnabled] = React.useState<boolean>(props.config.multitenancy.enabled || true);

@cliu123 The default value should come from where it is set in the props. The default setting in the file shouldn't be the deciding factor. If we set true in the local state, we won't depend on the configuration in opensearch_dashboards.yml, so the value will always be true until the call to fetch multi-tenancy data (from the backend), which is when the next evaluation happens to see if the popup should be shown..

IMO, this is how the default state should be set, unless mutli-tenancy flag will not be defined in opensearch_dashboards.yml at the cluster startup:

const [isMultiTenancyEnabled, setIsMultiTenancyEnabled] = React.useState<boolean>(props.config.multitenancy.enabled || false);

@Flyingliuhub @kavilla For customers facing this issue immediately, they should execute the following curl request to update the multi-tenancy config in security index:

curl -XPUT https://<usercreds>@URL/_plugins/_security/api/tenancy/config --data '{ "multitenancy_enabled" : false}' -H "Content-type: application/json" | jq

NOTE: Use admin creds and --insecure option if running on localhost

I have created a PR to fix this issue #1417

[abhivka7]

@DarshitChanpura I have a better approach to fix this issue. I am working on raising a PR for that. Please don't merge your PR until then.

[abhivka7]

I have raised a PR for fixing this issue: #1419
Please review.

[kavilla]

There appears to be other issues with the current implementation.

The docs say:

opensearch_security.multitenancy.enabled 	Enables or disables multi-tenancy in OpenSearch Dashboards. Default is true.

But looking at the config the default value is false. When we release a build we do set the config but anyone who pulls the source or min artifact can simply not set that value and the OpenSearch Dashboards side will be defaulted to false. While OpenSearch appears that default value is set to true. So the code in the public folder will utilize the dynamic config to determine to set up multitenancy but the code in the server folder will use the static config to determine if it should set up things related to multitenancy, to which some parts of the public folder expect to be set up.

For example, the OSD plugin will not setup the index template if the static OSD config is set to false.

Which in this context isn’t the biggest deal because if the static config is set to false the user will not be able to dynamically configure their multitenancy. This is due to the routes depend on the static config being true to be setup.

But it still makes for a bad experience because I just get a bunch of errors because the public client side will render the logic as if multitenancy is enabled while the server side will not execute the logic since the static config said multitenancy is disabled. You can see this by commenting on the config if using the release, starting the application, and navigate to the Security > Tenants > Configure and try to make any changes and save here. You will get errors.

In this feature's current state, we will have to be explicit that users can not use the dynamic config if they have disabled the static config in OSD.

However, we can not simply just make the code changes so that the server side respects the dynamic config because the routes and index template are currently only set up at security dashboards plugin discovery which only occurs at OSD startup. If we were to do that without refactoring other parts, a potential issue is where an admin would be able to update the configuration dynamically and then bounce OSD which will then trigger the re-discovery and not setup the index templates. Then if they proceed to re-enable this dynamically then the index-template will not exist and private tenants will fail to have the correct index mapping.

One proposal would be if the multitenancy is dynamic then we should ensure that we always set up these routes regardless and just reject requests if multitenancy is disabled. We can also consider moving the index template creation outside of the startup process.