Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get security_tenant search param from URL #1024

Conversation

cliu123
Copy link
Member

@cliu123 cliu123 commented Jul 6, 2022

Signed-off-by: cliu123 [email protected]

Description

The root cause of the issue is that the search param(security_tenant) in the URL is not captured.

Category

[Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation] Bug fix

Issues Resolved

#1012

Testing

  • The sharing saved object functionality doesn't have UTs, so there's no suitable place to add test for this fix for now. I've created an issue to track adding tests for this functionatlity.
  • Manual Testing:
    • Create a visualization in global tenant.
    • Copy the link to share the visualization.
    • Visit the link in private tenant.
    • Browser successfully re-directs to the right saved object.(used to be {"statusCode":404,"error":"Not Found","message":"Saved object [url/<saved_object_id>] not found"})

Check List

  • 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.

@cliu123 cliu123 requested a review from a team July 6, 2022 20:37
@codecov-commenter
Copy link

codecov-commenter commented Jul 6, 2022

Codecov Report

Merging #1024 (c4bc080) into main (4527d1a) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1024   +/-   ##
=======================================
  Coverage   72.27%   72.27%           
=======================================
  Files          87       87           
  Lines        1915     1915           
  Branches      249      249           
=======================================
  Hits         1384     1384           
  Misses        478      478           
  Partials       53       53           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4527d1a...c4bc080. Read the comment docs.

@cliu123 cliu123 added the backport 2.x backport to 2.x branch label Jul 6, 2022
Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

I would prefer we treat the source of this url, see the inline comment for more details

server/multitenancy/tenant_resolver.ts Show resolved Hide resolved
@peternied peternied merged commit 348c550 into opensearch-project:main Jul 7, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 7, 2022
Signed-off-by: cliu123 <[email protected]>
(cherry picked from commit 348c550)
@cliu123 cliu123 deleted the get_security_tenant_search_param_from_url branch July 7, 2022 18:35
cliu123 added a commit that referenced this pull request Jul 7, 2022
Signed-off-by: cliu123 <[email protected]>
(cherry picked from commit 348c550)

Co-authored-by: Chang Liu <[email protected]>
spartan2015 pushed a commit to spartan2015/security-dashboards-plugin that referenced this pull request Aug 8, 2022
rutuja-amazon added a commit to rutuja-amazon/security-dashboards-plugin that referenced this pull request Aug 10, 2022
rutuja-amazon added a commit to rutuja-amazon/security-dashboards-plugin that referenced this pull request Aug 10, 2022
rutuja-amazon added a commit to rutuja-amazon/security-dashboards-plugin that referenced this pull request Aug 10, 2022
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants