Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 3 commits into from
Jul 29, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8d4d27f
Add locking to goth and gothic calls with a RWMutex
zeripath Jul 27, 2021
da50ede
Reattempt oauth2 registration on login if registration failed
zeripath Jul 28, 2021
b8d35cd
Merge branch 'main' into fix-16096-re-attempt-oauth2-registration-and…
6543 Jul 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions services/auth/source/oauth2/init.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package oauth2

import (
"net/http"
"sync"

"code.gitea.io/gitea/models"
"code.gitea.io/gitea/modules/log"
Expand All @@ -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"

Expand Down Expand Up @@ -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 {
Expand All @@ -52,6 +59,9 @@ func Init() error {
return req.Header.Get(ProviderHeaderKey), nil
}

// Unlock our mutex
gothRWMutex.Unlock()

return initOAuth2LoginSources()
}

Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
return nil
Expand Down
9 changes: 9 additions & 0 deletions services/auth/source/oauth2/providers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand All @@ -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()
}

Expand Down
6 changes: 6 additions & 0 deletions services/auth/source/oauth2/source_callout.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Expand Down