Replace _opendistro route with _plugins #895

pc-jedi · 2022-01-31T11:05:59Z

Description

In the documentation for SAML the SSO URL and the white listed URL is already updated to _plugins/_security while the routes in the security plugin still are on _opendistro/_security. That causes a 404 response when trying to login.
This PR changes the route in the plugin and aligns with the documentation.

Why these changes are required?

To align with the new naming schema and have it working like it is documented.

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

Before this PR the whitelisted URLs for SSO have to be set to _opendistro/_security/.. also the configuration in the IdP to forward to the correct endpoint had to be _security/saml/acs.

New behavior is that the URLs are now aligned with the documentation.

Issues Resolved

#836

Testing

[Please provide details of testing done: unit testing, integration testing and manual testing]

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.

pc-jedi · 2022-02-03T10:45:03Z

@davidlago Could you have a look, please.

davidlago · 2022-02-03T16:32:02Z

This is a breaking change. Even though lagging behind the documentation, we should add the new named endpoint (and preserve the old one as deprecated). We can remove when 2.0 goes out.

pc-jedi · 2022-02-07T15:37:57Z

This is a breaking change. Even though lagging behind the documentation, we should add the new named endpoint (and preserve the old one as deprecated). We can remove when 2.0 goes out.

Makes sense. Do you know of a functionality to have multiple path defined like with an array, or should I simply do a copy + paste of the 2 routes?

davidlago · 2022-02-07T19:21:10Z

It looks like it is just a single value: https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/core/server/http/router/router.ts#L58

pc-jedi · 2022-02-08T09:21:55Z

It looks like it is just a single value: https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/core/server/http/router/router.ts#L58

Ok, then I will try to figure out a way to move the handler function into a separate function. Only problem I see is that we would need to bind the this and that the function parameter cannot have type inference and we have to specify the type explicit. If this would be ok, I'll change the PR towards that.

davidlago · 2022-02-12T18:08:37Z

UTs failing and it looks like this change that was breaking in 2.0 is affecting us here. It should be a quick fix, making the license field an array like in the link above.

davidlago · 2022-02-12T18:11:18Z

Please provide details as to how this was tested, thanks!

pc-jedi · 2022-03-10T08:59:32Z

Please provide details as to how this was tested, thanks!

Sorry for taking so long.
I had setup a local SAML IdP in a Docker container together with OpenSearch with security plugin and the OpenSearch Dashboards with a version of the plugin. Then I tested both URLs as endpoint to check if both are still working, which was the case.
I will fix the merge conflicts. Anything else I should take care of?

davidlago · 2022-03-14T14:12:30Z

@pc-jedi thanks for the help! I am sorry for the back and forth, but we are already starting to build 2.0 in main as we are trying to get out a preview version by the end of the month. That means that this PR targeting main no longer needs to worry about preserving the old deprecated route. Could you please remove that old route from the PR? Other than that this should be good to merge. Thanks again for the work on this one!!

pc-jedi · 2022-03-14T15:05:56Z

@davidlago No worries. I have rebased and squash all things together. All routes should now be _plugins/security. Also I replaced some strings with the already existing constants.

pc-jedi · 2022-03-14T16:03:47Z

I guess the integration test failed, because this plugin was just bumped to 1.3.0 and Dashboards 1.x branch is already at 1.4.0?
opensearch-project/OpenSearch-Dashboards#1341

I think we have to wait for #892

sharvil-parekh · 2022-03-24T10:19:10Z

Any timelines when the PR will be merged?

DarshitChanpura · 2022-03-25T15:24:07Z

I guess the integration test failed, because this plugin was just bumped to 1.3.0 and Dashboards 1.x branch is already at 1.4.0? opensearch-project/OpenSearch-Dashboards#1341

I think we have to wait for #892

There is a new PR for 2.0.0.0 bump: #928

Signed-off-by: Christian Düfel <[email protected]>

pc-jedi · 2022-04-26T05:16:03Z

Rebased. Please have a look again.

pc-jedi · 2022-05-04T09:35:47Z

Any plans to merge this so we could have this still in for 2.0? Or is it already too late?

DarshitChanpura · 2022-05-16T16:37:34Z

This build can be re-run once #989 is merged.

pc-jedi · 2022-05-23T06:44:54Z

Last build went through. Should I bump it again to latest main?

JustinasKO · 2022-07-12T13:26:25Z

FYI: this is breaking change and was included in minor version 2.1.0. Not sure if this is good practice. For minor version both endpoints (_plugins/_security/..., _opendistro/_security/...) should have be left working.
Not to mentions that its not even work with OpenSearch v2.1.0

devardee · 2022-07-12T19:20:40Z

@cliu123, @peternied we also need to make corresponding changes in the buildAssertionConsumerEndpoint function in the security plugin as well https://github.com/devardee/security-1/blob/be876c0078b23dc6567f6bd2f585d5f14245b201/src/main/java/com/amazon/dlic/auth/http/saml/Saml2SettingsProvider.java#L217-L219

varun-lodaya · 2022-07-14T13:21:09Z

@cliu123 @peternied are we working on fixing this? Seems this is breaking the SAML flow currently

This reverts commit e4e4032.

This reverts commit e4e4032. (cherry picked from commit cc5b763)

This reverts commit cc5b763.

This reverts commit e4e4032. (cherry picked from commit cc5b763) Co-authored-by: Darshit Chanpura <[email protected]>

…)" (opensearch-project#1035) This reverts commit e4e4032. Signed-off-by: Vasile Negru <[email protected]>

…roject#895)" (opensearch-project#1035)" This reverts commit c456883362610c61fcc5d54b2974d7a5c6327c1d. Signed-off-by: Vasile Negru <[email protected]> Signed-off-by: Vasile Negru <[email protected]>

…)" (opensearch-project#1035) This reverts commit e4e4032. Signed-off-by: Vasile Negru <[email protected]> Signed-off-by: Vasile Negru <[email protected]>

…roject#895)" (opensearch-project#1035)" This reverts commit c456883362610c61fcc5d54b2974d7a5c6327c1d. Signed-off-by: Vasile Negru <[email protected]>

pc-jedi requested a review from a team January 31, 2022 11:06

pc-jedi force-pushed the fix-routes branch from 32f9257 to 56f1df4 Compare January 31, 2022 11:09

pc-jedi force-pushed the fix-routes branch from 24ce203 to ecbc65c Compare February 14, 2022 09:49

pc-jedi closed this Mar 14, 2022

pc-jedi deleted the fix-routes branch March 14, 2022 14:57

pc-jedi restored the fix-routes branch March 14, 2022 14:57

pc-jedi reopened this Mar 14, 2022

pc-jedi force-pushed the fix-routes branch 2 times, most recently from 8d9426e to b4362b6 Compare March 14, 2022 15:03

DarshitChanpura previously approved these changes Mar 25, 2022

View reviewed changes

pc-jedi dismissed DarshitChanpura’s stale review via 744bfb2 April 19, 2022 14:15

pc-jedi force-pushed the fix-routes branch from b4362b6 to 744bfb2 Compare April 19, 2022 14:15

Replace _opendistro route with _plugins

388706b

Signed-off-by: Christian Düfel <[email protected]>

pc-jedi force-pushed the fix-routes branch from 744bfb2 to 388706b Compare April 26, 2022 05:15

Merge branch 'main' into fix-routes

dcbb401

Merge branch 'main' into fix-routes

ba1f676

Merge branch 'main' into fix-routes

994b7b1

peternied approved these changes May 25, 2022

View reviewed changes

cliu123 approved these changes May 25, 2022

View reviewed changes

cliu123 merged commit e4e4032 into opensearch-project:main May 25, 2022

JustinasKO mentioned this pull request Jul 12, 2022

[BUG] SAML endpoint still using _opendistro instead of _plugins #836

Open

hpkuppuraj mentioned this pull request Jul 13, 2022

Update SAML Endpoint opensearch-project/security#1936

Closed

3 tasks

DarshitChanpura added a commit that referenced this pull request Jul 18, 2022

Revert "Replace _opendistro route with _plugins (#895)"

a3fc7ac

This reverts commit e4e4032.

DarshitChanpura mentioned this pull request Jul 18, 2022

Revert "Replace _opendistro route with _plugins" #1035

Merged

DarshitChanpura added a commit that referenced this pull request Jul 20, 2022

Revert "Replace _opendistro route with _plugins (#895)" (#1035)

cc5b763

This reverts commit e4e4032.

opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 20, 2022

Revert "Replace _opendistro route with _plugins (#895)" (#1035)

f3a32a1

This reverts commit e4e4032. (cherry picked from commit cc5b763)

opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 20, 2022

Revert "Replace _opendistro route with _plugins (#895)" (#1035)

4228f60

This reverts commit e4e4032. (cherry picked from commit cc5b763)

DarshitChanpura added a commit that referenced this pull request Jul 20, 2022

Revert "Revert "Replace _opendistro route with _plugins (#895)" (#1035)"

a506d7a

This reverts commit cc5b763.

DarshitChanpura added a commit that referenced this pull request Jul 20, 2022

Revert "Replace _opendistro route with _plugins (#895)" (#1035) (#1037)

b0f5cc8

This reverts commit e4e4032. (cherry picked from commit cc5b763) Co-authored-by: Darshit Chanpura <[email protected]>

DarshitChanpura added a commit that referenced this pull request Jul 20, 2022

Revert "Replace _opendistro route with _plugins (#895)" (#1035) (#1036)

89a8560

This reverts commit e4e4032. (cherry picked from commit cc5b763) Co-authored-by: Darshit Chanpura <[email protected]>

cwperks mentioned this pull request Aug 30, 2022

SAML auth uses legacy _opendistro route opensearch-project/security#2060

Open

Desvelao mentioned this pull request Apr 24, 2023

Azure Active Directory SSO documentation wrong paths wazuh/wazuh-documentation#6066

Closed

cwperks mentioned this pull request Aug 15, 2023

[BUG] v2.1 security plugin is still using _opendistro/_security/saml/acs #1031

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace _opendistro route with _plugins #895

Replace _opendistro route with _plugins #895

pc-jedi commented Jan 31, 2022 •

edited

Loading

pc-jedi commented Feb 3, 2022

davidlago commented Feb 3, 2022

pc-jedi commented Feb 7, 2022 •

edited

Loading

davidlago commented Feb 7, 2022

pc-jedi commented Feb 8, 2022

davidlago commented Feb 12, 2022

davidlago commented Feb 12, 2022

pc-jedi commented Mar 10, 2022

davidlago commented Mar 14, 2022

pc-jedi commented Mar 14, 2022 •

edited

Loading

pc-jedi commented Mar 14, 2022 •

edited

Loading

sharvil-parekh commented Mar 24, 2022

DarshitChanpura commented Mar 25, 2022

pc-jedi commented Apr 26, 2022

pc-jedi commented May 4, 2022

DarshitChanpura commented May 16, 2022

pc-jedi commented May 23, 2022

JustinasKO commented Jul 12, 2022 •

edited

Loading

devardee commented Jul 12, 2022

varun-lodaya commented Jul 14, 2022

Replace _opendistro route with _plugins #895

Replace _opendistro route with _plugins #895

Conversation

pc-jedi commented Jan 31, 2022 • edited Loading

Description

Category

Why these changes are required?

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

Issues Resolved

Testing

Check List

pc-jedi commented Feb 3, 2022

davidlago commented Feb 3, 2022

pc-jedi commented Feb 7, 2022 • edited Loading

davidlago commented Feb 7, 2022

pc-jedi commented Feb 8, 2022

davidlago commented Feb 12, 2022

davidlago commented Feb 12, 2022

pc-jedi commented Mar 10, 2022

davidlago commented Mar 14, 2022

pc-jedi commented Mar 14, 2022 • edited Loading

pc-jedi commented Mar 14, 2022 • edited Loading

sharvil-parekh commented Mar 24, 2022

DarshitChanpura commented Mar 25, 2022

pc-jedi commented Apr 26, 2022

pc-jedi commented May 4, 2022

DarshitChanpura commented May 16, 2022

pc-jedi commented May 23, 2022

JustinasKO commented Jul 12, 2022 • edited Loading

devardee commented Jul 12, 2022

varun-lodaya commented Jul 14, 2022

pc-jedi commented Jan 31, 2022 •

edited

Loading

pc-jedi commented Feb 7, 2022 •

edited

Loading

pc-jedi commented Mar 14, 2022 •

edited

Loading

pc-jedi commented Mar 14, 2022 •

edited

Loading

JustinasKO commented Jul 12, 2022 •

edited

Loading