[Backport 1.3] Preserve URL Hash for SAML based login with Tests#1220

Closed

stephen-crawford wants to merge 13 commits intoopensearch-project:1.3from

stephen-crawford:SAMLBackport

Contributor

stephen-crawford commented Nov 16, 2022

Signed-off-by: Stephen Crawford steecraw@amazon.com

Description

Manually backports SAML tests and updates from recent commits into 1.3.

Why these changes are required?

Resolves requests in #1219 discussion.

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.


          Manual backport and transfer of tests from recent commits

37f9a4e

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

stephen-crawford requested a review from a team

November 16, 2022 23:03

stephen-crawford mentioned this pull request

[Backport 1.3] Preserve URL Hash for SAML based login #1219

Closed

peternied changed the title ~~Manual backport and transfer of tests from recent commits~~ [Backport 1.3] Preserve URL Hash for SAML based login + Tests

peternied changed the title ~~[Backport 1.3] Preserve URL Hash for SAML based login + Tests~~ [Backport 1.3] Preserve URL Hash for SAML based login with Tests

Member

peternied commented Nov 17, 2022

@scrawfor99 Could you look into the build/integration test failures?

stephen-crawford added 3 commits

November 17, 2022 09:17


          Reset changes in package.json, integration-test manually

187f2f0

Signed-off-by: Stephen Crawford <steecraw@amazon.com>


          Remove yarn from bootstrap step

a2dbf4a

Signed-off-by: Stephen Crawford <steecraw@amazon.com>


          Try newer dash

21f3073

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

Contributor Author

stephen-crawford commented Nov 17, 2022 •

edited

Loading

Update on the test failures: So looking at the errors, it seems that there is a versioning issue with the Cyprus tests that are called since the changes from the tests come from 2.4. I am looking through the different dependencies to try to find what needs to be switched. The error you get is


[2/4] Fetching packages...
error samlp@7.0.1: The engine "node" is incompatible with this module. Expected version ">=12". Got "10.24.1"
error Found incompatible module.
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
ERROR [bootstrap] failed:
ERROR Error: Command failed with exit code 1: /opt/hostedtoolcache/node/10.24.1/x64/lib/node_modules/yarn/bin/yarn.js install --non-interactive
         at makeError (/home/runner/work/security-dashboards-plugin/security-dashboards-plugin/OpenSearch-Dashboards/packages/osd-pm/dist/index.js:25139:11)
         at handlePromise (/home/runner/work/security-dashboards-plugin/security-dashboards-plugin/OpenSearch-Dashboards/packages/osd-pm/dist/index.js:24074:26)
         at process._tickCallback (internal/process/next_tick.js:68:7)
error Command failed with exit code 1.

During the run yarn step.

stephen-crawford added 3 commits

November 17, 2022 10:12


          Bump react hook, typescript versions

f53c76c

Signed-off-by: Stephen Crawford <steecraw@amazon.com>


          Fix comma

d7fc153

Signed-off-by: Stephen Crawford <steecraw@amazon.com>


          Add Self Signed

ea610a6

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

Member

peternied commented Nov 17, 2022 •

edited

Loading

@scrawfor99 One of the major changes in between major versions 1 and 2 was upgrading the version of node [1], Can you try an earlier version of samlp [2] that is compatible with node 10?

[1] opensearch-project/OpenSearch-Dashboards#1028
[2] https://www.npmjs.com/package/samlp/v/6.0.2?activeTab=versions

Contributor Author

stephen-crawford commented Nov 17, 2022 •

edited

Loading

Right now there is not a direct dependency on samlp. It is only on the saml-idp library https://github.com/mcguinness/saml-idp/blob/master/package.json#L38 which is dependent on a forked version of samlp: https://github.com/mcguinness/node-samlp/commits/master. I think we could maybe fork the saml-idp library and then swap the dependencies of it to the normal samlp and move it back (it is referencing an almost identical fork of samlp) but want to check and see what you think first. @peternied

stephen-crawford force-pushed the SAMLBackport branch from 7eda5ef to d50017e Compare

November 17, 2022 17:06

stephen-crawford added 2 commits

November 17, 2022 12:06


          manually add samlp dependency

d50017e

Signed-off-by: Stephen Crawford <steecraw@amazon.com>


          Yarn ignore engines

c7bebaa

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

peternied reviewed

View reviewed changes

.github/workflows/integration-test.yml Outdated

                         npm uninstall -g yarn
                         echo "Installing yarn ${{ steps.versions_step.outputs.yarn_version }}"
-                        npm i -g yarn@${{ steps.versions.outputs.yarn_version }}
+                        npm i -g yarn@${{ steps.versions.outputs.yarn_version }} --ignore-engines

Member

peternied Nov 17, 2022

Great idea! 🤞

stephen-crawford added 2 commits

November 17, 2022 12:29


          Add ignore engines to both test types

71ae84a

Signed-off-by: Stephen Crawford <steecraw@amazon.com>


          Remove Lint:Sass use

606269c

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

Contributor Author

stephen-crawford commented Nov 22, 2022

Update: I am looking into this further. For whatever reason, the yarn command to skip the engine check is not working. I suspect that it is because there is a later check within OSD Bootstrap. There is also a problem in the unit tests where the navigation button test does not operate properly. I am not sure if we intend to backport the button from its introduction in 2.n (.4 I think?) but if so this will require a few more changes to get it introduced.

stephen-crawford added 2 commits

November 22, 2022 10:49


          Reset to match main branches

7001f90

Signed-off-by: Stephen Crawford <steecraw@amazon.com>


          Add license header

952ff59

Signed-off-by: Stephen Crawford <steecraw@amazon.com>

DarshitChanpura reviewed

View reviewed changes

Member

DarshitChanpura left a comment

Left some review comments.

Also helpful to add .idea to .gitignore file so those changes are not pushed.

security-dashboards-plugin/.gitignore

Line 12 in 3abb707

.idea/

.github/workflows/integration-test.yml

                   steps:
                     - name: Download OpenSearch Core
                       run: |
-                        wget https://ci.opensearch.org/ci/dbc/distribution-build-opensearch/1.3.6/latest/linux/x64/tar/builds/opensearch/dist/opensearch-min-1.3.6-linux-x64.tar.gz

Member

DarshitChanpura Nov 22, 2022

I believe these 2.4.0 version updates shouldn't be part of this PR

Contributor Author

stephen-crawford Nov 22, 2022

Yeah, I am just trying to get anything to work at this point. So I just copied the entire file to make sure that it was not a missing line somewhere that was causing the issue. I will be going back through to pair down the changes to those necessary once I get anything working.

package.json

    
                  "lint:es": "node ../../scripts/eslint",

                  "lint:sass": "node ../../scripts/sasslint",

                  "lint": "yarn run lint:es && yarn run lint:sass",

                  "lint": "yarn run lint:es && yarn run lint:style",

Member

DarshitChanpura Nov 22, 2022

Why remove the call to sasslint here?

public/apps/account/test/account-nav-button.test.tsx

-                      username={userName}
-                      tenant="tenant1"
-                      config={config as any}
+              exports[`Account navigation button renders 1`] = `

Member

DarshitChanpura Nov 22, 2022

I'm not sure if you added this change. But this test file has been changed to a snapshot file. Do you know why is that?

.github/workflows/unit-test.yml

                         path: OpenSearch-Dashboards
                         repository: opensearch-project/OpenSearch-Dashboards
-                        ref: '1.x'
+                        ref: 'main'

Member

DarshitChanpura Nov 22, 2022

why point to main here?

Contributor Author

stephen-crawford Nov 22, 2022

See above about just copy+pasting the files with changes to try to get a working version of any sort. I am going to be fixing these once I get the yarn issue fixed.

cwperks mentioned this pull request

[Backport 1.3] Preserve URL Hash for SAML based login #1226

Merged

3 tasks

Contributor Author

stephen-crawford commented Nov 23, 2022

Closing in favor of #1226

stephen-crawford closed this

stephen-crawford deleted the SAMLBackport branch

December 12, 2022 15:19

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet