Lock goth/gothic and Re-attempt OAuth2 registration on login if registration failed at startup (#16570)
Backport #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 <art27@cantab.net>
This commit is contained in:
		
							parent
							
								
									840d240a61
								
							
						
					
					
						commit
						903bdefb58
					
				
					 2 changed files with 21 additions and 5 deletions
				
			
		|  | @ -155,11 +155,6 @@ func initOAuth2LoginSources() error { | ||||||
| 		err := oauth2.RegisterProvider(source.Name, oAuth2Config.Provider, oAuth2Config.ClientID, oAuth2Config.ClientSecret, oAuth2Config.OpenIDConnectAutoDiscoveryURL, oAuth2Config.CustomURLMapping) | 		err := oauth2.RegisterProvider(source.Name, oAuth2Config.Provider, oAuth2Config.ClientID, oAuth2Config.ClientSecret, oAuth2Config.OpenIDConnectAutoDiscoveryURL, oAuth2Config.CustomURLMapping) | ||||||
| 		if err != nil { | 		if err != nil { | ||||||
| 			log.Critical("Unable to register source: %s due to Error: %v. This source will be disabled.", source.Name, err) | 			log.Critical("Unable to register source: %s due to Error: %v. This source will be disabled.", source.Name, err) | ||||||
| 			source.IsActived = false |  | ||||||
| 			if err = UpdateSource(source); err != nil { |  | ||||||
| 				log.Critical("Unable to update source %s to disable it. Error: %v", err) |  | ||||||
| 				return err |  | ||||||
| 			} |  | ||||||
| 		} | 		} | ||||||
| 	} | 	} | ||||||
| 	return nil | 	return nil | ||||||
|  |  | ||||||
|  | @ -7,6 +7,7 @@ package oauth2 | ||||||
| import ( | import ( | ||||||
| 	"net/http" | 	"net/http" | ||||||
| 	"net/url" | 	"net/url" | ||||||
|  | 	"sync" | ||||||
| 
 | 
 | ||||||
| 	"code.gitea.io/gitea/modules/log" | 	"code.gitea.io/gitea/modules/log" | ||||||
| 	"code.gitea.io/gitea/modules/setting" | 	"code.gitea.io/gitea/modules/setting" | ||||||
|  | @ -34,6 +35,7 @@ import ( | ||||||
| var ( | var ( | ||||||
| 	sessionUsersStoreKey = "gitea-oauth2-sessions" | 	sessionUsersStoreKey = "gitea-oauth2-sessions" | ||||||
| 	providerHeaderKey    = "gitea-oauth2-provider" | 	providerHeaderKey    = "gitea-oauth2-provider" | ||||||
|  | 	gothRWMutex          = sync.RWMutex{} | ||||||
| ) | ) | ||||||
| 
 | 
 | ||||||
| // CustomURLMapping describes the urls values to use when customizing OAuth2 provider URLs
 | // CustomURLMapping describes the urls values to use when customizing OAuth2 provider URLs
 | ||||||
|  | @ -60,6 +62,10 @@ func Init(x *xorm.Engine) 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
 | 	// 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) | 	store.MaxLength(setting.OAuth2.MaxTokenLength) | ||||||
|  | 
 | ||||||
|  | 	gothRWMutex.Lock() | ||||||
|  | 	defer gothRWMutex.Unlock() | ||||||
|  | 
 | ||||||
| 	gothic.Store = store | 	gothic.Store = store | ||||||
| 
 | 
 | ||||||
| 	gothic.SetState = func(req *http.Request) string { | 	gothic.SetState = func(req *http.Request) string { | ||||||
|  | @ -82,6 +88,9 @@ func Auth(provider string, request *http.Request, response http.ResponseWriter) | ||||||
| 	// normally the gothic library will write some custom stuff to the response instead of our own nice error page
 | 	// normally the gothic library will write some custom stuff to the response instead of our own nice error page
 | ||||||
| 	//gothic.BeginAuthHandler(response, request)
 | 	//gothic.BeginAuthHandler(response, request)
 | ||||||
| 
 | 
 | ||||||
|  | 	gothRWMutex.RLock() | ||||||
|  | 	defer gothRWMutex.RUnlock() | ||||||
|  | 
 | ||||||
| 	url, err := gothic.GetAuthURL(response, request) | 	url, err := gothic.GetAuthURL(response, request) | ||||||
| 	if err == nil { | 	if err == nil { | ||||||
| 		http.Redirect(response, request, url, http.StatusTemporaryRedirect) | 		http.Redirect(response, request, url, http.StatusTemporaryRedirect) | ||||||
|  | @ -95,6 +104,9 @@ func ProviderCallback(provider string, request *http.Request, response http.Resp | ||||||
| 	// not sure if goth is thread safe (?) when using multiple providers
 | 	// not sure if goth is thread safe (?) when using multiple providers
 | ||||||
| 	request.Header.Set(providerHeaderKey, provider) | 	request.Header.Set(providerHeaderKey, provider) | ||||||
| 
 | 
 | ||||||
|  | 	gothRWMutex.RLock() | ||||||
|  | 	defer gothRWMutex.RUnlock() | ||||||
|  | 
 | ||||||
| 	user, err := gothic.CompleteUserAuth(response, request) | 	user, err := gothic.CompleteUserAuth(response, request) | ||||||
| 	if err != nil { | 	if err != nil { | ||||||
| 		return user, err | 		return user, err | ||||||
|  | @ -108,6 +120,9 @@ func RegisterProvider(providerName, providerType, clientID, clientSecret, openID | ||||||
| 	provider, err := createProvider(providerName, providerType, clientID, clientSecret, openIDConnectAutoDiscoveryURL, customURLMapping) | 	provider, err := createProvider(providerName, providerType, clientID, clientSecret, openIDConnectAutoDiscoveryURL, customURLMapping) | ||||||
| 
 | 
 | ||||||
| 	if err == nil && provider != nil { | 	if err == nil && provider != nil { | ||||||
|  | 		gothRWMutex.Lock() | ||||||
|  | 		defer gothRWMutex.Unlock() | ||||||
|  | 
 | ||||||
| 		goth.UseProviders(provider) | 		goth.UseProviders(provider) | ||||||
| 	} | 	} | ||||||
| 
 | 
 | ||||||
|  | @ -116,11 +131,17 @@ func RegisterProvider(providerName, providerType, clientID, clientSecret, openID | ||||||
| 
 | 
 | ||||||
| // RemoveProvider removes the given OAuth2 provider from the goth lib
 | // RemoveProvider removes the given OAuth2 provider from the goth lib
 | ||||||
| func RemoveProvider(providerName string) { | func RemoveProvider(providerName string) { | ||||||
|  | 	gothRWMutex.Lock() | ||||||
|  | 	defer gothRWMutex.Unlock() | ||||||
|  | 
 | ||||||
| 	delete(goth.GetProviders(), providerName) | 	delete(goth.GetProviders(), providerName) | ||||||
| } | } | ||||||
| 
 | 
 | ||||||
| // ClearProviders clears all OAuth2 providers from the goth lib
 | // ClearProviders clears all OAuth2 providers from the goth lib
 | ||||||
| func ClearProviders() { | func ClearProviders() { | ||||||
|  | 	gothRWMutex.Lock() | ||||||
|  | 	defer gothRWMutex.Unlock() | ||||||
|  | 
 | ||||||
| 	goth.ClearProviders() | 	goth.ClearProviders() | ||||||
| } | } | ||||||
| 
 | 
 | ||||||
|  |  | ||||||
		Loading…
	
		Reference in a new issue