Skip to content

Remove multi-tenant path check in auth handler#1151

Merged
cliu123 merged 8 commits intoopensearch-project:mainfrom
zengyan-amazon:multi-tenant-path-fix
Oct 28, 2022
Merged

Remove multi-tenant path check in auth handler#1151
cliu123 merged 8 commits intoopensearch-project:mainfrom
zengyan-amazon:multi-tenant-path-fix

Conversation

@zengyan-amazon
Copy link
Member

@zengyan-amazon zengyan-amazon commented Oct 20, 2022

Description

The Opensearch Dashboards tenant only impacts the saved objects access. The multi-tenant path check in the auth handler was meant to by pass the resolve tenant logic for some APIs which doesn't deal with saved objects. However with the new datasource support feature in OpenSearch Dashboards, Dashboards APIs which used to not dealing with saved objects such as data queries and index resolve operations will require the new data source saved object type under the hood.

This change removes the multi-tenant path check, so that tenant header will be injected to all route handlers. So that all route handlers can work with the right tenant in case they need to deal with any saved objects.

Signed-off-by: Yan Zeng zengyan@amazon.com

Category

Bug fix

Why these changes are required?

This change is required for OpenSearch Dashboards data source support feature to work with correct tenant when querying remote data sources.

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

Old behavior: auth handler will not inject tenant header for some APIs (e.g. APIs starts with /internal)
New behavior: auth handler will inject tenant header to all OSD APIs

Issues Resolved

opensearch-project/OpenSearch-Dashboards#2634

Testing

Manually tested
Old behavior causing error getting saved objects:
image

New behavior solved the issue:
image

Check List

  • New functionality includes testing
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@zengyan-amazon zengyan-amazon requested a review from a team October 20, 2022 17:37
kristenTian
kristenTian previously approved these changes Oct 20, 2022
cliu123
cliu123 previously approved these changes Oct 21, 2022
Copy link
Member

@DarshitChanpura DarshitChanpura left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ty for submitting this PR. Could you please add a test/modify a test to check this behavior?

The Opensearch Dashboards tenant only impacts the saved objects access. The multi-tenant path check in the auth handler
was meant to by pass the resolve tenant logic for some APIs which doesn't deal with saved objects. However with the new
datasource support feature in OpenSearch Dashboards, Dashboards APIs which used to not dealing with saved objects such
as data queries and index resolve operations will require the new data source saved object type under the hood.

This change removes the multi-tenant path check, so that tenant header will be injected to all route handlers. So that
all route handlers can work with the right tenant in case they need to deal with any saved objects.

Signed-off-by: Yan Zeng <zengyan@amazon.com>
Signed-off-by: Yan Zeng <zengyan@amazon.com>
Signed-off-by: Yan Zeng <zengyan@amazon.com>
Signed-off-by: Yan Zeng <zengyan@amazon.com>
Signed-off-by: Yan Zeng <zengyan@amazon.com>
cliu123
cliu123 previously approved these changes Oct 26, 2022
@zengyan-amazon zengyan-amazon added backport 2.x backport to 2.x branch v2.4.0 'Issues and PRs related to version v2.4.0' labels Oct 26, 2022
kristenTian
kristenTian previously approved these changes Oct 26, 2022
cwperks
cwperks previously approved these changes Oct 27, 2022
@cliu123
Copy link
Member

cliu123 commented Oct 28, 2022

Integration test failed. this.esClient.asScoped is not a function, but the function is there.

FAIL server/auth/types/authentication_type.test.ts
  ● test tenant header › common API includes tenant info

    this.esClient.asScoped is not a function

      113 |         });
      114 |     } catch (error) {
    > 115 |       throw new Error(error.message);
          |             ^
      116 |     }
      117 |   }
      118 |

      at SecurityClient.authinfo (plugins/security-dashboards-plugin/server/backend/opensearch_security_client.ts:115:13)
      at DummyAuthType.authHandler (plugins/security-dashboards-plugin/server/auth/types/authentication_type.ts:207:44)
      at Object.<anonymous> (plugins/security-dashboards-plugin/server/auth/types/authentication_type.test.ts:86:20)

Signed-off-by: Yan Zeng <zengyan@amazon.com>
@codecov-commenter
Copy link

codecov-commenter commented Oct 28, 2022

Codecov Report

Merging #1151 (e64d67d) into main (a21c7be) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1151   +/-   ##
=======================================
  Coverage   72.07%   72.07%           
=======================================
  Files          88       88           
  Lines        1959     1959           
  Branches      258      258           
=======================================
  Hits         1412     1412           
  Misses        490      490           
  Partials       57       57           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@cliu123 cliu123 dismissed DarshitChanpura’s stale review October 28, 2022 18:22

@DarshitChanpura Thanks for reviewing! All the comments have been resolved. We are merging this PR.

@cliu123 cliu123 merged commit 6236fe2 into opensearch-project:main Oct 28, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 28, 2022
Signed-off-by: Yan Zeng <zengyan@amazon.com>

Co-authored-by: Chang Liu <lc12251109@gmail.com>
(cherry picked from commit 6236fe2)
cliu123 pushed a commit that referenced this pull request Oct 28, 2022
Signed-off-by: Yan Zeng <zengyan@amazon.com>

Co-authored-by: Chang Liu <lc12251109@gmail.com>
(cherry picked from commit 6236fe2)

Co-authored-by: Yan Zeng <46499415+zengyan-amazon@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport 2.x backport to 2.x branch v2.4.0 'Issues and PRs related to version v2.4.0'

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants