Fixes Short URL redirection for SAML login

[devardee]

Description

Resolves a bug where SAML endpoints login flow was broken when attempted to login via short url.

Category

Bug fix

Why these changes are required?

To allow short urls to work successfully for SAML SSO logins.

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

Old: Short URL would throw 400 when try to initiate login for SAML endpoints
New: Short URL redirects to SAML login properly

Issues Resolved

• Resolves [BUG] 1.3.12 Short URL raises 400 error during SAML login #1627

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.

cc: @anijain-Amazon

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b1e986c) 67.27% compared to head (95695e4) 67.27%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1744   +/-   ##
=======================================
  Coverage   67.27%   67.27%           
=======================================
  Files          94       94           
  Lines        2408     2408           
  Branches      320      320           
=======================================
  Hits         1620     1620           
  Misses        711      711           
  Partials       77       77           

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

[peternied]

Thanks for the contribution, lets update the saml test cases to make sure this scenario is handled correctly with this change.

[DarshitChanpura]

@devardee to add to @peternied's comment, could you please update the PR title/description to be more detailed?

[devardee]

lets update the saml test cases to make sure this scenario is handled correctly with this change.

@peternied can you please guide me on what kind of testing needs to be added here ?

@DarshitChanpura the linked GH issue is very descriptive, can I copy/paste from there ?

[devardee]

I see there are end-to-end tests here, however, I am unable to run them locally, can the team pls help me on setting up my IDE, so that I can added new tests

[cwperks]

@devardee To run those tests:

1. Create a local distribution of core and install the security plugin with the demo configuration
2. Replace the demo configuration's config.yml with the config.yml in this workflow.
3. Start OpenSearch
4. Clone OSD and also clone the security-dashboards-plugin in the plugins folder of OSD
5. Configure config/opensearch_dashboards.yml with the values present in this workflow
6. Start Dashboards
7. In a third terminal window navigate to OpenSearch-Dashboards/plugins/security-dashboards-plugin and run the command from this line: yarn cypress:run --browser chrome --headless --spec "test/cypress/e2e/saml/*.js"

You may also need to start the node_idp locally to. In the OpenSearch-Dashboards/plugins/security-dashboards-plugin run: yarn pretest:jest_server. Generally, I start the IdP before starting OpenSearch so that OpenSearch can fetch the metadata from http://localhost:7000/metadata

[DarshitChanpura]

@DarshitChanpura the linked GH issue is very descriptive, can I copy/paste from there ?

I've made changes to the description. Feel free to correct if anything is missing.

[devardee]

@peternied , @DarshitChanpura I have added IT. Please take a look

[devardee]

the newly added test is failing in multi-auth test workflow 😔, can anyone please help me out on why that particular test is failing.

Not able to understand the failure from logs :

AssertionError: expected null to exist
      at Context.eval (webpack://opensearch-security-dashboards/./test/cypress/e2e/saml/saml_auth_test.spec.js:134:46)

[cwperks]

@devardee I made some slight modifications to the test to first login, then to use cy.request instead of cy.visit like this:

it('Login to Dashboard with Goto URL', () => {
  localStorage.setItem('home:newThemeModal:show', 'false');

  cy.visit('http://localhost:5601/app/opensearch_dashboards_overview', {
    failOnStatusCode: false,
  });

  samlLogin();

  cy.shortenUrl(SHORTEN_URL_DATA, 'global').then((response) => {
    const gotoUrl = `http://localhost:5601/goto/${response.urlId}?security_tenant=global`;
    cy.request(gotoUrl);
    cy.getCookie('security_authentication').should('exist');
  });
});

cy.request will make a request to the shortened URL using the cookies currently stored in the browser and those cookies are set in the browser after login.

[devardee]

@cwperks , thanks for the help 😊. All the tests passed locally. Hope it pass in the github CI as well

[cwperks]

@devardee Did you try the suggestion from this comment? Login first, then navigate to shortUrl?

[devardee]

Hi Team, pls take a look, github ci has passed

cc: @cwperks

[DarshitChanpura]

LGTM. Thanks @devardee !

[stephen-crawford]

Great job!

Other maintains have been following up - great work @devardee

[wbeckler]

It would be awesome if this could make it into 2.12