search

rtcharity / eahub.org

A global directory for effective altruists to connect
https://eahub.org
MIT License
18 stars 6 forks source link

sso with google #1239

Closed viktor-yunenko closed 3 years ago

viktor-yunenko commented 3 years ago

Hi @sebgrebe, it's a small addition, could you approve it for a go-live?

I didn't handle the styles in a great way, but still I advise to push live & test it like that before we do the forum post.

Related to #1170

viktor-yunenko commented 3 years ago

hm, ok there's an issue for existing users, the flow doesn't look good

viktor-yunenko commented 3 years ago

Fixed now.

viktor-yunenko commented 3 years ago

@sebgrebe, I would say the sooner we review & deploy it - the faster we can import the data safely without worrying that there's a hidden issue with the SSO that we didn't know about ;)

viktor-yunenko commented 3 years ago

Also I expect it to be the most popular option after the forum post.

sebgrebe commented 3 years ago

When trying to login or signup with my Gmail account, I'm not receiving the verification email. I assume that's because we're not using the right EMAIL_URL in the environment variable of the SSO server. How did you check on your end that it worked? Locally?

This was caused by me having had an account with an unverified email address on that server under my gmail address. Once I deleted that account, it went smoothly.

Do we have a weird edge case here where someone signs up with their email, doesn't verify the email address and then tries to login with a google account with the same email address? Shouldn't it ignore that you haven't verified your email initially in this case as a Google email is verified by definition?

viktor-yunenko commented 3 years ago

Do we have a weird edge case here where someone signs up with their email, doesn't verify the email address and then tries to login with a google account with the same email address? Shouldn't it ignore that you haven't verified your email initially in this case as a Google email is verified by definition?

We should, but since django-allauth was enabled with multi-emails it creates complications. I thought about cutting it out several months ago, but back then I didn't have a good reason except for the potential issues.

For now I propose to leave it in place. Yes, the stage doesn't have a real email SMTP, which should be the case. It should have mailtrap on it though in the future.

Otherwise yes, you would have received a confirmation email.

sebgrebe commented 3 years ago

We should, but since django-allauth was enabled with multi-emails it creates complications. I thought about cutting it out several months ago, but back then I didn't have a good reason except for the potential issues.

For now I propose to leave it in place.

Sounds good, the number of affected users will be very small (if any). I've approved.

Can we add a ticket for fixing it so that we don't forget?

viktor-yunenko commented 3 years ago

Sure, but do you want to fix it though? They will receive the confirmation email. In the future I would prefer to prioritize cutting out the multi-email feature, since I don't think we need it?