-
Notifications
You must be signed in to change notification settings - Fork 158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FEATURE]Automation Test for OIDC #1150
Comments
Hi @aoguan1990, is there any further update about this issue/request? |
[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. |
[Triage 2/6] Moving from "sprint backlog" to backlog. |
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):
|
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 See the original comment from the contributor of these tests: https://github.com/opensearch-project/security-dashboards-plugin/pull/1088/files#r970807852
|
@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. :) |
@jakubp-eliatra I believe that has to do with the entry in the When the OIDC backend vends back a JWT to OpenSearch, the security plugin expects it to contain both the configured For OpenID backend |
@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:
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?
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
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? :) |
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 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. |
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. |
Hi @RyanL1997, thanks for answering. So the problem you mention with Regarding the cookie assertion for tests @cwperks, when I log in to Dashboards using Keycloak, I only see a single cookie called |
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. |
@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. |
@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:
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. |
We just opened a PR! opensearch-project/opensearch-dashboards-functional-test#595 Few words of explanation:
Here's some points worth discussing:
|
@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 |
@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): 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 What should be our next steps now? Migrating the setup for OIDC and execution of the test to this cypress-test.yml file? |
@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. |
@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. |
Hi @jakubp-eliatra, thank you for this update! Yes, it seems like overriding should resolve the limitation of |
@cwperks You're right, I think that a separate PR for Cypress update is very much in order here, so I opened it: 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? :) |
@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. |
@davidlago Adrian from Eliatra here, I am working on this today. Can I have an assignment to the issue, please? |
Hi @adrian-golawski-eliatra , I think PR #1579 fully covered the content of this issue. We should close this one for now. |
Is your feature request related to a problem?
Automation test for OIDC authentication methodology
What solution would you like?
OIDC Test Scope includes:
The text was updated successfully, but these errors were encountered: