Initial draft PR to introduce Integration Tests for SAML auth flow by devardee · Pull Request #1044 · opensearch-project/security-dashboards-plugin

devardee · 2022-07-28T14:48:29Z

Description

Integration test to validate SAML flow, this is a draft PR. Integ tests are based on selenium web driver.

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.

… driver

…ation in developer guide

DarshitChanpura · 2022-07-28T19:42:53Z

test/jest_integration/runIdpServer.js

+
+// Create certificate pair on the fly and pass it to runServer
+runServer({
+  acsUrl: 'http://localhost:5601/_opendistro/_security/saml/acs',


is it possible to run idp server with acs /_plugins/_security/saml/acs? or will it be covered in next iteration of this PR?

did not understand, can you please explain?

DarshitChanpura · 2022-07-28T19:44:03Z

server/auth/types/saml/routes.ts

      {
-        path: '/_plugins/_security/saml/acs',
+        // need to change to /_opendistro to execute the tests.
+        path: '/_opendistro/_security/saml/acs',


Is there a way you can test both endpoints or just the new one?

we can ignore this, since you had already merged this change

peternied

Really great work, I've added a couple of comments, but its looking very close. I noticed there were 4 scenarios called out in the saml tests but I only see 2 tests cases, were you working on implementing all 4?

peternied · 2022-07-28T19:58:23Z

test/jest.config.server.js

+  //for now only run saml integration test.
+  testMatch: ['**/test/jest_integration/**/saml_auth.tests.ts'],


Please revert

Note you can also use use test.only for quickly focusing on a single test, see https://devhints.io/jest Focusing tests

yes, I will revert this config change

peternied · 2022-07-28T20:00:42Z

test/jest_integration/saml_auth.tests.ts

+
+});
+
+function sleepFor(sleepDuration: number) {


Doesn't look used,
IMO this is the better mechanism https://decipher.dev/30-seconds-of-typescript/docs/sleep/

yes, will remove this

peternied · 2022-07-28T20:02:32Z

idp-private-key.pem

@@ -0,0 +1,28 @@
+-----BEGIN PRIVATE KEY-----


I don't see a reference, can we remove both checked in certs?

the IDP server uses this certificate to generate idp's metadata, we can acutally generate these certificates on the fly, I have just hardcoded for now

peternied · 2022-07-28T20:03:46Z

package.json

    "lint:es": "node ../../scripts/eslint",
    "lint:style": "node ../../scripts/stylelint",
    "lint": "yarn run lint:es && yarn run lint:style",
+    "pretest:jest_server": "node ./test/jest_integration/runIdpServer.js &",


Can this be started during saml_auth.tests.ts start up so it only impacts those tests it applies to?

yes, this can be done, will only start idp server for the saml tests

peternied · 2022-07-28T20:08:36Z

test/jest_integration/saml_auth.tests.ts

+        }
+      );
+    } catch (error) {
+      console.log('Got an error!!');


include that error in the logged output

peternied · 2022-07-28T20:10:48Z

test/jest_integration/saml_auth.tests.ts

+    await driver
+      .wait(
+        until.elementsLocated(
+          By.xpath("/html/body/div[1]/div/div/div/div[2]/div/main/div[1]/span/button/span")


Is there a way this can be found by id that would be cleaner?

sure, I will need to look at the DOM to see for ID's

test/jest_integration/saml_auth.tests.ts

devardee · 2022-08-02T12:56:31Z

Yes, I will add test to capture all the scenarios

peternied

I don't see any pushed changes, please update the PR so it can be reviewed.

devardee · 2022-08-03T06:47:56Z

sure will update the PR, btw can we merge this PR #1001 ?

peternied · 2022-08-09T18:59:35Z

@devardee I haven't seen any updates on this PR - could you let us know what you thoughts are? When I attempt to run the code locally I get an error that there is no gecko driver for firefox present, so it seems like there are some additional fundamentals that are missing.

  ● start OpenSearch Dashboards server › Login to app/dev_tools#/console when SAML is enabled

    The geckodriver executable could not be found on the current PATH. Please download the latest version from https://github.com/mozilla/geckodriver/releases/ and ensure it can be found on your PATH.

      87 |     // initWebDriver()
      88 |
    > 89 |     driver = new Builder().forBrowser('firefox').setFirefoxOptions({addArguments: '--headless'}).build();
         |                                                                                                  ^
      90 |
      91 |     await wreck.patch(
      92 |       'https://localhost:9200/_plugins/_security/api/rolesmapping/all_access',

      at findGeckoDriver (plugins/security-dashboards-plugin/node_modules/selenium-webdriver/firefox.js:463:11)
      at new ServiceBuilder (plugins/security-dashboards-plugin/node_modules/selenium-webdriver/firefox.js:556:22)
      at Function.createSession (plugins/security-dashboards-plugin/node_modules/selenium-webdriver/firefox.js:612:21)
      at createDriver (plugins/security-dashboards-plugin/node_modules/selenium-webdriver/index.js:146:33)
      at Builder.build (plugins/security-dashboards-plugin/node_modules/selenium-webdriver/index.js:682:16)
      at Object.<anonymous> (plugins/security-dashboards-plugin/test/jest_integration/saml_auth.test.ts:89:98)

devardee · 2022-08-10T13:52:08Z

I will resume my work on this, I am hoping to complete (refactor + adding additional test cases) by end of next week.

@peternied to run the selenium tests locally you need to install the geckodriver and add it to your PATH env variable. To run this test in CI, I was thinking of installing gecko driver via setup script and run it in Headless mode, is this the correct approach ?

peternied · 2022-08-11T23:36:13Z

I was thinking of installing gecko driver via setup script and run it in Headless mode

Please make sure you document what it takes to run the tests, as some folks are on mac/windows/linux - platform agnostic would be the best if there is an easy way to incorporate. No matter the local steps, please also update the ci.yml workflow to make sure the GitHub Action is setup with whatever it will need to run the tests.

peternied · 2022-08-22T16:32:56Z

@devardee I haven't seen any updates, anything that you need help with?

Signed-off-by: Deepak Devarakonda <devardee@amazon.com>

Signed-off-by: Aniketh Jain <anijainc@amazon.com>

devardee · 2022-09-01T03:42:06Z

Closing this PR in favor of #1088

devardee and others added 4 commits May 26, 2022 16:55

Preserve url hash for saml based authentication

04392e6

Merge branch 'opensearch-project:main' into main

67adc8e

Refactor + add support to run saml based integ tests via selenium web…

6c317b3

… driver

Add plugins.security.unsupported.restapi.allow_securityconfig_modific…

f8b69b1

…ation in developer guide

devardee requested a review from a team July 28, 2022 14:48

DarshitChanpura reviewed Jul 28, 2022

View reviewed changes

DarshitChanpura requested changes Jul 28, 2022

View reviewed changes

peternied requested changes Jul 28, 2022

View reviewed changes

peternied requested changes Aug 2, 2022

View reviewed changes

peternied mentioned this pull request Aug 4, 2022

Preserve URL Hash for SAML based login #1039

Merged

2 tasks

expani mentioned this pull request Aug 19, 2022

Fix for Tenancy info getting lost on re-login in SAML Authentication flow #1058

Merged

3 tasks

devardee and others added 5 commits August 30, 2022 14:02

Add one more test

9af841e

Signed-off-by: Deepak Devarakonda <devardee@amazon.com>

Added tests for checking tenancy retention after logout in SAML

c56f6d1

Signed-off-by: Aniketh Jain <anijainc@amazon.com>

Lint formatting fixes

3432bc4

Signed-off-by: Aniketh Jain <anijainc@amazon.com>

Removed unused imports

2e181cf

Signed-off-by: Aniketh Jain <anijainc@amazon.com>

Added preferred tenants and improved error logger

4f09fac

Signed-off-by: Aniketh Jain <anijainc@amazon.com>

This was referenced Aug 30, 2022

SAML Integration Tests #1087

Closed

SAML Integration Tests #1088

Merged

devardee closed this Sep 1, 2022

		//for now only run saml integration test.
		testMatch: ['/test/jest_integration//saml_auth.tests.ts'],


		});

		function sleepFor(sleepDuration: number) {

Conversation

devardee commented Jul 28, 2022

Description

Category

Check List

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peternied left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

devardee commented Aug 2, 2022

Uh oh!

peternied left a comment

Choose a reason for hiding this comment

Uh oh!

devardee commented Aug 3, 2022

Uh oh!

peternied commented Aug 9, 2022

Uh oh!

devardee commented Aug 10, 2022

Uh oh!

peternied commented Aug 11, 2022

Uh oh!

peternied commented Aug 22, 2022

Uh oh!

devardee commented Sep 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants