Read Only Tenant Mode #4498

jakubp-eliatra · 2023-07-05T12:53:24Z

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.

Issues Resolved

This is a complementary PR for: opensearch-project/security-dashboards-plugin#1472, related with issues here: opensearch-project/security#2701

Check List

Signed-off-by: jakubp-eliatra <jakub.przybylski@theworkingroup.net>

ashwin-pc · 2023-07-05T22:48:10Z

@jakubp-eliatra Can you as a part of this change also tackle #2575? Since you are modifying the capabilities service, it would also be nice to know how it is being used, including how to use the new flag :)

jakubp-eliatra · 2023-07-06T10:02:20Z

@ashwin-pc Yes, we will document how the capabilities are used to achieve read only mode. Outside of that, I would suggest to leave #2575 as a separate issue still, due to capabilities not being really used in plugins (outside of read only mode logic) and due to enlarging the current scope of PR.

Signed-off-by: Kajetan Nobel <k.nobel@routegroup.pl>

…nants Signed-off-by: Kajetan Nobel <k.nobel@routegroup.pl>

ashwin-pc · 2023-07-19T17:32:53Z

@jakubp-eliatra Thats okay, the goal here is to document the change that you are making and add any additional context you have about the capabilities service too. Its a part of the PR checklist for that reason. It does not have to be a comprehensive doc on the capabilities service as a whole, that can be covered in #2575. The problem is that we may never get to it otherwise and kinda have to rely on changes like this to slowly tackle away at the documentation gaps. Imagine how it would have benefitted you if the last person who touched the capabilities service had documented it well.

…nants Signed-off-by: Kajetan Nobel <k.nobel@routegroup.pl>

Signed-off-by: Kajetan Nobel <k.nobel@routegroup.pl>

kajetan-nobel · 2023-07-28T12:51:10Z

Hey @ashwin-pc, I've taken this task over from @jakubp-eliatra.

I've added docs (please let me know if there is any lack there), also integration tests have been prepared here:
opensearch-project/opensearch-dashboards-functional-test#792

…nants Signed-off-by: Kajetan Nobel <kajetan.nobel@eliatra.com>

Signed-off-by: Kajetan Nobel <kajetan.nobel@eliatra.com>

kajetan-nobel · 2023-09-27T08:27:51Z

@kajetan-nobel This is missing a changelog entry.

@joshuarrrr I added :)

ashwin-pc · 2023-10-03T00:10:04Z

@kajetan-nobel how can i validate these changes? can you add some instructions to run this change locally and make sure it works as expected?

kajetan-nobel · 2023-10-03T07:47:57Z

@ashwin-pc

Switch the dashboard branch to the branch of this PR
Switch the security plugin branch to the branch of Show controls as read only based on tenant permissions security-dashboards-plugin#1472
Sign in as Admin and follow this part of docs
Create an example of data for the tenant as admin
Sign in to the read-only tenant user who isn't an admin
It shouldn't show any edit buttons.

Signed-off-by: Kajetan Nobel <kajetan.nobel@eliatra.com>

kajetan-nobel · 2023-10-10T10:35:16Z

Note: @ashwin-pc gonna pick it up right after 2.11's crunch time
If could also any other maintainers of this repository review this PR for me please? Thank you.

SuZhou-Joe · 2023-10-12T09:37:43Z

src/core/server/security/readonly_service.test.ts

+/*
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ *
+ * Any modifications Copyright OpenSearch Contributors. See
+ * GitHub history for details.
+ */
+
+/*
+ * Licensed to Elasticsearch B.V. under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch B.V. licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */


The license header is not correct I suppose.

Yeah this is the right license header

/* * Copyright OpenSearch Contributors * SPDX-License-Identifier: Apache-2.0 */

The same goes for all the license headers in this PR

SuZhou-Joe · 2023-10-12T09:37:57Z

src/core/server/security/readonly_service.ts

+/*
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ *
+ * Any modifications Copyright OpenSearch Contributors. See
+ * GitHub history for details.
+ */
+
+/*
+ * Licensed to Elasticsearch B.V. under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch B.V. licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */


The license header is not correct I suppose.

ashwin-pc

The PR looks good aside from the license headers. I see some additional fetch errors on a few pages when using a read only user but that isnt related to this PR i think. I can approve once the license headers are fixed :)

ashwin-pc · 2023-10-13T01:45:49Z

src/core/server/security/readonly_service.test.ts

+/*
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ *
+ * Any modifications Copyright OpenSearch Contributors. See
+ * GitHub history for details.
+ */
+
+/*
+ * Licensed to Elasticsearch B.V. under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch B.V. licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */


Yeah this is the right license header

/* * Copyright OpenSearch Contributors * SPDX-License-Identifier: Apache-2.0 */

The same goes for all the license headers in this PR

Signed-off-by: Kajetan Nobel <kajetan.nobel@eliatra.com>

kajetan-nobel · 2023-10-24T14:01:40Z

@ashwin-pc @SuZhou-Joe license headers have been changed. Ready to review.

SuZhou-Joe · 2023-10-26T03:48:41Z

src/plugins/discover/server/plugin.ts

@@ -36,6 +36,15 @@ import { searchSavedObjectType } from './saved_objects';
 export class DiscoverServerPlugin implements Plugin<object, object> {
  public setup(core: CoreSetup) {
    core.capabilities.registerProvider(capabilitiesProvider);
+    core.capabilities.registerSwitcher(async (request, capabilites) => {


As there is no consumer under the line of await, I'd recommend removing the async / await in this function, not a big concern.

@SuZhou-Joe hideForReadonly is async and it's needed to be due to implementation from security plugin site

Actually because there is no consumer following the await, so it's the same to make it from async / await to a simple () => core.security.readonlyService.... because both will return a promise to the function caller. but still, not a big concern.

I would rather keep it as it is

peternied

Thanks for this improvement to dashboards - I've just finished reviewing the companion change in the security plugin - very nice design + docs 🥇!

Minor suggestion to the service interface, see inline.

peternied · 2023-10-26T18:47:39Z

src/core/server/security/readonly_service.ts

+import { IReadOnlyService } from './types';
+
+export class ReadonlyService implements IReadOnlyService {
+  async isReadonly(request: OpenSearchDashboardsRequest): Promise<boolean> {


What do you think of changing the output from being boolean to be an enum that has values like ReadOnly, ReadWrite, Unknown?

I think this would better describe results for inline reading especially in the controllers where its hard to 'see' what the impact of a true vs false will be.

@peternied then it wouldn't be a ReadonlyService but TenantCapabilityService or something like that. As long as it has an impact only on capabilities I suggest keeping it as it is right now and extending that in the next PR :) Especially as long capabilities are only boolean and will not contain enum values.

CHANGELOG.md

ananzh · 2023-10-26T19:40:19Z

src/core/server/security/readonly_service.ts

+import { IReadOnlyService } from './types';
+
+export class ReadonlyService implements IReadOnlyService {
+  async isReadonly(request: OpenSearchDashboardsRequest): Promise<boolean> {


Seems isReadonly method currently always returns false and the hideForReadonly method will always return the capabilites argument as-is, without merging it with hideCapabilities. Seems to me that these functions are redundant or unnecessary. Is the current isReadonly method a placeholder, intended to be overridden or expanded upon in the future? should we add a comment on them?

@ananzh purpose is to keep working even without security-plugin (btw. here is an extension of the implementation https://github.com/opensearch-project/security-dashboards-plugin/pull/14720). For more context please follow opensearch-project/security#2701 (comment)

Co-authored-by: Anan Zhuang <ananzh@amazon.com> Signed-off-by: Kajetan Nobel <k.nobel@routegroup.pl>

* merge conflict resolved * Restore config/opensearch_dashboards.yml * Fix capabilities tsc * docs: read only tenant mode * feat: introduce security service in core and readonly service * fix: adds securityServiceMock * feat: adds tests and default default readonly service * docs: fill up docs for read only tenant mode --------- Signed-off-by: jakubp-eliatra <jakub.przybylski@theworkingroup.net> Signed-off-by: Kajetan Nobel <k.nobel@routegroup.pl> Signed-off-by: Kajetan Nobel <kajetan.nobel@eliatra.com> Co-authored-by: jakubp-eliatra <jakub.przybylski@theworkingroup.net> Co-authored-by: Kajetan Nobel <k.nobel@routegroup.pl> Co-authored-by: Peter Nied <peternied@hotmail.com> Co-authored-by: Ashwin P Chandran <ashwinpc@amazon.com> Co-authored-by: Kajetan Nobel <kajetan.nobel@eliatra.com> Co-authored-by: Anan Zhuang <ananzh@amazon.com> (cherry picked from commit abc97ea) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md

* merge conflict resolved * Restore config/opensearch_dashboards.yml * Fix capabilities tsc * docs: read only tenant mode * feat: introduce security service in core and readonly service * fix: adds securityServiceMock * feat: adds tests and default default readonly service * docs: fill up docs for read only tenant mode --------- Signed-off-by: jakubp-eliatra <jakub.przybylski@theworkingroup.net> Signed-off-by: Kajetan Nobel <k.nobel@routegroup.pl> Signed-off-by: Kajetan Nobel <kajetan.nobel@eliatra.com> Co-authored-by: jakubp-eliatra <jakub.przybylski@theworkingroup.net> Co-authored-by: Kajetan Nobel <k.nobel@routegroup.pl> Co-authored-by: Peter Nied <peternied@hotmail.com> Co-authored-by: Ashwin P Chandran <ashwinpc@amazon.com> Co-authored-by: Kajetan Nobel <kajetan.nobel@eliatra.com> Co-authored-by: Anan Zhuang <ananzh@amazon.com> (cherry picked from commit abc97ea) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

jakubp-eliatra changed the title ~~Read Only Tenant Mod~~ Read Only Tenant Mode Jul 5, 2023

github-actions bot added the first-time-contributor label Jul 5, 2023

merge conflict resolved

b5fd84b

Signed-off-by: jakubp-eliatra <jakub.przybylski@theworkingroup.net>

jakubp-eliatra force-pushed the read-only-mode-for-tenants branch from e3252fe to b5fd84b Compare July 5, 2023 12:54

kajetan-nobel added 3 commits July 18, 2023 22:23

Restore config/opensearch_dashboards.yml

34b6105

Signed-off-by: Kajetan Nobel <k.nobel@routegroup.pl>

Fix capabilities tsc

ad6bac0

Signed-off-by: Kajetan Nobel <k.nobel@routegroup.pl>

Merge remote-tracking branch 'origin/main' into read-only-mode-for-te…

39a08e6

…nants Signed-off-by: Kajetan Nobel <k.nobel@routegroup.pl>

kajetan-nobel force-pushed the read-only-mode-for-tenants branch from 58459a9 to 39a08e6 Compare July 18, 2023 20:24

kajetan-nobel mentioned this pull request Jul 26, 2023

Read-only tenant mode tests opensearch-project/opensearch-dashboards-functional-test#792

Merged

1 task

kajetan-nobel added 2 commits July 27, 2023 18:27

Merge remote-tracking branch 'origin/main' into read-only-mode-for-te…

8e34af2

…nants Signed-off-by: Kajetan Nobel <k.nobel@routegroup.pl>

docs: read only tenant mode

3f7799a

Signed-off-by: Kajetan Nobel <k.nobel@routegroup.pl>

jakubp-eliatra marked this pull request as ready for review July 28, 2023 12:45

jakubp-eliatra requested review from ananzh, kavilla, seanneumann, AMoo-Miki, ashwin-pc, joshuarrrr, abbyhu2000, zengyan-amazon, kristenTian, zhongnansu, manasvinibs, ZilongX, Flyingliuhub and BSFishy as code owners July 28, 2023 12:45

kajetan-nobel added 2 commits September 27, 2023 10:19

Merge remote-tracking branch 'origin/main' into read-only-mode-for-te…

4899570

…nants Signed-off-by: Kajetan Nobel <kajetan.nobel@eliatra.com>

docs: update changelog

77d3e69

Signed-off-by: Kajetan Nobel <kajetan.nobel@eliatra.com>

ashwin-pc removed the needs more info Requires more information from poster label Oct 3, 2023

kajetan-nobel mentioned this pull request Oct 10, 2023

Show controls as read only based on tenant permissions opensearch-project/security-dashboards-plugin#1472

Merged

Merge branch 'main' into read-only-mode-for-tenants

79da52d

Signed-off-by: Kajetan Nobel <kajetan.nobel@eliatra.com>

SuZhou-Joe reviewed Oct 12, 2023

View reviewed changes

ashwin-pc reviewed Oct 13, 2023

View reviewed changes

kajetan-nobel added 2 commits October 23, 2023 10:10

Merge branch 'main' into read-only-mode-for-tenants

75b755a

Signed-off-by: Kajetan Nobel <kajetan.nobel@eliatra.com>

feat: change to correct license headers

66100c0

Signed-off-by: Kajetan Nobel <kajetan.nobel@eliatra.com>

ashwin-pc previously approved these changes Oct 25, 2023

View reviewed changes

SuZhou-Joe reviewed Oct 26, 2023

View reviewed changes

SuZhou-Joe approved these changes Oct 26, 2023

View reviewed changes

peternied reviewed Oct 26, 2023

View reviewed changes

ananzh reviewed Oct 26, 2023

View reviewed changes

Update CHANGELOG.md

7f5ad08

Co-authored-by: Anan Zhuang <ananzh@amazon.com> Signed-off-by: Kajetan Nobel <k.nobel@routegroup.pl>

kajetan-nobel dismissed ashwin-pc’s stale review via 7f5ad08 October 30, 2023 19:18

ananzh approved these changes Oct 31, 2023

View reviewed changes

ashwin-pc approved these changes Nov 6, 2023

View reviewed changes

ananzh merged commit abc97ea into opensearch-project:main Nov 6, 2023
63 of 65 checks passed

joshuarrrr added the backport 2.x label Nov 6, 2023

ananzh added the v2.12.0 label Nov 6, 2023

opensearch-trigger-bot bot mentioned this pull request Nov 6, 2023

[Backport 2.x] Read Only Tenant Mode #5436

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read Only Tenant Mode #4498

Read Only Tenant Mode #4498

jakubp-eliatra commented Jul 5, 2023

ashwin-pc commented Jul 5, 2023

jakubp-eliatra commented Jul 6, 2023

ashwin-pc commented Jul 19, 2023

kajetan-nobel commented Jul 28, 2023

kajetan-nobel commented Sep 27, 2023

ashwin-pc commented Oct 3, 2023

kajetan-nobel commented Oct 3, 2023

kajetan-nobel commented Oct 10, 2023

SuZhou-Joe Oct 12, 2023

ashwin-pc Oct 13, 2023

SuZhou-Joe Oct 12, 2023

ashwin-pc left a comment

ashwin-pc Oct 13, 2023

kajetan-nobel commented Oct 24, 2023

SuZhou-Joe Oct 26, 2023

kajetan-nobel Oct 26, 2023

SuZhou-Joe Oct 30, 2023 •

edited

Loading

kajetan-nobel Oct 30, 2023

peternied left a comment

peternied Oct 26, 2023

kajetan-nobel Oct 30, 2023 •

edited

Loading

ananzh Oct 26, 2023

kajetan-nobel Oct 30, 2023

Read Only Tenant Mode #4498

Read Only Tenant Mode #4498

Conversation

jakubp-eliatra commented Jul 5, 2023

Description

Issues Resolved

Check List

ashwin-pc commented Jul 5, 2023

jakubp-eliatra commented Jul 6, 2023

ashwin-pc commented Jul 19, 2023

kajetan-nobel commented Jul 28, 2023

kajetan-nobel commented Sep 27, 2023

ashwin-pc commented Oct 3, 2023

kajetan-nobel commented Oct 3, 2023

kajetan-nobel commented Oct 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashwin-pc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kajetan-nobel commented Oct 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuZhou-Joe Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peternied left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kajetan-nobel Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuZhou-Joe Oct 30, 2023 •

edited

Loading

kajetan-nobel Oct 30, 2023 •

edited

Loading