Skip to content
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

test: OIDC login workflow #19

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

SohamRatnaparkhi
Copy link
Member

IMPORTANT: Please create an issue before filing a pull request! Changes need to be discussed before proceeding. Please read the contribution guidelines.

Details
Currently, there are no tests to check if some code breaks OIDC login workflow for Krini.
Refer to the corresponding issue with #1 for more information.

Testing

Appropriate tests have been written. Code passes all the test.

spec.cy.js.mp4

image

Documentation
No documentation required as it is not a feature upgrade.

Style

My code strongly adheres to the style convention of the project.

Closing issues
Closes #1

@vercel
Copy link

vercel bot commented Mar 4, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
krini ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 6, 2023 at 7:00PM (UTC)

@anuragxxd
Copy link
Member

Hey @SohamRatnaparkhi, Can you remove commit docs: making docs verbose from this pr as it is not related to the issue this pr is solving!

@SohamRatnaparkhi
Copy link
Member Author

SohamRatnaparkhi commented Mar 4, 2023

Hey @SohamRatnaparkhi, Can you remove commit docs: making docs verbose from this pr as it is not related to the issue this pr is solving!

Yep, I tried removing that commit but I couldn't do so. Hence, I removed the changes in README file in the last commit (test: oicd auth tests added and README changes removed ) . I too wanted to remove that commit. Is there a way to do so. If not, then anyways merging this PR won't affect README.

Thank you!

@anuragxxd
Copy link
Member

Hey @SohamRatnaparkhi, you can maybe rebase the commit from the git history & then try to force push to the branch you are working on. You can see this for your ref: https://stackoverflow.com/questions/30893040/remove-commit-from-history

@SohamRatnaparkhi
Copy link
Member Author

SohamRatnaparkhi commented Mar 4, 2023

Hey @SohamRatnaparkhi, you can maybe rebase the commit from the git history & then try to force push to the branch you are working on. You can see this for your ref: https://stackoverflow.com/questions/30893040/remove-commit-from-history

Hey @git-anurag-hub ,
Thank you for pointing out to the stack overflow article. I have read the article and executed the following commands and have dropped the unnecessary commit made to README.

git rebase -i 77b6650~
git push origin test_OICD_auth

Now the README changes commit doesn't exist. Kindly inform me if I have erred, so that I may rectify my mistakes and make another change.
Thank you!

@anuragxxd
Copy link
Member

Hey @SohamRatnaparkhi, thanks for the change! Yeah now the commit is gone!

Can you resolve the conflicts & also configure the .env.test file with the sample username & password (you can obviously remove it while making a pr & change it to some dummy env). Also I can see that the test is failing & is not making the user login process complete, can you check that too.

Please ping in case of any help. Thanks for the pr again :)

@SohamRatnaparkhi
Copy link
Member Author

SohamRatnaparkhi commented Mar 6, 2023

Hey @SohamRatnaparkhi, thanks for the change! Yeah now the commit is gone!

Can you resolve the conflicts & also configure the .env.test file with the sample username & password (you can obviously remove it while making a pr & change it to some dummy env). Also I can see that the test is failing & is not making the user login process complete, can you check that too.

Please ping in case of any help. Thanks for the pr again :)

Hi,
I was having some problems related to conflicts on my local repo, but I was able to solve them. I will solve the problems you mentioned and will make another commit.
Cypress provides a feature of cypress env as mention in this link: Setting environment variables. Should we use the second method in the docs (above link) in our tests?
Thank you!

@SohamRatnaparkhi
Copy link
Member Author

SohamRatnaparkhi commented Mar 7, 2023

Hi, I noticed an issue related to failing tests that I recently encountered. Upon investigation, it seems that Cypress may be encountering difficulty in retrieving environment variables from the env file. As a result, when the host_uri in the .\config.js file is set to process.env.REACT_APP_HOST_URI, the tests fail to execute properly. In my specific case, the cy.visit('/') command does not direct the browser to localhost:3000 as intended, but instead points to http://localhost:46761/.

Upon manually setting host_uri = localhost:3000 or manually setting host_uri to production link, the tests were able to execute successfully. I have also provided a video demonstration of this issue, in which I have used my personal LS password for testing purposes.
Demo video for test on development server


Screencast.from.03-07-2023.01.10.06.PM.webm

@uniqueg
Copy link
Member

uniqueg commented Mar 7, 2023

Hi, I noticed an issue related to failing tests that I recently encountered. Upon investigation, it seems that Cypress may be encountering difficulty in retrieving environment variables from the env file. As a result, when the host_uri in the .\config.js file is set to process.env.REACT_APP_HOST_URI, the tests fail to execute properly. In my specific case, the cy.visit('/') command does not direct the browser to localhost:3000 as intended, but instead points to http://localhost:46761/.

Upon manually setting host_uri = localhost:3000 or manually setting host_uri to production link, the tests were able to execute successfully. I have also provided a video demonstration of this issue, in which I have used my personal LS password for testing purposes. Demo video for test on development server

Solid work, @SohamRatnaparkhi! :) Any chance you can also fix the failing test on the CI machines?
If you need us to set a variable or something, please let us know.

Or wait... is the "only" problem now that we don't have a service account that we can use to provide a username and password?

@uniqueg uniqueg closed this Mar 7, 2023
@uniqueg uniqueg reopened this Mar 7, 2023
@uniqueg
Copy link
Member

uniqueg commented Mar 7, 2023

Oops, seems like I had accidentally close the PR, my bad. Opened it again.

Regarding the "test user", I see some problems coming up: I can imagine that LS AAI will not like to have a service account for that purpose. But I'll ask. Otherwise I could use mine. Either way, you'd need to be able to consume GitHub repo secrets in your test. Obviously we can't just commit a valid user/pass combination to the repo, even if it were an unprivileged service account.

@SohamRatnaparkhi
Copy link
Member Author

Oops, seems like I had accidentally close the PR, my bad. Opened it again.

Regarding the "test user", I see some problems coming up: I can imagine that LS AAI will not like to have a service account for that purpose. But I'll ask. Otherwise I could use mine. Either way, you'd need to be able to consume GitHub repo secrets in your test. Obviously we can't just commit a valid user/pass combination to the repo, even if it were an unprivileged service account.

Yes @uniqueg , I will need to pull the dummy/service account password and username from the GitHub secrets.
We can add a step in GitHub actions workflow in checks.yml link which will pull the values of GitHub secrets and store them as an env. Then, we can use those env values while authentication during the test. We can probably do something similar to steps mentioned in this article.

@anuragxxd
Copy link
Member

Hi, I noticed an issue related to failing tests that I recently encountered. Upon investigation, it seems that Cypress may be encountering difficulty in retrieving environment variables from the env file. As a result, when the host_uri in the .\config.js file is set to process.env.REACT_APP_HOST_URI, the tests fail to execute properly. In my specific case, the cy.visit('/') command does not direct the browser to localhost:3000 as intended, but instead points to http://localhost:46761/.

Upon manually setting host_uri = localhost:3000 or manually setting host_uri to production link, the tests were able to execute successfully. I have also provided a video demonstration of this issue, in which I have used my personal LS password for testing purposes. Demo video for test on development server

Hey @SohamRatnaparkhi, can you verify if without the test the application is working fine for you. Like all the login & other stuff is picking up the host_uri from the env variable!

@SohamRatnaparkhi
Copy link
Member Author

Hi, I noticed an issue related to failing tests that I recently encountered. Upon investigation, it seems that Cypress may be encountering difficulty in retrieving environment variables from the env file. As a result, when the host_uri in the .\config.js file is set to process.env.REACT_APP_HOST_URI, the tests fail to execute properly. In my specific case, the cy.visit('/') command does not direct the browser to localhost:3000 as intended, but instead points to http://localhost:46761.
Upon manually setting host_uri = localhost:3000 or manually setting host_uri to production link, the tests were able to execute successfully. I have also provided a video demonstration of this issue, in which I have used my personal LS password for testing purposes. Demo video for test on development server

Hey @SohamRatnaparkhi, can you verify if without the test the application is working fine for you. Like all the login & other stuff is picking up the host_uri from the env variable!

Hey @git-anurag-hub , I did some debugging and I can ensure you that the host_uri is being pulled correctly from env.development file in the the login component. But, I am getting a strange new error and this is the first time I am getting that error You can see the debugging result as well as the error in the first video.

Error invalid_grant
There was an error processing your request.

Invalid redirect: http://localhost:3000 does not match one of the registered values.
Screencast.from.03-08-2023.04.50.28.PM.webm

Another point that I noticed while debugging is -

  1. Cypress is not able to pull the variables from the env file
  2. The invalid redirect error is only occurring on local host as when I use host_uri as the production deployment, I don't get that error.

Video regarding the above conclusions with respect to Cypress. I have console logged the host uri that cypress recieves and you can see the results on right side of screen( I am highlighting them)
Important time stamps - 0:13, 0:31, 0:50:

Screencast for debugging Cypress
The strange thing is that, it is today only that I am getting the invalid redirect error on local host.

@uniqueg
Copy link
Member

uniqueg commented Mar 8, 2023

@SohamRatnaparkhi: Cool. Ping me if you are done debugging with @git-anurag-hub and you figured out how to pull the GitHub secrets for consumption in your tests (maybe LS_AAI_USER and LS_AAI_PASS). I will then set the secrets.

@anuragxxd
Copy link
Member

Hey @SohamRatnaparkhi, I just pulled your branch & everything seem to work fine on my end for the LS AAI login & I was able to login successfully! Can you stash your changes on this branch & check the functionality with having local HEAD of your brach synced with this remote branch!

@SohamRatnaparkhi
Copy link
Member Author

SohamRatnaparkhi commented Mar 9, 2023

Hey @SohamRatnaparkhi, I just pulled your branch & everything seem to work fine on my end for the LS AAI login & I was able to login successfully! Can you stash your changes on this branch & check the functionality with having local HEAD of your brach synced with this remote branch!

It is strange but without even stashing the changes, it worked on my end. I cleared the browser cache and then everything worked well.

Now, how should we proceed regarding the test user part? Should we go by this article or some other method?

@SohamRatnaparkhi
Copy link
Member Author

Hi @git-anurag-hub and @uniqueg ,
I have found a solution to the test-user issue. Probably, REACT_APP env variables don't directly work with cypress latest version. But we can export env variables from command line/workflow with prefix CYPRESS_ (ex CYPRESS_HOST_URI)

This feature can be found here: Cypress environment variables

We can use this as follows:

  • In development:

    • We will ask contributors/users to run following commands while setting up repo locally especially before making a commit.
    •  export CYPRESS_HOST_URI=http://localhost:3000
       export CYPRESS_LS_AAI_USER=<their LS_AAI_USERNAME>
       export CYPRESS_LS_AAI_PASSWORD=<their LS_AAI_PASSWORD>
      
  • In production/Github CI workflows:

    • We will have to setup env key in Github workflow as mentioned in their docs (link)
    • The Gihtub secrets will have 3 variables - CYPRESS_LS_AAI_USER, CYPRESS_LS_AAI_PASSWORD, CYPRESS_HOST_URI set by @uniqueg , and then we can add them to env as follows:
    •   ....
        env:
        		CYPRESS_HOST_URI: ${{secrets.CYPRESS_HOST_URI}}
        		CYPRESS_LS_AAI_USER: ${{secrets.CYPRESS_LS_AAI_USER}}
        		CYPRESS_LS_AAI_PASSWORD: ${{secrets.CYPRESS_LS_AAI_PASSWORD}}
        jobs:		
        ...			

Now, in the test file: these variables can be accesses by Cypress.env("KEY") ( Ex: Cypress.env("HOST_URI") )

I have tested this locally, and it works well there.

@uniqueg
Copy link
Member

uniqueg commented Mar 11, 2023

Cool, sounds good :)

Just FYI, ELIXIR/LS AAI are working on support for such test cases. Please check out the #aai channel on our Slack board for details (should be the last couple of messages in that channel).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement the Cypress e2e test to test the login flow of krini
3 participants