freeCodeCamp / CodeAlly-CodeRoad-freeCodeCamp

10 stars 5 forks source link

CodeAlly not available to Users when third-party cookies are disabled. #42

Closed raisedadead closed 2 years ago

raisedadead commented 2 years ago

Affected page

All pages that use CodeAlly

To reproduce

Steps to reproduce the behavior:

  1. Open an incognito Window
  2. Go to a challenge page
  3. Click on 'Click here to start the course'
  4. See error.

Expected behavior

A better UX

Screenshots

image
moT01 commented 2 years ago

I'm not sure what the solution is here. You need to be logged in to github so you can log in to CodeAlly to run these - so an incognito window won't work. Looks like there's a few seemingly hacky ways to detect that - I didn't look into it much yet, but we could show a pop-up if someone tries to start a course using incognito. Or we could just add a note on the main page - maybe "You cannot run these using an incognito or private window" or something. Or CodeAlly could give a better screen there? Perhaps a combination of a better screen on CodeAlly's end and one of the other two. If we did something on our end to stop a user before they try to start, CodeAlly wouldn't have to change anything.

AdamZaczek commented 2 years ago

We can see that many users try this so this one will be fixed asap. I agree with Tom's suggestions. We need to add information that you need to have local storage enabled though because CodeAlly does not work in incognito anyways. We will check if it's available by checking if we can write into it.

raisedadead commented 2 years ago

I think I know what is happening here. I have had third-party cookies disabled in incognito mode. This is the default OOTB behavior of Chrome when in incognito.

Meaning the onus comes on us check if TP cookies are disabled and show an error page.

raisedadead commented 2 years ago

You can replicate the error by doing these:

  1. Visit: chrome://settings/cookies
  2. Disable TP Cookies.
  3. Visit the challenge page & click start course.

Beyond that (i.e, when TP cookies are allowed) you should see the CodeAlly button normally. So this is not even a GitHub thing at all.

I enabled TP cookies for incognito and got to the GitHub screen like I should have in the first place:

Details image

We just have to figure out a way to detect and display the user a proper message for these TP cookies.

moT01 commented 2 years ago

There has been a flurry of this issue lately. We discussed it internally a little bit. Part of the conversation went something like this...

"we could attach the session info to each request as an Authorization header, rather than a cookie." "Are you suggesting that we ask CodeAlly to change the way they auth users on their own platform?" "I don't think it's reasonable to ask them to put in tons of work just for us, but if they're interested in avoiding the problems surrounding third party cookies anyway that could be worth exploring."

Is this something you might consider doing @AdamZaczek? Is there any more details you want to provide @raisedadead @ojeytonwilliams? Not sure any more are needed. I guess the argument for implementing it is that anyone who wants to embed CodeAlly will run into this problem.

I think we're going to add some sort of message on our end for now, but we wanted to ask about this approach.

AdamZaczek commented 2 years ago

Changing a login system is too much.

I had the same idea as you did but on our end and I wanted to give it a shot this week.

I think about catching localStorage access denial and displaying a message if it's not supported. I'm thinking to do this at the CodeAlly level, we can be sure that we catch the issue and display a message only if that's happening.

If it can't be accessed, we will let users know how to enable it. I think I can have a working version in the beta environment to share in no time. The only reason it's still not there is that we're adding soft and hard reset functionalities as well.

I could add a demo by the end of the week and put it on

Should I add this and put it on https://appbeta.codeally.io/ so you can test how it works?

moT01 commented 2 years ago

Does this sound like a valid approach @freeCodeCamp/dev-team? So CodeAlly checks for localStorage access - and if it doesn't work, users get a message. So if a user is running CodeAlly in the iframe and has third party cookies blocked, CodeAlly wouldn't be able to use local storage - is that what would happen there?

Will we be able to embed using that domain to test it out @AdamZaczek? Using a URL like https://appbeta.codeally.io/embed/?repoUrl=https://github.com/freeCodeCamp/learn-bash-by-building-a-boilerplate. If so - yes, add it there and we can give it a quick test.

naomi-lgbt commented 2 years ago

This sounds like an excellent approach to me.

ShaunSHamilton commented 2 years ago

Will we need to enable something like allow-storage-access-by-user-activation for this test?

AdamZaczek commented 2 years ago

One set of changes (1) is ready to be tested. The other requires a tweak on Freecodecamp's side.

@ShaunSHamilton

allow-storage-access-by-user-activation

is made to solve our exact issue but it's non-standard and not widely supported according to https://caniuse.com/mdn-html_elements_iframe_sandbox-allow-storage-access-by-user-activation. I figured out a way that should work everywhere.

1. Issues with 3rd party localStorage, happening on incognito and Brave.

I pushed https://appbeta.codeally.io/ embedded within a different domain to reproduce what's going on: https://teal-sunburst-97d62b.netlify.app/.

If you open the example in incognito, you will see the message. We can modify it in no time if you want.

image

It detects localStorage crashes so any case of 3rd party local storage not supported will be caught.

PS I added soft reset too! Feel free to give it a try:

image

2. Brave's 3rd party cookies.

This behavior was more complex to solve. In particular, Brave will block login when cookies are set like in the screenshot:

image

This results in users not being able to log in, and seeing this screen after an attempt of authentication:

image

What's really happening is that authentication works but the localStorage from the app can not be shared with an iframe. The good news is that we already have a logged-in user that we can use to make login work.

We send you a tempToken in query params in the URL after a user logs in. If you pass this token to an iframe as query param, the user will be logged in, even on Brave. An example of how you could pass tempToken to a URL:

&tempToken=${localStorage.getItem('tempToken')}

We will use it to authenticate and all incognito + Brave issues will be handled.

To test appbeta.codeally.io you can use my dev tempToken:

eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJfaWQiOiI2MTk4Mjg4NjFiZjU1NTFjOTg5ZGEyZWQiLCJ0eXBlIjoiZGVmYXVsdCIsImlhdCI6MTY1MDEwNTk4Nn0.1cdfjaucCfTk1hddb16-0MzFpEdRHhU75ZZKroXTWh4

tempToken demo

This example has a user logged in when Brave blocks the cookies by passing a tempToken to the iframe. https://teal-sunburst-97d62b.netlify.app/passing-temp-token

Note: I believe we should also handle https://github.com/freeCodeCamp/CodeAlly-CodeRoad-freeCodeCamp/issues/46 at the same time. Otherwise, deploying the fix might cause more issues than solve.

Conclusions

Looks like we only need a bunch of tests and a small tweak to the iframe's src. If you want my help or more explanation, I'll be happy to jump for a short call.

moT01 commented 2 years ago

This is looking good @AdamZaczek - I think anyway...

I'm not quite sure what I need to do on my end to get brave working. After I log in to CodeAlly, I get redirected back to https://www.freecodecamp.org/learn/relational-database/learn-bash-by-building-a-boilerplate/build-a-boilerplate or whatever page I was on - so you're saying that will include a query parameter? maybe tempToken=<token-string-here>? Then I should include that in the embed URL - maybe https://codeally.io/embed/?repoUrl=<url>&envVariables=CODEROAD_WEBHOOK_TOKEN=<user_token>;tempToken=<tempToken> I may need more explanation. So then it's basically a hack around someone blocking third party cookies and they will be able to log in even though they are blocked?

CodeRoad isn't on these test domains (as far as I can tell), so I couldn't test the soft reset button while in the middle of a tutorial. All it does is shut down and restart the VM? It should be fine if that's the case.

AdamZaczek commented 2 years ago

@moT01 We pass a token for you. If you log in at

https://teal-sunburst-97d62b.netlify.app/

you will see that you will come back with tempToken in query params. We need this param in iframes src to share sessions. I would store it in localStorage and pass it to iframe.

Let's have a chat to make sure you have all the info you need.

moT01 commented 2 years ago

Tested as much as I could before the meeting - here's what I saw:

URL: teal-sunburst-97d62b.netlify.app teal-sunburst-97d62b.netlify.app teal-sunburst-97d62b.netlify.app/passing-temp-token teal-sunburst-97d62b.netlify.app/passing-temp-token
Third party cookies: disabled enabled disabled enabled
Firefox get message got stuck in login loop - then it worked… I primarily use this with CodeAlly and don’t have issues so it’s probably fine get message can login and run VM
Chrome get message can login and run VM get message can login and run VM
Safari (old) get message get stuck in a limbo screen get message can login and run VM
Brave stuck in login loop can login and run VM can login and run VM can login and run VM
AdamZaczek commented 2 years ago

Here's our todo list:

  1. Pass the tempToken to the iframe, just like we pass envVariables. Example https://teal-sunburst-97d62b.netlify.app/passing-temp-token/
  2. Add a new Date.getMinutes() or similar to the iframe to fix https://github.com/freeCodeCamp/CodeAlly-CodeRoad-freeCodeCamp/issues/46
  3. Decide if we want any changes in the message shown when localStorage is disabled https://user-images.githubusercontent.com/14284341/163595977-9d49a40d-abf0-4a1a-ba76-52065db13114.png
  4. Schedule the deployment of the new Freecodecamp version and the new CodeAlly for the same hour so we can prevent issue 46.

Note: On development, iframes should be done using our beta https://appbeta.codeally.io/app.

moT01 commented 2 years ago

Hey @AdamZaczek - mind taking a quick look at that PR? I think it's what we wanted. I am getting the tempToken and passing it in, and I added a Date.now() to fix those webpack caching issues.

AdamZaczek commented 2 years ago

Looks good! I'm 95% sure we didn't miss anything. We had to deploy a few weeks ago due to Netlify no longer working for us (when you saw some crashes for a few hours) so the changes are online on our end.

Let me know when you deploy and let's test this!

moT01 commented 2 years ago

PR's have been merged - will be live on next deploy 🎉

AdamZaczek commented 2 years ago

Works! Note - I still see some issues with Brave - we don't correctly get the goBackTo there but it looks like it's the only browsers when things don't work as planned just yet.

Caching is still an issue for returning users. We'll try to fix this for good. For now - visiting codeally directly a few times fixes it.

Glad to have this live!

raisedadead commented 2 years ago

Sorry, @AdamZaczek - I think we confused you accidentally. We merged the PR (last evening, my time) and deployed it just now (as of this comment, it's all in production).

Cheers and thanks again for your excellent collab on this.