Skip to content

Commit 903bdef

Browse files
authored
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 <[email protected]>
1 parent 840d240 commit 903bdef

File tree

2 files changed

+21
-5
lines changed

2 files changed

+21
-5
lines changed

models/oauth2.go

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -155,11 +155,6 @@ func initOAuth2LoginSources() error {
155155
err := oauth2.RegisterProvider(source.Name, oAuth2Config.Provider, oAuth2Config.ClientID, oAuth2Config.ClientSecret, oAuth2Config.OpenIDConnectAutoDiscoveryURL, oAuth2Config.CustomURLMapping)
156156
if err != nil {
157157
log.Critical("Unable to register source: %s due to Error: %v. This source will be disabled.", source.Name, err)
158-
source.IsActived = false
159-
if err = UpdateSource(source); err != nil {
160-
log.Critical("Unable to update source %s to disable it. Error: %v", err)
161-
return err
162-
}
163158
}
164159
}
165160
return nil

modules/auth/oauth2/oauth2.go

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ package oauth2
77
import (
88
"net/http"
99
"net/url"
10+
"sync"
1011

1112
"code.gitea.io/gitea/modules/log"
1213
"code.gitea.io/gitea/modules/setting"
@@ -34,6 +35,7 @@ import (
3435
var (
3536
sessionUsersStoreKey = "gitea-oauth2-sessions"
3637
providerHeaderKey = "gitea-oauth2-provider"
38+
gothRWMutex = sync.RWMutex{}
3739
)
3840

3941
// CustomURLMapping describes the urls values to use when customizing OAuth2 provider URLs
@@ -60,6 +62,10 @@ func Init(x *xorm.Engine) error {
6062

6163
// Note, when using the FilesystemStore only the session.ID is written to a browser cookie, so this is explicit for the storage on disk
6264
store.MaxLength(setting.OAuth2.MaxTokenLength)
65+
66+
gothRWMutex.Lock()
67+
defer gothRWMutex.Unlock()
68+
6369
gothic.Store = store
6470

6571
gothic.SetState = func(req *http.Request) string {
@@ -82,6 +88,9 @@ func Auth(provider string, request *http.Request, response http.ResponseWriter)
8288
// normally the gothic library will write some custom stuff to the response instead of our own nice error page
8389
//gothic.BeginAuthHandler(response, request)
8490

91+
gothRWMutex.RLock()
92+
defer gothRWMutex.RUnlock()
93+
8594
url, err := gothic.GetAuthURL(response, request)
8695
if err == nil {
8796
http.Redirect(response, request, url, http.StatusTemporaryRedirect)
@@ -95,6 +104,9 @@ func ProviderCallback(provider string, request *http.Request, response http.Resp
95104
// not sure if goth is thread safe (?) when using multiple providers
96105
request.Header.Set(providerHeaderKey, provider)
97106

107+
gothRWMutex.RLock()
108+
defer gothRWMutex.RUnlock()
109+
98110
user, err := gothic.CompleteUserAuth(response, request)
99111
if err != nil {
100112
return user, err
@@ -108,6 +120,9 @@ func RegisterProvider(providerName, providerType, clientID, clientSecret, openID
108120
provider, err := createProvider(providerName, providerType, clientID, clientSecret, openIDConnectAutoDiscoveryURL, customURLMapping)
109121

110122
if err == nil && provider != nil {
123+
gothRWMutex.Lock()
124+
defer gothRWMutex.Unlock()
125+
111126
goth.UseProviders(provider)
112127
}
113128

@@ -116,11 +131,17 @@ func RegisterProvider(providerName, providerType, clientID, clientSecret, openID
116131

117132
// RemoveProvider removes the given OAuth2 provider from the goth lib
118133
func RemoveProvider(providerName string) {
134+
gothRWMutex.Lock()
135+
defer gothRWMutex.Unlock()
136+
119137
delete(goth.GetProviders(), providerName)
120138
}
121139

122140
// ClearProviders clears all OAuth2 providers from the goth lib
123141
func ClearProviders() {
142+
gothRWMutex.Lock()
143+
defer gothRWMutex.Unlock()
144+
124145
goth.ClearProviders()
125146
}
126147

0 commit comments

Comments
 (0)