Lock goth/gothic and Re-attempt OAuth2 registration on login if registration failed...

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 <art27@cantab.net>

diff --git a/services/auth/source/oauth2/init.go b/services/auth/source/oauth2/init.go
index f797fd7fd48521f04320be7f6063477333f7c333..64328aa381789b4d251006a08e0649a97f8e0d85 100644 (file)
--- 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 bf97f8002aa34eb26e053e8764f12a5166aa485e..8df8d629617af01d1fb94efbd520e7f1da87e97e 100644 (file)
--- 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 8f4663f3bea128e3ab04e98682dd66e8dfbfe5e2..c0ac7e041052992bf187f523f07ce967472c6d9b 100644 (file)
--- 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