From ffb3f41798215679cb4c7570342a79d1fffbc3a6 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 6 Apr 2021 10:58:34 +0100 Subject: [PATCH 1/5] Create a session on ReverseProxy and ensure that ReverseProxy users cannot change username ReverseProxy users should generate a session on reverse proxy username change. Also prevent ReverseProxy users from changing their username. Fix #2407 Signed-off-by: Andrew Thornton --- modules/auth/sso/reverseproxy.go | 20 +++++++++++++++----- modules/auth/sso/sso.go | 17 ++++++++++++++++- modules/context/context.go | 3 +++ templates/user/settings/profile.tmpl | 4 ++-- 4 files changed, 36 insertions(+), 8 deletions(-) diff --git a/modules/auth/sso/reverseproxy.go b/modules/auth/sso/reverseproxy.go index ca9450e71429d..6de14322fd343 100644 --- a/modules/auth/sso/reverseproxy.go +++ b/modules/auth/sso/reverseproxy.go @@ -12,6 +12,7 @@ import ( "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/web/middleware" gouuid "github.com/google/uuid" ) @@ -68,13 +69,22 @@ func (r *ReverseProxy) VerifyAuthData(req *http.Request, w http.ResponseWriter, user, err := models.GetUserByName(username) if err != nil { - if models.IsErrUserNotExist(err) && r.isAutoRegisterAllowed() { - return r.newUser(req) + if !models.IsErrUserNotExist(err) || r.isAutoRegisterAllowed() { + log.Error("GetUserByName: %v", err) + return nil } - log.Error("GetUserByName: %v", err) - return nil + user = r.newUser(req) } + // Make sure requests to API paths and PWA resources do not create a new session + if !middleware.IsAPIPath(req) && !isAttachmentDownload(req) && !isGitOrLFSPath(req) { + if sess.Get("uid").(int64) != user.ID { + handleSignIn(w, req, sess, user) + } + } + log.Info("Setting IsReverseProxy") + store.GetData()["IsReverseProxy"] = true + return user } @@ -102,7 +112,6 @@ func (r *ReverseProxy) newUser(req *http.Request) *models.User { user := &models.User{ Name: username, Email: email, - Passwd: username, IsActive: true, } if err := models.CreateUser(user); err != nil { @@ -110,5 +119,6 @@ func (r *ReverseProxy) newUser(req *http.Request) *models.User { log.Error("CreateUser: %v", err) return nil } + return user } diff --git a/modules/auth/sso/sso.go b/modules/auth/sso/sso.go index e670f1a8a7193..56e608895871b 100644 --- a/modules/auth/sso/sso.go +++ b/modules/auth/sso/sso.go @@ -9,10 +9,12 @@ import ( "fmt" "net/http" "reflect" + "regexp" "strings" "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/web/middleware" ) @@ -27,8 +29,8 @@ import ( // for users that have already signed in. var ssoMethods = []SingleSignOn{ &OAuth2{}, - &Session{}, &ReverseProxy{}, + &Session{}, &Basic{}, } @@ -98,6 +100,19 @@ func isAttachmentDownload(req *http.Request) bool { return strings.HasPrefix(req.URL.Path, "/attachments/") && req.Method == "GET" } +var gitPathRe = regexp.MustCompile(`^/[a-zA-Z0-9_.-]+/[a-zA-Z0-9_.-]+/(?:git-(?:(?:upload)|(?:receive))-pack$)|(?:info/refs$)|(?:HEAD$)|(?:objects/)`) +var lfsPathRe = regexp.MustCompile(`^/[a-zA-Z0-9_.-]+/[a-zA-Z0-9_.-]+/info/lfs/`) + +func isGitOrLFSPath(req *http.Request) bool { + if gitPathRe.MatchString(req.URL.Path) { + return true + } + if setting.LFS.StartServer { + return lfsPathRe.MatchString(req.URL.Path) + } + return false +} + // handleSignIn clears existing session variables and stores new ones for the specified user object func handleSignIn(resp http.ResponseWriter, req *http.Request, sess SessionStore, user *models.User) { _ = sess.Delete("openid_verified_uri") diff --git a/modules/context/context.go b/modules/context/context.go index b876487d5e004..78a59130ca062 100644 --- a/modules/context/context.go +++ b/modules/context/context.go @@ -683,6 +683,9 @@ func Contexter() func(next http.Handler) http.Handler { } else { ctx.Data["SignedUserID"] = int64(0) ctx.Data["SignedUserName"] = "" + + // ensure the session uid is deleted + _ = ctx.Session.Delete("uid") } ctx.Resp.Header().Set(`X-Frame-Options`, `SAMEORIGIN`) diff --git a/templates/user/settings/profile.tmpl b/templates/user/settings/profile.tmpl index ee3cc589041a8..9f07226632fcd 100644 --- a/templates/user/settings/profile.tmpl +++ b/templates/user/settings/profile.tmpl @@ -15,8 +15,8 @@ {{.i18n.Tr "settings.change_username_prompt"}} {{.i18n.Tr "settings.change_username_redirect_prompt"}} - - {{if not .SignedUser.IsLocal}} + + {{if or (not .SignedUser.IsLocal) .IsReverseProxy}}

{{$.i18n.Tr "settings.password_username_disabled"}}

{{end}} From 79d5c3a63fb40248963f7823ecf7e90c65de47c8 Mon Sep 17 00:00:00 2001 From: zeripath Date: Tue, 6 Apr 2021 20:45:08 +0100 Subject: [PATCH 2/5] Update modules/auth/sso/reverseproxy.go --- modules/auth/sso/reverseproxy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/auth/sso/reverseproxy.go b/modules/auth/sso/reverseproxy.go index 6de14322fd343..7af51fcd378a3 100644 --- a/modules/auth/sso/reverseproxy.go +++ b/modules/auth/sso/reverseproxy.go @@ -76,7 +76,7 @@ func (r *ReverseProxy) VerifyAuthData(req *http.Request, w http.ResponseWriter, user = r.newUser(req) } - // Make sure requests to API paths and PWA resources do not create a new session + // Make sure requests to API paths, attachment downloads, git and LFS do not create a new session if !middleware.IsAPIPath(req) && !isAttachmentDownload(req) && !isGitOrLFSPath(req) { if sess.Get("uid").(int64) != user.ID { handleSignIn(w, req, sess, user) From 936161708917c0f5235d22eb12d17144e0a6b89a Mon Sep 17 00:00:00 2001 From: zeripath Date: Sun, 11 Apr 2021 12:40:15 +0100 Subject: [PATCH 3/5] Update modules/auth/sso/reverseproxy.go Oops --- modules/auth/sso/reverseproxy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/auth/sso/reverseproxy.go b/modules/auth/sso/reverseproxy.go index 7af51fcd378a3..3eeea56736c06 100644 --- a/modules/auth/sso/reverseproxy.go +++ b/modules/auth/sso/reverseproxy.go @@ -69,7 +69,7 @@ func (r *ReverseProxy) VerifyAuthData(req *http.Request, w http.ResponseWriter, user, err := models.GetUserByName(username) if err != nil { - if !models.IsErrUserNotExist(err) || r.isAutoRegisterAllowed() { + if !models.IsErrUserNotExist(err) || !r.isAutoRegisterAllowed() { log.Error("GetUserByName: %v", err) return nil } From 7d8737ad6c7ed0ccf3139f16218dd5df28d73f42 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 4 May 2021 18:30:56 +0100 Subject: [PATCH 4/5] add testcase Signed-off-by: Andrew Thornton --- modules/auth/sso/sso.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/auth/sso/sso.go b/modules/auth/sso/sso.go index 56e608895871b..3abe5517d0f2d 100644 --- a/modules/auth/sso/sso.go +++ b/modules/auth/sso/sso.go @@ -100,7 +100,7 @@ func isAttachmentDownload(req *http.Request) bool { return strings.HasPrefix(req.URL.Path, "/attachments/") && req.Method == "GET" } -var gitPathRe = regexp.MustCompile(`^/[a-zA-Z0-9_.-]+/[a-zA-Z0-9_.-]+/(?:git-(?:(?:upload)|(?:receive))-pack$)|(?:info/refs$)|(?:HEAD$)|(?:objects/)`) +var gitPathRe = regexp.MustCompile(`^/[a-zA-Z0-9_.-]+/[a-zA-Z0-9_.-]+/(?:(?:git-(?:(?:upload)|(?:receive))-pack$)|(?:info/refs$)|(?:HEAD$)|(?:objects/))`) var lfsPathRe = regexp.MustCompile(`^/[a-zA-Z0-9_.-]+/[a-zA-Z0-9_.-]+/info/lfs/`) func isGitOrLFSPath(req *http.Request) bool { From 475226d407d27fcca1c15130f1cce78655139109 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Tue, 4 May 2021 18:45:57 +0100 Subject: [PATCH 5/5] add test Signed-off-by: Andrew Thornton --- modules/auth/sso/sso_test.go | 124 +++++++++++++++++++++++++++++++++++ 1 file changed, 124 insertions(+) create mode 100644 modules/auth/sso/sso_test.go diff --git a/modules/auth/sso/sso_test.go b/modules/auth/sso/sso_test.go new file mode 100644 index 0000000000000..b6a7f099e3a2f --- /dev/null +++ b/modules/auth/sso/sso_test.go @@ -0,0 +1,124 @@ +// Copyright 2014 The Gogs Authors. All rights reserved. +// Copyright 2019 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 sso + +import ( + "net/http" + "testing" + + "code.gitea.io/gitea/modules/setting" +) + +func Test_isGitOrLFSPath(t *testing.T) { + + tests := []struct { + path string + + want bool + }{ + { + "/owner/repo/git-upload-pack", + true, + }, + { + "/owner/repo/git-receive-pack", + true, + }, + { + "/owner/repo/info/refs", + true, + }, + { + "/owner/repo/HEAD", + true, + }, + { + "/owner/repo/objects/info/alternates", + true, + }, + { + "/owner/repo/objects/info/http-alternates", + true, + }, + { + "/owner/repo/objects/info/packs", + true, + }, + { + "/owner/repo/objects/info/blahahsdhsdkla", + true, + }, + { + "/owner/repo/objects/01/23456789abcdef0123456789abcdef01234567", + true, + }, + { + "/owner/repo/objects/pack/pack-123456789012345678921234567893124567894.pack", + true, + }, + { + "/owner/repo/objects/pack/pack-0123456789abcdef0123456789abcdef0123456.idx", + true, + }, + { + "/owner/repo/stars", + false, + }, + { + "/notowner", + false, + }, + { + "/owner/repo", + false, + }, + { + "/owner/repo/commit/123456789012345678921234567893124567894", + false, + }, + } + lfsTests := []string{ + "/owner/repo/info/lfs/", + "/owner/repo/info/lfs/objects/batch", + "/owner/repo/info/lfs/objects/oid/filename", + "/owner/repo/info/lfs/objects/oid", + "/owner/repo/info/lfs/objects", + "/owner/repo/info/lfs/verify", + "/owner/repo/info/lfs/locks", + "/owner/repo/info/lfs/locks/verify", + "/owner/repo/info/lfs/locks/123/unlock", + } + + origLFSStartServer := setting.LFS.StartServer + + for _, tt := range tests { + t.Run(tt.path, func(t *testing.T) { + req, _ := http.NewRequest("POST", "http://localhost"+tt.path, nil) + setting.LFS.StartServer = false + if got := isGitOrLFSPath(req); got != tt.want { + t.Errorf("isGitOrLFSPath() = %v, want %v", got, tt.want) + } + setting.LFS.StartServer = true + if got := isGitOrLFSPath(req); got != tt.want { + t.Errorf("isGitOrLFSPath() = %v, want %v", got, tt.want) + } + }) + } + for _, tt := range lfsTests { + t.Run(tt, func(t *testing.T) { + req, _ := http.NewRequest("POST", tt, nil) + setting.LFS.StartServer = false + if got := isGitOrLFSPath(req); got != setting.LFS.StartServer { + t.Errorf("isGitOrLFSPath(%q) = %v, want %v, %v", tt, got, setting.LFS.StartServer, gitPathRe.MatchString(tt)) + } + setting.LFS.StartServer = true + if got := isGitOrLFSPath(req); got != setting.LFS.StartServer { + t.Errorf("isGitOrLFSPath(%q) = %v, want %v", tt, got, setting.LFS.StartServer) + } + }) + } + setting.LFS.StartServer = origLFSStartServer +}