From 346520a5f9e8774e9941a94e333ced482da8435d Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 22 Mar 2022 19:37:19 +0800 Subject: [PATCH 1/5] remove the global methods but create dynamiclly --- routers/api/v1/api.go | 22 +++++++++++- routers/api/v1/auth.go | 12 +++++++ routers/api/v1/auth_windows.go | 11 ++++++ routers/web/auth.go | 12 +++++++ routers/web/auth_windows.go | 16 +++++++++ routers/web/web.go | 27 ++++++++++++++- services/auth/auth.go | 62 +++------------------------------- services/auth/group.go | 20 +++++++++++ 8 files changed, 122 insertions(+), 60 deletions(-) create mode 100644 routers/api/v1/auth.go create mode 100644 routers/api/v1/auth_windows.go create mode 100644 routers/web/auth.go create mode 100644 routers/web/auth_windows.go diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 4b30164026745..963e380857fd2 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -560,6 +560,26 @@ func bind(obj interface{}) http.HandlerFunc { }) } +// The OAuth2 plugin is expected to be executed first, as it must ignore the user id stored +// in the session (if there is a user id stored in session other plugins might return the user +// object for that id). +// +// The Session plugin is expected to be executed second, in order to skip authentication +// for users that have already signed in. +func buildAuthGroup() *auth.Group { + group := auth.NewGroup( + &auth.OAuth2{}, + &auth.Basic{}, + auth.SharedSession, + ) + if setting.Service.EnableReverseProxyAuth { + group.Add(&auth.ReverseProxy{}) + } + specialAdd(group) + + return group +} + // Routes registers all v1 APIs routes to web application. func Routes(sessioner func(http.Handler) http.Handler) *web.Route { m := web.NewRoute() @@ -581,7 +601,7 @@ func Routes(sessioner func(http.Handler) http.Handler) *web.Route { m.Use(context.APIContexter()) // Get user from session if logged in. - m.Use(context.APIAuth(auth.NewGroup(auth.Methods()...))) + m.Use(context.APIAuth(buildAuthGroup())) m.Use(context.ToggleAPI(&context.ToggleOptions{ SignInRequired: setting.Service.RequireSignInView, diff --git a/routers/api/v1/auth.go b/routers/api/v1/auth.go new file mode 100644 index 0000000000000..359c9ec56bcc7 --- /dev/null +++ b/routers/api/v1/auth.go @@ -0,0 +1,12 @@ +// Copyright 2022 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +//go:build !windows +// +build !windows + +package v1 + +import auth_service "code.gitea.io/gitea/services/auth" + +func specialAdd(group *auth_service.Group) {} diff --git a/routers/api/v1/auth_windows.go b/routers/api/v1/auth_windows.go new file mode 100644 index 0000000000000..b504dc5fda419 --- /dev/null +++ b/routers/api/v1/auth_windows.go @@ -0,0 +1,11 @@ +// Copyright 2022 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package v1 + +func specialAdd(group *auth_service.Group) { + if auth.IsSSPIEnabled() { + group.Add(&auth_service.SSPI{}) + } +} diff --git a/routers/web/auth.go b/routers/web/auth.go new file mode 100644 index 0000000000000..4a7fb856be2e7 --- /dev/null +++ b/routers/web/auth.go @@ -0,0 +1,12 @@ +// Copyright 2022 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +//go:build !windows +// +build !windows + +package web + +import auth_service "code.gitea.io/gitea/services/auth" + +func specialAdd(group *auth_service.Group) {} diff --git a/routers/web/auth_windows.go b/routers/web/auth_windows.go new file mode 100644 index 0000000000000..dd5c8e39f465e --- /dev/null +++ b/routers/web/auth_windows.go @@ -0,0 +1,16 @@ +// Copyright 2022 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package web + +import ( + "code.gitea.io/gitea/models/auth" + auth_service "code.gitea.io/gitea/services/auth" +) + +func specialAdd(group *auth_service.Group) { + if auth.IsSSPIEnabled() { + group.Add(&auth_service.SSPI{}) + } +} diff --git a/routers/web/web.go b/routers/web/web.go index b40a43058d423..ebcd7cb7345e4 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -71,6 +71,26 @@ func CorsHandler() func(next http.Handler) http.Handler { } } +// The OAuth2 plugin is expected to be executed first, as it must ignore the user id stored +// in the session (if there is a user id stored in session other plugins might return the user +// object for that id). +// +// The Session plugin is expected to be executed second, in order to skip authentication +// for users that have already signed in. +func buildAuthGroup() *auth_service.Group { + group := auth_service.NewGroup( + &auth_service.OAuth2{}, + &auth_service.Basic{}, + auth_service.SharedSession, + ) + if setting.Service.EnableReverseProxyAuth { + group.Add(&auth_service.ReverseProxy{}) + } + specialAdd(group) + + return group +} + // Routes returns all web routes func Routes(sessioner func(http.Handler) http.Handler) *web.Route { routes := web.NewRoute() @@ -158,8 +178,13 @@ func Routes(sessioner func(http.Handler) http.Handler) *web.Route { // Removed: toolbox.Toolboxer middleware will provide debug information which seems unnecessary common = append(common, context.Contexter()) + group := buildAuthGroup() + if err := group.Init(); err != nil { + log.Error("Could not initialize '%s' auth method, error: %s", group.Name(), err) + } + // Get user from session if logged in. - common = append(common, context.Auth(auth_service.NewGroup(auth_service.Methods()...))) + common = append(common, context.Auth(group)) // GetHead allows a HEAD request redirect to GET if HEAD method is not defined for that route common = append(common, middleware.GetHead) diff --git a/services/auth/auth.go b/services/auth/auth.go index bdff777f506e4..6d6edef14704c 100644 --- a/services/auth/auth.go +++ b/services/auth/auth.go @@ -8,7 +8,6 @@ package auth import ( "fmt" "net/http" - "reflect" "regexp" "strings" @@ -21,75 +20,22 @@ import ( "code.gitea.io/gitea/modules/web/middleware" ) -// authMethods contains the list of authentication plugins in the order they are expected to be -// executed. -// -// The OAuth2 plugin is expected to be executed first, as it must ignore the user id stored -// in the session (if there is a user id stored in session other plugins might return the user -// object for that id). -// -// The Session plugin is expected to be executed second, in order to skip authentication -// for users that have already signed in. -var authMethods = []Method{ - &OAuth2{}, - &Basic{}, - &Session{}, -} - // The purpose of the following three function variables is to let the linter know that // those functions are not dead code and are actually being used var ( _ = handleSignIn -) - -// Methods returns the instances of all registered methods -func Methods() []Method { - return authMethods -} -// Register adds the specified instance to the list of available methods -func Register(method Method) { - authMethods = append(authMethods, method) -} + // SharedSession the session auth should only be used by web, but now both web and API/v1 + // will use it. We can remvoe this after Web removed dependent API/v1 + SharedSession = &Session{} +) // Init should be called exactly once when the application starts to allow plugins // to allocate necessary resources func Init() { - if setting.Service.EnableReverseProxyAuth { - Register(&ReverseProxy{}) - } - specialInit() - for _, method := range Methods() { - initializable, ok := method.(Initializable) - if !ok { - continue - } - - err := initializable.Init() - if err != nil { - log.Error("Could not initialize '%s' auth method, error: %s", reflect.TypeOf(method).String(), err) - } - } - webauthn.Init() } -// Free should be called exactly once when the application is terminating to allow Auth plugins -// to release necessary resources -func Free() { - for _, method := range Methods() { - freeable, ok := method.(Freeable) - if !ok { - continue - } - - err := freeable.Free() - if err != nil { - log.Error("Could not free '%s' auth method, error: %s", reflect.TypeOf(method).String(), err) - } - } -} - // isAttachmentDownload check if request is a file download (GET) with URL to an attachment func isAttachmentDownload(req *http.Request) bool { return strings.HasPrefix(req.URL.Path, "/attachments/") && req.Method == "GET" diff --git a/services/auth/group.go b/services/auth/group.go index bf047338bbd59..0f40e1a76c9be 100644 --- a/services/auth/group.go +++ b/services/auth/group.go @@ -6,6 +6,8 @@ package auth import ( "net/http" + "reflect" + "strings" "code.gitea.io/gitea/models/db" user_model "code.gitea.io/gitea/models/user" @@ -30,6 +32,24 @@ func NewGroup(methods ...Method) *Group { } } +// Add adds a new method to group +func (b *Group) Add(method Method) { + b.methods = append(b.methods, method) +} + +// Name returns group's methods name +func (b *Group) Name() string { + names := make([]string, 0, len(b.methods)) + for _, m := range b.methods { + if n, ok := m.(Named); ok { + names = append(names, n.Name()) + } else { + names = append(names, reflect.TypeOf(m).Elem().Name()) + } + } + return strings.Join(names, ",") +} + // Init does nothing as the Basic implementation does not need to allocate any resources func (b *Group) Init() error { for _, method := range b.methods { From 5fc9986dc9773cdea896cab7a5ef1df86b556bb2 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 23 Mar 2022 10:18:32 +0800 Subject: [PATCH 2/5] Fix lint --- services/auth/placeholder.go | 10 ---------- 1 file changed, 10 deletions(-) delete mode 100644 services/auth/placeholder.go diff --git a/services/auth/placeholder.go b/services/auth/placeholder.go deleted file mode 100644 index d9a0ceae7c45a..0000000000000 --- a/services/auth/placeholder.go +++ /dev/null @@ -1,10 +0,0 @@ -// Copyright 2021 The Gitea Authors. All rights reserved. -// Use of this source code is governed by a MIT-style -// license that can be found in the LICENSE file. - -//go:build !windows -// +build !windows - -package auth - -func specialInit() {} From 3cb1d69a8ad6ebe611fd8f14c5d9bdd463a23be4 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 23 Mar 2022 10:30:15 +0800 Subject: [PATCH 3/5] Fix windows lint --- routers/api/v1/auth_windows.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/routers/api/v1/auth_windows.go b/routers/api/v1/auth_windows.go index b504dc5fda419..c6cd28440325f 100644 --- a/routers/api/v1/auth_windows.go +++ b/routers/api/v1/auth_windows.go @@ -4,6 +4,8 @@ package v1 +import auth_service "code.gitea.io/gitea/services/auth" + func specialAdd(group *auth_service.Group) { if auth.IsSSPIEnabled() { group.Add(&auth_service.SSPI{}) From 5de17ab2cf3cad74e3982f964bbe6b6f0fd0ec5d Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 23 Mar 2022 14:36:32 +0800 Subject: [PATCH 4/5] Fix windows lint --- routers/api/v1/auth_windows.go | 9 ++++++++- routers/web/auth_windows.go | 4 ++++ services/auth/sspi_windows.go | 10 ---------- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/routers/api/v1/auth_windows.go b/routers/api/v1/auth_windows.go index c6cd28440325f..d41c4bb223ff6 100644 --- a/routers/api/v1/auth_windows.go +++ b/routers/api/v1/auth_windows.go @@ -4,8 +4,15 @@ package v1 -import auth_service "code.gitea.io/gitea/services/auth" +import ( + "code.gitea.io/gitea/models/auth" + auth_service "code.gitea.io/gitea/services/auth" +) +// specialAdd registers the SSPI auth method as the last method in the list. +// The SSPI plugin is expected to be executed last, as it returns 401 status code if negotiation +// fails (or if negotiation should continue), which would prevent other authentication methods +// to execute at all. func specialAdd(group *auth_service.Group) { if auth.IsSSPIEnabled() { group.Add(&auth_service.SSPI{}) diff --git a/routers/web/auth_windows.go b/routers/web/auth_windows.go index dd5c8e39f465e..f404fd377160e 100644 --- a/routers/web/auth_windows.go +++ b/routers/web/auth_windows.go @@ -9,6 +9,10 @@ import ( auth_service "code.gitea.io/gitea/services/auth" ) +// specialAdd registers the SSPI auth method as the last method in the list. +// The SSPI plugin is expected to be executed last, as it returns 401 status code if negotiation +// fails (or if negotiation should continue), which would prevent other authentication methods +// to execute at all. func specialAdd(group *auth_service.Group) { if auth.IsSSPIEnabled() { group.Add(&auth_service.SSPI{}) diff --git a/services/auth/sspi_windows.go b/services/auth/sspi_windows.go index 3a8c8bed443ee..63e70e61d4335 100644 --- a/services/auth/sspi_windows.go +++ b/services/auth/sspi_windows.go @@ -244,13 +244,3 @@ func sanitizeUsername(username string, cfg *sspi.Source) string { username = replaceSeparators(username, cfg) return username } - -// specialInit registers the SSPI auth method as the last method in the list. -// The SSPI plugin is expected to be executed last, as it returns 401 status code if negotiation -// fails (or if negotiation should continue), which would prevent other authentication methods -// to execute at all. -func specialInit() { - if auth.IsSSPIEnabled() { - Register(&SSPI{}) - } -} From 9900b1d5301e652f5b8d2ed1e8cc38c72294fd86 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 25 Mar 2022 09:11:32 +0800 Subject: [PATCH 5/5] some improvements --- routers/api/v1/api.go | 11 ++++++++--- routers/web/web.go | 4 ++-- services/auth/auth.go | 2 +- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 963e380857fd2..405658d5bb7e2 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -569,8 +569,8 @@ func bind(obj interface{}) http.HandlerFunc { func buildAuthGroup() *auth.Group { group := auth.NewGroup( &auth.OAuth2{}, - &auth.Basic{}, - auth.SharedSession, + &auth.Basic{}, // FIXME: this should be removed once we don't allow basic auth in API + auth.SharedSession, // FIXME: this should be removed once all UI don't reference API/v1, see https://github.com/go-gitea/gitea/pull/16052 ) if setting.Service.EnableReverseProxyAuth { group.Add(&auth.ReverseProxy{}) @@ -600,8 +600,13 @@ func Routes(sessioner func(http.Handler) http.Handler) *web.Route { } m.Use(context.APIContexter()) + group := buildAuthGroup() + if err := group.Init(); err != nil { + log.Error("Could not initialize '%s' auth method, error: %s", group.Name(), err) + } + // Get user from session if logged in. - m.Use(context.APIAuth(buildAuthGroup())) + m.Use(context.APIAuth(group)) m.Use(context.ToggleAPI(&context.ToggleOptions{ SignInRequired: setting.Service.RequireSignInView, diff --git a/routers/web/web.go b/routers/web/web.go index ebcd7cb7345e4..37cb20e6c0b15 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -79,8 +79,8 @@ func CorsHandler() func(next http.Handler) http.Handler { // for users that have already signed in. func buildAuthGroup() *auth_service.Group { group := auth_service.NewGroup( - &auth_service.OAuth2{}, - &auth_service.Basic{}, + &auth_service.OAuth2{}, // FIXME: this should be removed and only applied in download and oauth realted routers + &auth_service.Basic{}, // FIXME: this should be removed and only applied in download and git/lfs routers auth_service.SharedSession, ) if setting.Service.EnableReverseProxyAuth { diff --git a/services/auth/auth.go b/services/auth/auth.go index 6d6edef14704c..a379cb101315d 100644 --- a/services/auth/auth.go +++ b/services/auth/auth.go @@ -26,7 +26,7 @@ var ( _ = handleSignIn // SharedSession the session auth should only be used by web, but now both web and API/v1 - // will use it. We can remvoe this after Web removed dependent API/v1 + // will use it. We can remove this after Web removed dependent API/v1 SharedSession = &Session{} )