search

DSGT-DLP / Deep-Learning-Playground

Web Application where people new to Deep Learning can input a dataset and toy around with basic Pytorch modules without writing any code
MIT License
24 stars 8 forks source link

Fix auth #925

Closed dwu359 closed 1 year ago

dwu359 commented 1 year ago

Fix Authentication

What user problem are we solving? Signing In With Github didn't work on Firefox or Safari.

What solution does this PR provide? This provides a temporary fix of signing in with popup instead of redirects. The popup method is ok, but there is also the chance where some setting in a user's browser prevents the popup from appearing.

My guess for why signing in via redirects didn't work is that right now all of firebase's signinwithredirect methods utilize third-party cookies, which are gradually being deprecated.

There seems a growing standard for browsers to use FedCM for identity federation instead: https://github.com/fedidcg/FedCM/blob/main/explainer.md. FedCM however seems really new (I asked ChatGPT and it had no idea what it is), and doesn't have support for all of the major browsers yet (mainly firefox and safari), but it seems like those two also plan to support FedCM in the future.

This is what firebase suggests as fixes to this problem in the meantime: https://firebase.google.com/docs/auth/web/redirect-best-practices.

I chose the signInWithPopup option in the website above since it was the simplest, but for production we could try out the reverse proxy option using nginx if we want to keep redirects for prod.

Testing Methodology Signing in with github and google works now in firefox and safari and chrome.

Any other considerations

karkir0003 commented 1 year ago

what abt chrome? @dwu359

karkir0003 commented 1 year ago

@dwu359 screen recording of fix?

dwu359 commented 1 year ago

Yes it worked on chrome, and it still works on chrome. Idt a screen recording is necessary, but I can provide one if you guys have trouble testing it.

karkir0003 commented 1 year ago

@dwu359 have you verified that the user can stay signed in if we sign in, close and reopen the page? is that functionality affected since the local storage update stuff was removed

dwu359 commented 1 year ago

Yes that still happens, the only difference is that a loading screen doesn't appear for the split second between the page loading and firebase loading auth. The user shouldn't notice for most cases.

karkir0003 commented 1 year ago

@dwu359 wait on @farisdurrani to review

farisdurrani commented 1 year ago

Looks good, feel free to merge