Lock goth/gothic and Re-attempt OAuth2 registration on login if registration failed at startup #16564

zeripath · 2021-07-27T20:23:26Z

zeripath commented

2021-07-27 20:23:26 +00:00

This PR has two parts:

Add locking to goth and gothic calls with a RWMutex

The goth and gothic calls are currently unlocked and thus are a cause of multiple potential races

Reattempt OAuth2 registration on login if registration failed

If OAuth2 registration fails at startup we currently disable the login_source however an alternative approach could be to reattempt registration on login attempt.

Fix #16096

This PR has two parts: * Add locking to goth and gothic calls with a RWMutex The goth and gothic calls are currently unlocked and thus are a cause of multiple potential races * Reattempt OAuth2 registration on login if registration failed If OAuth2 registration fails at startup we currently disable the login_source however an alternative approach could be to reattempt registration on login attempt. Fix #16096

lunny reviewed 2021-07-28 01:35:35 +00:00

services/auth/source/oauth2/init.go

lunny commented

2021-07-28 01:35:35 +00:00

These codes added here to allow Gitea start up when some auth sources take down temporarily.

zeripath reviewed 2021-07-28 07:35:08 +00:00

services/auth/source/oauth2/init.go

zeripath commented

2021-07-28 07:35:08 +00:00

Yes.

The proposed changes in the Auth and Callback handlers allow us to stop auto disabling and reattempt to register at login time if registration failed at startup.

(But you've reminded me I need to recheck GetProviders once we have the hard lock to prevent double registration.)

If that "second" attempt at registration fails again then the user will be presented with a 500 page - but it genuinely is an internal server error at that point - although I fully expect that there will be complaints about that too.

(Hopefully such complaints would actually come with logs so we could at least attempt to present a nicer error page.)

Yes. The proposed changes in the Auth and Callback handlers allow us to stop auto disabling and reattempt to register at login time if registration failed at startup. (But you've reminded me I need to recheck GetProviders once we have the hard lock to prevent double registration.) If that "second" attempt at registration fails again then the user will be presented with a 500 page - but it genuinely is an internal server error at that point - although I fully expect that there will be complaints about that too. (Hopefully such complaints would actually come with logs so we could at least attempt to present a nicer error page.)

zeripath reviewed 2021-07-28 08:17:26 +00:00

services/auth/source/oauth2/init.go

zeripath commented

2021-07-28 08:17:25 +00:00

(But you've reminded me I need to recheck GetProviders once we have the hard lock to prevent double registration.)

Actually we're ok from the double register PoV but the issue is that RegisterSource could be somewhat slow so users may double/triple attempt to login. Will have a think.

> (But you've reminded me I need to recheck GetProviders once we have the hard lock to prevent double registration.) Actually we're ok from the double register PoV but the issue is that RegisterSource could be somewhat slow so users may double/triple attempt to login. Will have a think.

zeripath reviewed 2021-07-28 08:31:58 +00:00

services/auth/source/oauth2/init.go

zeripath commented

2021-07-28 08:31:58 +00:00

Hmm... looking at routers/web/user/auth.go:582 and the changes made by #14116 resetting is actually already handled.

So I'll let #14116 handle the re-registration attempt.

Hmm... looking at routers/web/user/auth.go:582 and the changes made by #14116 resetting is actually already handled. So I'll let #14116 handle the re-registration attempt.