[FEATURE]Automation Test for OIDC

[aoguan1990]

Is your feature request related to a problem?
Automation test for OIDC authentication methodology

What solution would you like?
OIDC Test Scope includes:

1. Configuration and input validation Test Cases (Positive and Negative)
2. First-Time Login Test Cases (Positive and Negative)
3. Utilize cookie re-Login Test Case (Positive and Negative)
4. Logout Test Cases (Positive and Negative)
5. Authorization validation Test Case

[RyanL1997]

Hi @aoguan1990, is there any further update about this issue/request?

[stephen-crawford]

[Triage] @RyanL1997 could you update this issue with your findings on follow-up? Thank you.

Keeping in sprint backlog due to importance of testing. To be addressed.

[stephen-crawford]

[Triage 2/6] Moving from "sprint backlog" to backlog.

[jakubp-eliatra]

Hi there, I'm starting to work on this issue. :)

However I'm not sure about 2 things, based on my knowledge about SAML test which covers a similar login flow (https://github.com/opensearch-project/security-dashboards-plugin/blob/main/test/jest_integration/saml_auth.test.ts):

1. 'input validation' is mentioned within the test scope, but what does it mean exactly in terms of acceptance criteria? I'm not sure I see any input validation within the SAML test, so I think I would need more details in order to incorporate some input validation into the test. :)
2. when utilizing cookies (and checking if they're correct), why in the SAML test are they checked only for amount (equal to 2, line 248 in the SAML test file) instead of, for example, specific contents within the cookies? In order to design a cookie re-login test case, a cookie validation would be needed, so I'm wondering what would be the best practice for cookie validation as it doesn't seem clear to me based on the SAML test flow. :)

[cwperks]

Hey @jakubp-eliatra , the login form for OIDC will be in the external IdP and any input validation would be done in the app external to opensearch. Can you clarify what you mean by input validation in this context?

The assertions in the SAML test could be a lot richer: https://github.com/opensearch-project/security-dashboards-plugin/blob/main/test/jest_integration/saml_auth.test.ts#L246-L249

The JWT that is given to the client for SAML and OIDC auth is stored in the cookie and a richer assertion could be to look at the value of cookie.credentials?.authHeaderValue and verify that the JWT contains the expected subject_key and roles_key claims.

See the original comment from the contributor of these tests: https://github.com/opensearch-project/security-dashboards-plugin/pull/1088/files#r970807852

one cookie for set by the idp and one cookie set by the security dashboard plugin. So in total there should be two cookies

[jakubp-eliatra]

@cwperks thanks for your quick answers! Regarding the input validation, I meant point 1 of test scope in the issue description. Therefore, I wasn't sure what would be the assertions for checking those external IdP inputs (whether this means basically testing if wrong credentials won't work or something more i.e. more complex validation).

The cookie part of the question is much clearer now though, thanks. :)

[cwperks]

@jakubp-eliatra I believe that has to do with the entry in the authc: section when setting up an oidc backend in the security plugin's config.yml file. For the SAML tests that would be this section: https://github.com/opensearch-project/security-dashboards-plugin/blob/main/test/jest_integration/saml_auth.test.ts#L135-L153

When the OIDC backend vends back a JWT to OpenSearch, the security plugin expects it to contain both the configured subject_key and roles_key claims within the token. If the token does not contain those claims then it is invalid and cannot be used.

For OpenID backend openid_connect_url is required for configuration. subject_key is optional and has a default of preferred_username. roles_key is also optional, but is necessary if you want to get backend roles from the JWT. This section on the documentation website has a list of config values: https://opensearch.org/docs/latest/security/authentication-backends/openid-connect/#configure-openid-connect-integration

[jakubp-eliatra]

@cwperks that's a very helpful answer, thanks! :)

I have few other, more setup-oriented questions about the actual setup for writing the test if we'd like to proceed with Cypress:

1. First and foremost, to utilize an external IdP we would need a cross-origin support within Cypress and here we use version 9.5.4: https://github.com/opensearch-project/opensearch-dashboards-functional-test/blob/main/package.json; however the experimental support for cross-origin tests was introduced in 9.6.0 while an official support (meaning most likely a stable one) was introduced in version 12: https://www.cypress.io/blog/2022/12/06/announcing-cypress-12/

We use Node v14.20.1 and Cypress 12 relies on Node v16+. What should be our approach here? Using the experimental support and upgrading probably just Cypress version or use the stable support and upgrade Node as well?

2. Here: https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/TESTING.md is a mention that 'Functional testing in OpenSearch Dashboards is migrating to the opensearch-dashboards-functional-test repository. All new functional tests should be written there.'.

Is this the appropriate location for OIDC test then? https://github.com/opensearch-project/opensearch-dashboards-functional-test/tree/7cdf3e63c4d3df8e6ee19b164707cc325dc30b93/cypress/integration/plugins/security-dashboards-plugin

I have noticed that some Selenium tests are placed within /test/functional directory while the SAML Selenium test is placed in plugins/security-dashboards-plugin/test/jest_integration. Am I correct to assume both of these locations (/test/ folder and plugins/security-dashboards-plugin/test/) as deprecated for the Cypress test files now?

3. The same readme (https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/TESTING.md) says: OpenSearch Dashboards uses Jest for unit and integration tests, Selenium for functional tests, and Cypress for backwards compatibility tests. This seems to suggest that Selenium should be used for OIDC test as Cypress is meant to be used rather for backwards compatibility test.

Since we would like to use Cypress for OIDC test (if we get past the same-origin issue with IdP testing), then we can assume this part of readme is not up to date, correct? :)

[RyanL1997]

Is this the appropriate location for OIDC test then? https://github.com/opensearch-project/opensearch-dashboards-functional-test/tree/7cdf3e63c4d3df8e6ee19b164707cc325dc30b93/cypress/integration/plugins/security-dashboards-plugin

Hi @jakubp-eliatra , thank you for the follow up on this issue. Yes ideally all the functional test should be in the functional-test repo. However, one of the challenges of that environment is that they have re-written the cy.visit() and cy.request() to support the basic auth only (https://github.com/opensearch-project/opensearch-dashboards-functional-test/blob/7cdf3e63c4d3df8e6ee19b164707cc325dc30b93/cypress/utils/commands.js#L38-L85).

In addition, as you mentioned the cypress version is locked to 9.5.4 in the functional test repo.

According to all these, in my opinion, I'd prefer to locate the OIDC test in the security dashboard repo, so that we can run it in build time.

[cwperks]

Hey @jakubp-eliatra,

The SAML integration tests were added last year to this repo due to all of the limitations you raised. You may find some useful context on the PR that introduced those tests: #1088 and the PR that raised the initial issue of lack of integration tests for SAML: #1039

Because of the limitations of the current version of cypress in the function tests repo, I recommend to use selenium and add the tests to this repo. One advantage of having the tests in this repo is that they will run at build time.

I am not sure of the timeline for when OSD would upgrade to node 16 or afterwards when the functional tests repo can be upgraded to cypress 12. We need some assurances that changes do not break OIDC flow now and should not wait for a node upgrade or cypress upgrade in other repos.

[jakubp-eliatra]

Hi @RyanL1997, thanks for answering. So the problem you mention with cy.visit() and cy.request() is now resolved due to the fact that we are overriding it, correct?

Regarding the cookie assertion for tests @cwperks, when I log in to Dashboards using Keycloak, I only see a single cookie called security_authentication and it's in this format: https://github.com/tkeetch/iron-filings#token-format. Am I doing something wrong here? :) It seems encoded, so we are not aware how we can test it as JWT token with subject_key and roles_key. For now I settled for just testing if a cookie called security_authentication exists, as it's all I can do now.

[nibix]

Adding to the question from @jakubp-eliatra : As we are talking about functional tests, I would recommend a black box test approach, i.e. just test the functionality and not the technical details how this is achieved. White box testing covering technical details should be done on the unit test level.

[cwperks]

@nibix What if we wanted to write a test with backend roles? We could verify the backend roles by examining the cookie (See details for how to do so here: #1376) or we could click around the UI in the test to see if the user has access to expected functionality. i.e. An admin should see Security in the main menu, but a regular user should not.

[nibix]

@cwperks This is of course also possible in a black box test, i.e., by checking for the actual presence of the menu item in the main menu.

We were rather referring to doing assertions in the content of the cookie, as described here:

The JWT that is given to the client for SAML and OIDC auth is stored in the cookie and a richer assertion could be to look at the value of cookie.credentials?.authHeaderValue and verify that the JWT contains the expected subject_key and roles_key claims.

This would be then white box testing. We would recommend to avoid that in functional tests, which should strictly test from a functional/user pov. Additionally, there is a technical hurdle: by default, the cookie is encrypted. Thus, it is not straight forward to access the JWT structure.

[jakubp-eliatra]

We just opened a PR! opensearch-project/opensearch-dashboards-functional-test#595

Few words of explanation:

1. this is only a draft that serves more as a PoC/a proposition as of right now; :)
2. what we did there is we ported the SAML test assertions (logging in to dashboard's overview, logging in to console and logging in with hash in the URL) for OIDC, but for Cypress;
3. we did the PR in Cypress (although there is a Selenium alternative in development in parallel), because we still think it's a good idea and wanted to hear your thoughts on this. :)

Here's some points worth discussing:

1. @cwperks even though OSD hasn't upgraded Node from v14 yet, there is a crucial change in 2.6 - node is no longer listed as exactly 14.20.1 version, but as ^14.20.1. So we used Node v18.15.1 on our CI here: https://github.com/opensearch-project/opensearch-dashboards-functional-test/blob/acb8e7305970953eb4a90667470c2d5a5fb6c3a6/.github/workflows/oidc-e2e-test.yml to set everything up with success.
2. Since the Node problem became a no-issue (or so it seems), I also allowed myself to bump the Cypress version a tiny bit (from 9.5.4 to 9.6.1, where the experimental cross-origin testing was introduced) and it works great. We were able to navigate between IdP and OSD in the test back and forth in Cypress without issues. :) Perhaps we can wait for @RyanL1997 answer here, because if we can override problematic Cypress functions also in 9.6.1, then this problem would also be solved.
3. The CI setup mentioned above means a complete installation of OpenSearch, OpenSearch Dashboards and Keycloak along with the server configuration for OIDC, all that was achieved using GitHub actions. This we consider a huge win, since it means a separation and independence between server configuration and test logic. This also enables as to utilize/tweak this setup for other testing cases or other testing libraries with little to no effort.
4. On a side note, we also utilized https://www.npmjs.com/package/cypress-mochawesome-reporter for clear test reports for our development needs as it worked out of the box for us (don't mind that, we know that there is mocha-junit-reporter used in this project, but we weren't able to get it running so easily on our fork :)).
5. The test assertions mimic assertions of SAML test (user gets logged in to various places of the app and after that we check for a cookie to confirm successful login). We can increment from there and add any assertions you'd like (that are mentioned above, for example re-login cases or input validation). Also worth noting is that locally the test passes everytime (analogical assertions for SAML test were flaky) on my machine and it does so much faster than SAML test (the long wait periods i.e. 30 seconds were not necessary).

[cwperks]

@jakubp-eliatra That's awesome to see it working with cypress! Thank you for the write-up and explanation. Long-term all functional tests should be written with cypress, so once that repo is upgraded to a version of cypress with official support for cross-origin tests then I'm in favor of moving the functional tests with selenium in this repo over to the functional tests repo.

If this change gets merged into the functional test repo, then we should follow it up by expanding on the functional tests at build time here: https://github.com/opensearch-project/security-dashboards-plugin/blob/main/.github/workflows/cypress-test.yml

[jakubp-eliatra]

@cwperks Thanks a lot for the positive feedback. :)

FYI I just bumped Cypress to v12.8.1 and everything still works on my end (my local Node is 14.20.1):
https://github.com/sebastianmichalski/opensearch-dashboards-functional-test/pull/1/files

There were some updates in Cypress naming conventions that we should be aware of, but the process of renaming everything was automated and smooth. There's also a great speed upgrade in the test's speed, the test passes in 5 seconds in even 3 seconds instead of ~20 seconds on my local machine.

What should be our next steps now? Migrating the setup for OIDC and execution of the test to this cypress-test.yml file?

[cwperks]

@tianleh is there anything preventing the upgrade of cypress in the opensearch-dashboards-functional-test repo? I see an open issue for an upgrade, but there are no comments on the issue. There's supported functionality in Cypress >= 12 for cross-origin testing which is required for this repo to test with external IdPs. @jakubp-eliatra opened up a PR to introduce functional tests for OIDC (opensearch-project/opensearch-dashboards-functional-test#595) that we would like to review/merge, but it requires upgrading cypress.

@jakubp-eliatra Do you think it makes sense to open up a narrower PR against the functional tests repo to upgrade the version of cypress?

The other thing I would ask is to update the PR description with the list of tests added and a little description of each. I will review the PR later today and leave comments, but I'm not a maintainer of the functional tests repo so the context will help the maintainers of the repo.

[cwperks]

@jakubp-eliatra Assuming there is nothing blocking the upgrade of cypress then next steps are to port the SAML tests from this repo -> functional test repo and remove the selenium tests and associated dependencies from this repo.

The node-based SAML IdP that is used has not been updated in a long time. I would be interested to see if there are alternatives to be used than the node-based SAML IdP.

[RyanL1997]

Hi @jakubp-eliatra, thank you for this update! Yes, it seems like overriding should resolve the limitation of visit() and request().

[jakubp-eliatra]

@cwperks You're right, I think that a separate PR for Cypress update is very much in order here, so I opened it:
opensearch-project/opensearch-dashboards-functional-test#598

Is it correct that I set base as 'main' branch there? Because our fork of the repo for OIDC test is based on 2.x, should we base on 'main' branch for OIDC test PR? :)

[cwperks]

@jakubp-eliatra In most cases you should make your change against main and use the backport process (through labels, ask maintainers to add them) to backport the change to the 2.x branch to get included in the next release on the 2.x line. The code in main is code for the next unreleased major version (as of writing that's 3.0.0) and code for other versions is in the respective branch for that version. 2.x contains unreleased code for the next release on 2.x which will be 2.7.

The only time you would target a specific branch is if the change is only required for that branch. Backports are created automatically by a bot and sometimes fails if there are differences between the branches that cannot automatically be resolved which requires manual backports to be created. There are definitely instances where you may want to target a branch, but it would be the exception to the norm. If you are developing new functionality, it will go into main first and be backported to other branches accordingly.

[adrian-golawski-eliatra]

@davidlago Adrian from Eliatra here, I am working on this today. Can I have an assignment to the issue, please?

[RyanL1997]

Hi @adrian-golawski-eliatra , I think PR #1579 fully covered the content of this issue. We should close this one for now.