Skip to content
This repository has been archived by the owner on Aug 7, 2024. It is now read-only.

Test to mock GitHub oauth with Next-Auth #6164

Merged
merged 13 commits into from
Apr 26, 2023
Merged

Test to mock GitHub oauth with Next-Auth #6164

merged 13 commits into from
Apr 26, 2023

Conversation

eddiejaoude
Copy link
Member

@eddiejaoude eddiejaoude commented Apr 18, 2023

Fixes Issue

Building on from #6151

fixes #5788

  • save mock user to database
  • successfully log in with mock user

Changes proposed

Check List (Check all the applicable boxes)

  • My code follows the code style of this project.
  • My change requires changes to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.
  • This PR does not contain plagiarized content.
  • The title of my pull request is a short description of the requested changes.

Screenshots

Note to reviewers

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's great having you contribute to this project

Welcome to the community 🤓

If you would like to continue contributing to open source and would like to do it with an awesome inclusive community, you should join our Discord chat and our GitHub Organisation - we help and encourage each other to contribute to open source little and often 🤓 . Any questions let us know.

@github-actions github-actions bot added large Pull request with more than 30 changed lines tests labels Apr 18, 2023
@amandamartin-dev
Copy link
Contributor

amandamartin-dev commented Apr 19, 2023

I have a few theories I'm exploring as to why this won't work "as is" from the blog tutorial version. Feel free to follow my rabbit hole or consider others:

1.We may have to set up the mock server for this version to work even though we have no need to mock the API calls. I'm thinking this because the mock server is what intercepts and modifies the information allowing login to github. I got this idea re-reading this comment in the article where it talks about the data provided for the GH secrets and such won't matter since it will be intercepted. We are currently getting errors that it's invalid JWT becuase we are NOT intercepting it "// 3. Here we are binding our user with a "Github fake account", this is needed since we are using OAuth, we don't have to worry about this data since we are gonna intercept and mock the direct Github API calls"

2.Theory 2 -this won't work bc similarly to what Dan noticed, we need to provide more information, however, I'm not sure how to build a fake yet valid JWT. Another potential area to explore... (see [nextauth].js for the auth options that led me to this train of thought)
Still poking around but want to share my thoughts in case it shakes anything out of your head!

@eddiejaoude
Copy link
Member Author

Thanks Amanda! Another option/idea for plan C, is we enable email/password login during testing instead of GitHub OAuth, and we perform a real login but only to out DB (we would have to inject the user into the DB, and maybe have a password field that isn't used during production).

@SaraJaoude SaraJaoude added the issue linked Pull Request has issue linked label Apr 19, 2023
@amandamartin-dev
Copy link
Contributor

adding note to this issue from Discord suggestion to review https://next-auth.js.org/configuration/callbacks

@dan-mba
Copy link
Member

dan-mba commented Apr 24, 2023

I did some debug work.

After modifying the tests & removing storageState which isn't really relevant for our use case, I found that next-auth doesn't like the sessionToken as it isn't a valid JWT.

There is a library to create valid JWTs
I will look into if using this will create a JWT that next-auth likes

@amandamartin-dev
Copy link
Contributor

I did some debug work.

After modifying the tests & removing storageState which isn't really relevant for our use case, I found that next-auth doesn't like the sessionToken as it isn't a valid JWT.

There is a library to create valid JWTs I will look into if using this will create a JWT that next-auth likes

Amazing! I was thinking the same thing after you noticed the JWT issue but hadn't looked into mocking that yet. I'm about to head into some meetings so won't have time to look at this again until tomorrow likely so feel free to use this branch if you want. I haven't made any changes yet today

@dan-mba
Copy link
Member

dan-mba commented Apr 24, 2023

I got the session Token working 🥳

Now the problem is the statistics page doesn't have all the info it needs in getServerSideProps.
Hopefully this is an easier problem to fix.

@dan-mba
Copy link
Member

dan-mba commented Apr 24, 2023

The automated test fails because we nee NEXTAUTH_SECRET to be set in order for the test to run.

Not exactly sure how to do this.
Do we need to add a default setup to .env.example?

@amandamartin-dev
Copy link
Contributor

I got the session Token working 🥳

Now the problem is the statistics page doesn't have all the info it needs in getServerSideProps. Hopefully this is an easier problem to fix.

Dan!!!! This is incredible news. I can't wait to take a look at the code. And the stats page makes sense and should be easy to fix. I'm pretty sure we didn't make a full profile, just login so there's no account. But login only actually covers the scope of this issue so we might be able to qa this and then split that out. Wdyt @eddiejaoude ?

@amandamartin-dev
Copy link
Contributor

The automated test fails because we nee NEXTAUTH_SECRET to be set in order for the test to run.

Not exactly sure how to do this. Do we need to add a default setup to .env.example?

I think you are right and from reading the docs, they may require it in the future anyway so we would be ahead of any future changes for prod and dev.

"Defining an explicit secret will make this problem go away. We will likely make this option mandatory, even in development, in the future."

Here's the docs for quick referencce: https://next-auth.js.org/configuration/options#secret

@dan-mba
Copy link
Member

dan-mba commented Apr 25, 2023

Automated test works!
🥳🥳🥳

Copy link
Contributor

@amandamartin-dev amandamartin-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few suggestions, also another question I do not know the answer to. Is it fine to leave this test user sitting in the database? Will it be accidentally queried in any other code we have that we know of? We could destroy after tests. but then we are creating each time, which perhaps we dont' want to do. Thoughts?

.env.example Show resolved Hide resolved
tests/setup/auth.js Outdated Show resolved Hide resolved
tests/setup/auth.js Outdated Show resolved Hide resolved
dan-mba and others added 2 commits April 25, 2023 08:23
Co-authored-by: Amanda <97615019+amandamartin-dev@users.noreply.github.com>
Co-authored-by: Amanda <97615019+amandamartin-dev@users.noreply.github.com>
@dan-mba
Copy link
Member

dan-mba commented Apr 25, 2023

Left a few suggestions, also another question I do not know the answer to. Is it fine to leave this test user sitting in the database? Will it be accidentally queried in any other code we have that we know of? We could destroy after tests. but then we are creating each time, which perhaps we don't want to do. Thoughts?

All tests are run on an empty database, not the production database.
All changes are discarded after the action runs.

@amandamartin-dev
Copy link
Contributor

All tests are run on an empty database, not the production database. All changes are discarded after the action runs.

Oh! Thank you for that. It makes sense but just wasn't sure. So I guess the only thing left is to officially open this to review? And then open a subsequent issue to write a test for the stats page (that addresses the data issues you noticed?)

@dan-mba
Copy link
Member

dan-mba commented Apr 25, 2023

Oh! Thank you for that. It makes sense but just wasn't sure. So I guess the only thing left is to officially open this to review? And then open a subsequent issue to write a test for the stats page (that addresses the data issues you noticed?)

I got rid of the data issue (we were passing session as a prop, but it was unused).

@amandamartin-dev amandamartin-dev marked this pull request as ready for review April 25, 2023 13:09
Copy link
Contributor

@amandamartin-dev amandamartin-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ran in gitpod and checked the playwright reports for good measure LGTM!!

@eddiejaoude
Copy link
Member Author

Wow great work 🚀 ! I was out yesterday and looks like you both had fun and success 🎉

@eddiejaoude
Copy link
Member Author

Looks great, amazing work, thank you both!

I kept getting timing outs on the search and map tests locally, and this was due to the use list api payload being so big, I hope you don't mind but I fixed it and added it to this PR - please review it though.

@eddiejaoude eddiejaoude merged commit 9da69cc into main Apr 26, 2023
@eddiejaoude eddiejaoude deleted the test-mock-oauth branch April 26, 2023 05:23
@eddiejaoude
Copy link
Member Author

I got too excited and merged it 😱 I am so excited we have this feature 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue linked Pull Request has issue linked large Pull request with more than 30 changed lines tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[OTHER] Playwright automated tests for authenticated pages
4 participants