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

Custom Branding: Relative URL should be allowed #5572

Merged
merged 8 commits into from
Dec 15, 2023

Conversation

Alankarsharma
Copy link
Contributor

Description

We have open search running behind a reverse proxy along with other service (Logo is deployed here). From OpenSearch server external URLs are not accessible so if we give this value then Dashboard on startup are not able to fetch the logo and revert to original, if we enter internal URL then server starts properly but logo are not loaded as internal URL is not accessible from external network.

Here if relative URL is passed returning true so that server can start properly and logos are also loaded properly in browser.

Workaround like hostname mapping work only if both internal and external service are deployed on HTTP or HTTPS, but in case of SSL offloading this also fails.

Issues Resolved

Screenshot

Testing the changes

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

@Alankarsharma Alankarsharma changed the title Relative URL should be allowed Custom Branding: Relative URL should be allowed Dec 5, 2023
Copy link

codecov bot commented Dec 6, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9ba5cf5) 66.98% compared to head (e8cb4eb) 66.98%.
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5572      +/-   ##
==========================================
- Coverage   66.98%   66.98%   -0.01%     
==========================================
  Files        3293     3293              
  Lines       63294    63296       +2     
  Branches    10067    10068       +1     
==========================================
  Hits        42396    42396              
- Misses      18458    18459       +1     
- Partials     2440     2441       +1     
Flag Coverage Δ
Linux_1 35.24% <ø> (ø)
Linux_2 55.18% <100.00%> (+<0.01%) ⬆️
Linux_3 43.78% <0.00%> (-0.01%) ⬇️
Linux_4 35.34% <0.00%> (-0.01%) ⬇️
Windows_1 35.27% <ø> (ø)
Windows_2 55.14% <100.00%> (+<0.01%) ⬆️
Windows_3 43.80% <0.00%> (-0.01%) ⬇️
Windows_4 35.34% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@kavilla
Copy link
Member

kavilla commented Dec 7, 2023

Hello @Alankarsharma, thanks for opening!

Could you please sign the commit? https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/CONTRIBUTING.md#developer-certificate-of-origin.

I merged the branch with main to recheck the tests so you might have to pull. But you can amend the commit.

@kavilla
Copy link
Member

kavilla commented Dec 7, 2023

cc: @abbyhu2000 can you check this out?

Alankarsharma and others added 4 commits December 8, 2023 23:23
Signed-off-by: Alankarsharma <[email protected]>
…t#5579)

Signed-off-by: Josh Romero <[email protected]>
(cherry picked from commit 39fdcad)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Signed-off-by: Alankarsharma <[email protected]>
…opensearch-project#5577)

* [BUG][Data] Support for custom filters with heterogeneous data fields

When enabling the advanced setting `courier:ignoreFilterIfFieldNotInIndex`
Custom OpenSearch Query DSL filters could technically be applied to index
patterns that map to indices that are not exactly the same. Since the
custom query filter is a user input then users can really type anything
that they need. Or any field that they know is present but we do not know
for sure.

Therefore, we can check if the id which is the index pattern title to check
if we should apply the filter or not.

Issue resolved:
https://github.com/opensearch-project/dashboards-visualizations/issues/281

I believe issue:
opensearch-project#5423

Should closed as that is expected functionality.

Signed-off-by: Kawika Avilla <[email protected]>

* [Cleanup] utilize the same helper function

Originally when implementing the fix the historical comment caused concern about
potential breaking changes.

But after discussion, we decided it is more clear to consolidate the helper functions.

Signed-off-by: Kawika Avilla <[email protected]>
Signed-off-by: Alankarsharma <[email protected]>
Fixed it, added test case for this as well.

Signed-off-by: Alankarsharma <[email protected]>
@@ -171,7 +171,7 @@ export class RenderingService {
};
}

public async stop() {}
public async stop() { }
Copy link
Member

Choose a reason for hiding this comment

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

Could you delete the empty space here? it causes a linter error @Alankarsharma

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thanks for the review

@abbyhu2000
Copy link
Member

Just a linter error, otherwise LGTM!

Signed-off-by: Alankarsharma <[email protected]>
@abbyhu2000
Copy link
Member

@Alankarsharma Could you please also add the change in the Changelog.md?

Signed-off-by: Alankarsharma <[email protected]>
@Alankarsharma
Copy link
Contributor Author

@Alankarsharma Could you please also add the change in the Changelog.md?

Done

@SuZhou-Joe SuZhou-Joe self-assigned this Dec 15, 2023
Copy link
Member

@SuZhou-Joe SuZhou-Joe left a comment

Choose a reason for hiding this comment

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

Overall, LGTM and relative url should not introduce any extra security concern like ssrf as we've already do a suffix check beforehand and this config can only be set by cluster admin in yml file.

@ananzh ananzh merged commit d8cbc17 into opensearch-project:main Dec 15, 2023
59 of 61 checks passed
@ananzh ananzh added branding enhancement New feature or request labels Dec 15, 2023
opensearch-trigger-bot bot added a commit that referenced this pull request Dec 15, 2023
When enabling the advanced setting `courier:ignoreFilterIfFieldNotInIndex`
Custom OpenSearch Query DSL filters could technically be applied to index
patterns that map to indices that are not exactly the same. Since the
custom query filter is a user input then users can really type anything
that they need. Or any field that they know is present but we do not know
for sure.

Therefore, we can check if the id which is the index pattern title to check
if we should apply the filter or not.

Issue resolved:
https://github.com/opensearch-project/dashboards-visualizations/issues/281

Issue could be closed:
#5423

---------

Signed-off-by: Alankarsharma <[email protected]>
Signed-off-by: Kawika Avilla <[email protected]>
Co-authored-by: opensearch-trigger-bot[bot] <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Kawika Avilla <[email protected]>
Co-authored-by: Qingyang(Abby) Hu <[email protected]>
(cherry picked from commit d8cbc17)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@Alankarsharma Alankarsharma deleted the patch-1 branch December 16, 2023 16:33
abbyhu2000 added a commit that referenced this pull request Dec 19, 2023
When enabling the advanced setting `courier:ignoreFilterIfFieldNotInIndex`
Custom OpenSearch Query DSL filters could technically be applied to index
patterns that map to indices that are not exactly the same. Since the
custom query filter is a user input then users can really type anything
that they need. Or any field that they know is present but we do not know
for sure.

Therefore, we can check if the id which is the index pattern title to check
if we should apply the filter or not.

Issue resolved:
https://github.com/opensearch-project/dashboards-visualizations/issues/281

Issue could be closed:
#5423

---------







(cherry picked from commit d8cbc17)

Signed-off-by: Alankarsharma <[email protected]>
Signed-off-by: Kawika Avilla <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: opensearch-trigger-bot[bot] <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com>
Co-authored-by: Kawika Avilla <[email protected]>
Co-authored-by: Qingyang(Abby) Hu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants