From 72738f0cb5332f9f7a606b3a43970a5619ba0f64 Mon Sep 17 00:00:00 2001 From: zeripath Date: Thu, 29 Jul 2021 18:53:18 +0100 Subject: [PATCH] Lock goth/gothic and Re-attempt OAuth2 registration on login if registration failed at startup (#16564) 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 Signed-off-by: Andrew Thornton --- services/auth/source/oauth2/init.go | 17 +++++++++++------ services/auth/source/oauth2/providers.go | 9 +++++++++ services/auth/source/oauth2/source_callout.go | 6 ++++++ 3 files changed, 26 insertions(+), 6 deletions(-) diff --git a/services/auth/source/oauth2/init.go b/services/auth/source/oauth2/init.go index f797fd7fd..64328aa38 100644 --- a/services/auth/source/oauth2/init.go +++ b/services/auth/source/oauth2/init.go @@ -6,6 +6,7 @@ package oauth2 import ( "net/http" + "sync" "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/log" @@ -15,6 +16,8 @@ import ( "github.com/markbates/goth/gothic" ) +var gothRWMutex = sync.RWMutex{} + // SessionTableName is the table name that OAuth2 will use to store things const SessionTableName = "oauth2_session" @@ -42,6 +45,10 @@ func Init() error { // Note, when using the FilesystemStore only the session.ID is written to a browser cookie, so this is explicit for the storage on disk store.MaxLength(setting.OAuth2.MaxTokenLength) + + // Lock our mutex + gothRWMutex.Lock() + gothic.Store = store gothic.SetState = func(req *http.Request) string { @@ -52,6 +59,9 @@ func Init() error { return req.Header.Get(ProviderHeaderKey), nil } + // Unlock our mutex + gothRWMutex.Unlock() + return initOAuth2LoginSources() } @@ -71,12 +81,7 @@ func initOAuth2LoginSources() error { } err := oauth2Source.RegisterSource() if err != nil { - log.Critical("Unable to register source: %s due to Error: %v. This source will be disabled.", source.Name, err) - source.IsActive = false - if err = models.UpdateSource(source); err != nil { - log.Critical("Unable to update source %s to disable it. Error: %v", err) - return err - } + log.Critical("Unable to register source: %s due to Error: %v.", source.Name, err) } } return nil diff --git a/services/auth/source/oauth2/providers.go b/services/auth/source/oauth2/providers.go index bf97f8002..8df8d6296 100644 --- a/services/auth/source/oauth2/providers.go +++ b/services/auth/source/oauth2/providers.go @@ -121,6 +121,9 @@ func RegisterProvider(providerName, providerType, clientID, clientSecret, openID provider, err := createProvider(providerName, providerType, clientID, clientSecret, openIDConnectAutoDiscoveryURL, customURLMapping) if err == nil && provider != nil { + gothRWMutex.Lock() + defer gothRWMutex.Unlock() + goth.UseProviders(provider) } @@ -129,11 +132,17 @@ func RegisterProvider(providerName, providerType, clientID, clientSecret, openID // RemoveProvider removes the given OAuth2 provider from the goth lib func RemoveProvider(providerName string) { + gothRWMutex.Lock() + defer gothRWMutex.Unlock() + delete(goth.GetProviders(), providerName) } // ClearProviders clears all OAuth2 providers from the goth lib func ClearProviders() { + gothRWMutex.Lock() + defer gothRWMutex.Unlock() + goth.ClearProviders() } diff --git a/services/auth/source/oauth2/source_callout.go b/services/auth/source/oauth2/source_callout.go index 8f4663f3b..c0ac7e041 100644 --- a/services/auth/source/oauth2/source_callout.go +++ b/services/auth/source/oauth2/source_callout.go @@ -20,6 +20,9 @@ func (source *Source) Callout(request *http.Request, response http.ResponseWrite // normally the gothic library will write some custom stuff to the response instead of our own nice error page //gothic.BeginAuthHandler(response, request) + gothRWMutex.RLock() + defer gothRWMutex.RUnlock() + url, err := gothic.GetAuthURL(response, request) if err == nil { http.Redirect(response, request, url, http.StatusTemporaryRedirect) @@ -33,6 +36,9 @@ func (source *Source) Callback(request *http.Request, response http.ResponseWrit // not sure if goth is thread safe (?) when using multiple providers request.Header.Set(ProviderHeaderKey, source.loginSource.Name) + gothRWMutex.RLock() + defer gothRWMutex.RUnlock() + user, err := gothic.CompleteUserAuth(response, request) if err != nil { return user, err