FEATURE: Use full page redirection for all external auth methods (PR #8092)

Using popups is becoming increasingly rare. Full page redirects are already used on mobile, and for some providers. This commit removes all logic related to popup authentication, leaving only the full page redirect method.

For more info, see https://meta.discourse.org/t/do-we-need-popups-for-login/127988

GitHub

1 Like

You’ve signed the CLA, davidtaylorhq. Thank you! This pull request is ready for review.

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

What really scares me about this is that breaking auth is such a traumatic thing to do, and there are dozens if not more plugins for Discourse around auth that we can’t be 100% sure will work with these changes right unless we test them?

I guess what I’m getting at here is I’m not against merging it, and I think it’s a good idea, but we need a really good deployment/testing plan associated with this.

1 Like

This {} = {} is really odd syntax. It’s an options hash but nothing is destructed from it because it’s not used?

Maybe _opts would be clearer.

1 Like

return LoginMethod.buildPostForm(authUrl).then(form => form.submit());

2 Likes

while you are at it, this kind of things are very useless now… we used to have them because of this.get(“name”), but having name instead of this.name is not worth the extra lines IMO

2 Likes

I was trying to avoid changing the method signature. However, now that I think about it, that doesn’t really matter in Javascript. I’ll remove it.

2 Likes

We have lived with this on meta for a while now, I feel the risk is not too huge merging this.

It is such an awesome change, feel free to merge this once you are ready

1 Like