Merge pull request #1048 from gotify/fix-create-user-permission

Fix create user permission

Author: jmattheis

api/user.go

@@ -153,7 +153,7 @@ func (a *UserAPI) GetCurrentUser(ctx *gin.Context) {
 // Create a user.
 //
 // With enabled registration: non admin users can be created without authentication.
-// With disabled registrations: users can only be created by admin users.
+// With disabled registrations: users can only be created by admin users with an elevated session.
 //
 // Requires elevated authentication.
 //
@@ -202,36 +202,25 @@ func (a *UserAPI) CreateUser(ctx *gin.Context) {
 			Admin: user.Admin,
 			Pass:  pw,
 		}
-		existingUser, err := a.DB.GetUserByName(internal.Name)
-		if success := successOrAbort(ctx, 500, err); !success {
-			return
-		}
-
-		var requestedBy *model.User
-		uid := auth.TryGetUserID(ctx)
-		if uid != nil {
-			requestedBy, err = a.DB.GetUserByID(*uid)
-			if err != nil {
-				ctx.AbortWithError(http.StatusInternalServerError, fmt.Errorf("could not get user: %s", err))
-				return
-			}
-		}
 
-		if requestedBy == nil || !requestedBy.Admin {
-			status := http.StatusUnauthorized
-			if requestedBy != nil {
-				status = http.StatusForbidden
-			}
+		// The auth middleware guarantees authenticated requests to be elevated admins.
+		// Only unauthenticated requests are limited to the registration checks.
+		if auth.TryGetUserID(ctx) == nil {
 			if !a.Registration {
-				ctx.AbortWithError(status, errors.New("you are not allowed to access this api"))
+				ctx.AbortWithError(http.StatusUnauthorized, errors.New("you are not allowed to access this api"))
 				return
 			}
 			if internal.Admin {
-				ctx.AbortWithError(status, errors.New("you are not allowed to create an admin user"))
+				ctx.AbortWithError(http.StatusUnauthorized, errors.New("you are not allowed to create an admin user"))
 				return
 			}
 		}
 
+		existingUser, err := a.DB.GetUserByName(internal.Name)
+		if success := successOrAbort(ctx, 500, err); !success {
+			return
+		}
+
 		if existingUser == nil {
 			if success := successOrAbort(ctx, 500, a.DB.CreateUser(internal)); !success {
 				return

api/user_test.go

@@ -187,45 +187,6 @@ func (s *UserSuite) Test_CreateUser() {
 	assert.True(s.T(), s.notifiedAdd)
 }
 
-func (s *UserSuite) Test_CreateUser_ByNonAdmin() {
-	s.loginUser()
-
-	s.ctx.Request = httptest.NewRequest("POST", "/user", strings.NewReader(`{"name": "tom", "pass": "1", "admin": false}`))
-	s.ctx.Request.Header.Set("Content-Type", "application/json")
-
-	s.a.CreateUser(s.ctx)
-
-	assert.Equal(s.T(), 403, s.recorder.Code)
-}
-
-func (s *UserSuite) Test_CreateUser_Register_ByNonAdmin() {
-	s.loginUser()
-	s.a.Registration = true
-
-	s.ctx.Request = httptest.NewRequest("POST", "/user", strings.NewReader(`{"name": "tom", "pass": "1", "admin": false}`))
-	s.ctx.Request.Header.Set("Content-Type", "application/json")
-
-	s.a.CreateUser(s.ctx)
-
-	assert.Equal(s.T(), 200, s.recorder.Code)
-	if created, err := s.db.GetUserByName("tom"); assert.NoError(s.T(), err) {
-		assert.NotNil(s.T(), created)
-	}
-}
-
-func (s *UserSuite) Test_CreateUser_Register_Admin_ByNonAdmin() {
-	s.a.Registration = true
-	s.loginUser()
-
-	s.ctx.Request = httptest.NewRequest("POST", "/user", strings.NewReader(`{"name": "tom", "pass": "1", "admin": true}`))
-	s.ctx.Request.Header.Set("Content-Type", "application/json")
-
-	s.a.CreateUser(s.ctx)
-
-	assert.Equal(s.T(), 403, s.recorder.Code)
-	s.db.AssertUsernameNotExist("tom")
-}
-
 func (s *UserSuite) Test_CreateUser_Anonymous() {
 	s.noLogin()
 
@@ -510,11 +471,6 @@ func (s *UserSuite) loginAdmin() {
 	auth.RegisterUser(s.ctx, &model.User{ID: 1, Admin: true})
 }
 
-func (s *UserSuite) loginUser() {
-	s.db.CreateUser(&model.User{ID: 1, Name: "user", Admin: false})
-	auth.RegisterUser(s.ctx, &model.User{ID: 1})
-}
-
 func (s *UserSuite) noLogin() {
 	// No authentication is registered
 }

auth/authentication.go

@@ -9,6 +9,7 @@ import (
 	"github.com/gin-gonic/gin"
 	"github.com/gotify/server/v3/auth/password"
 	"github.com/gotify/server/v3/model"
+	"github.com/rs/zerolog/log"
 )
 
 type authState int
@@ -48,7 +49,22 @@ type Auth struct {
 
 // RequireAdmin requires an elevated client token or basic auth, the user must be an admin.
 func (a *Auth) RequireAdmin(ctx *gin.Context) {
-	a.evaluateOr401(ctx, a.handleUser(a.checkUserAdmin), a.handleClient(a.checkClientAdmin, a.checkClientElevated))
+	a.evaluateOr401(ctx, a.adminHandlers()...)
+}
+
+// OptionalAdmin allows optional authentication. When authentication is present
+// an elevated client token or basic auth of an admin user must be provided.
+func (a *Auth) OptionalAdmin(ctx *gin.Context) {
+	if !a.evaluate(ctx, a.adminHandlers()...) {
+		ctx.Next()
+	}
+}
+
+func (a *Auth) adminHandlers() []func(ctx *gin.Context) (authState, error) {
+	return []func(ctx *gin.Context) (authState, error){
+		a.handleUser(a.checkUserAdmin),
+		a.handleClient(a.checkClientAdmin, a.checkClientElevated),
+	}
 }
 
 // RequireClient returns a gin middleware which requires a client token or basic authentication header to be supplied
@@ -84,12 +100,6 @@ func (a *Auth) RequireApplicationOrClient(ctx *gin.Context) {
 	a.evaluateOr401(ctx, a.handleApplication, a.handleClient(), a.handleUser())
 }
 
-func (a *Auth) Optional(ctx *gin.Context) {
-	if !a.evaluate(ctx, a.handleUser(), a.handleClient()) {
-		ctx.Next()
-	}
-}
-
 func (a *Auth) evaluate(ctx *gin.Context, funcs ...func(ctx *gin.Context) (authState, error)) bool {
 	if a.rejectForeignOrigin(ctx) {
 		return true
@@ -298,6 +308,9 @@ func (a *Auth) tokenFromAuthorizationHeader(ctx *gin.Context) string {
 func (a *Auth) checkClientAdmin(client *model.Client) (authState, error) {
 	if user, err := a.DB.GetUserByID(client.UserID); err != nil {
 		return authStateSkip, err
+	} else if user == nil {
+		log.Warn().Uint("client_id", client.ID).Uint("user_id", client.UserID).Msg("User for authenticated client doesn't exist")
+		return authStateForbidden, nil
 	} else if !user.Admin {
 		return authStateForbidden, nil
 	}

auth/authentication_test.go

@@ -63,6 +63,9 @@ func (s *AuthenticationSuite) SetupSuite() {
 			{Token: "clienttoken_admin_elevated", Name: "elevated phone2", ElevatedUntil: &elevated},
 		},
 	})
+
+	// client whose user does not exist anymore
+	s.DB.CreateClient(&model.Client{Token: "clienttoken_orphan_elevated", Name: "orphan phone", UserID: 999, ElevatedUntil: &elevated})
 }
 
 func (s *AuthenticationSuite) TearDownSuite() {
@@ -141,6 +144,11 @@ func (s *AuthenticationSuite) TestNothingProvided() {
 	assert.Equal(s.T(), 401, recorder.Code)
 }
 
+func (s *AuthenticationSuite) TestOrphanedClientToken() {
+	s.assertHeaderRequest("X-Gotify-Key", "clienttoken_orphan_elevated", s.auth.RequireAdmin, 403)
+	s.assertHeaderRequest("X-Gotify-Key", "clienttoken_orphan_elevated", s.auth.OptionalAdmin, 403)
+}
+
 func (s *AuthenticationSuite) TestHeaderApiKeyToken() {
 	// not existing token
 	s.assertHeaderRequest("X-Gotify-Key", "ergerogerg", s.auth.RequireApplicationToken, 401)
@@ -282,33 +290,47 @@ func (s *AuthenticationSuite) TestBasicAuthDisabled() {
 	s.assertHeaderRequest("Authorization", "Basic YWRtaW46cHc=", s.auth.RequireClient, 403)
 	s.assertHeaderRequest("Authorization", "Basic YWRtaW46cHc=", s.auth.RequireAdmin, 403)
 	s.assertHeaderRequest("Authorization", "Basic YWRtaW46cHc=", s.auth.RequireElevatedClient, 403)
+	s.assertHeaderRequest("Authorization", "Basic YWRtaW46cHc=", s.auth.OptionalAdmin, 403)
 }
 
-func (s *AuthenticationSuite) TestOptionalAuth() {
-	// various invalid users
-	ctx := s.assertQueryRequest("token", "ergerogerg", s.auth.Optional, 200)
-	assert.Nil(s.T(), TryGetUserID(ctx))
-	ctx = s.assertHeaderRequest("X-Gotify-Key", "ergerogerg", s.auth.Optional, 200)
+func (s *AuthenticationSuite) TestOptionalAdminAuth() {
+	ctx := s.assertQueryRequest("token", "ergerogerg", s.auth.OptionalAdmin, 200)
 	assert.Nil(s.T(), TryGetUserID(ctx))
-	ctx = s.assertHeaderRequest("Authorization", "Basic bm90ZXhpc3Rpbmc6cHc=", s.auth.Optional, 200)
+	ctx = s.assertQueryRequest("token", "apptoken_admin", s.auth.OptionalAdmin, 200)
 	assert.Nil(s.T(), TryGetUserID(ctx))
-	ctx = s.assertHeaderRequest("Authorization", "Basic YWRtaW46cHd4", s.auth.Optional, 200)
-	assert.Nil(s.T(), TryGetUserID(ctx))
-	ctx = s.assertQueryRequest("tokenx", "clienttoken", s.auth.Optional, 200)
-	assert.Nil(s.T(), TryGetUserID(ctx))
-	ctx = s.assertQueryRequest("token", "apptoken_admin", s.auth.Optional, 200)
+
+	// nothing provided
+	ctx = s.assertRequest(s.auth.OptionalAdmin, 200)
 	assert.Nil(s.T(), TryGetUserID(ctx))
 
-	// user existing:pw
-	ctx = s.assertHeaderRequest("Authorization", "Basic ZXhpc3Rpbmc6cHc=", s.auth.Optional, 200)
-	assert.Equal(s.T(), uint(1), *TryGetUserID(ctx))
-	ctx = s.assertQueryRequest("token", "clienttoken", s.auth.Optional, 200)
-	assert.Equal(s.T(), uint(1), *TryGetUserID(ctx))
+	// user existing:pw (non-admin)
+	s.assertHeaderRequest("Authorization", "Basic ZXhpc3Rpbmc6cHc=", s.auth.OptionalAdmin, 403)
 
-	// user admin:pw
-	ctx = s.assertHeaderRequest("Authorization", "Basic YWRtaW46cHc=", s.auth.Optional, 200)
+	// clienttoken (non-admin, not elevated)
+	s.assertQueryRequest("token", "clienttoken", s.auth.OptionalAdmin, 403)
+	s.assertHeaderRequest("X-Gotify-Key", "clienttoken", s.auth.OptionalAdmin, 403)
+	s.assertHeaderRequest("Authorization", "Bearer clienttoken", s.auth.OptionalAdmin, 403)
+
+	// clienttoken_elevated (non-admin, elevated)
+	s.assertQueryRequest("token", "clienttoken_elevated", s.auth.OptionalAdmin, 403)
+	s.assertHeaderRequest("X-Gotify-Key", "clienttoken_elevated", s.auth.OptionalAdmin, 403)
+	s.assertHeaderRequest("Authorization", "Bearer clienttoken_elevated", s.auth.OptionalAdmin, 403)
+
+	// user admin:pw (basic auth counts as elevated)
+	ctx = s.assertHeaderRequest("Authorization", "Basic YWRtaW46cHc=", s.auth.OptionalAdmin, 200)
 	assert.Equal(s.T(), uint(2), *TryGetUserID(ctx))
-	ctx = s.assertQueryRequest("token", "clienttoken_admin", s.auth.Optional, 200)
+
+	// clienttoken_admin (not elevated)
+	s.assertQueryRequest("token", "clienttoken_admin", s.auth.OptionalAdmin, 403)
+	s.assertHeaderRequest("X-Gotify-Key", "clienttoken_admin", s.auth.OptionalAdmin, 403)
+	s.assertHeaderRequest("Authorization", "Bearer clienttoken_admin", s.auth.OptionalAdmin, 403)
+
+	// clienttoken_admin_elevated
+	ctx = s.assertQueryRequest("token", "clienttoken_admin_elevated", s.auth.OptionalAdmin, 200)
+	assert.Equal(s.T(), uint(2), *TryGetUserID(ctx))
+	ctx = s.assertHeaderRequest("X-Gotify-Key", "clienttoken_admin_elevated", s.auth.OptionalAdmin, 200)
+	assert.Equal(s.T(), uint(2), *TryGetUserID(ctx))
+	ctx = s.assertHeaderRequest("Authorization", "Bearer clienttoken_admin_elevated", s.auth.OptionalAdmin, 200)
 	assert.Equal(s.T(), uint(2), *TryGetUserID(ctx))
 }
 
@@ -368,6 +390,15 @@ func (s *AuthenticationSuite) assertHeaderRequest(key, value string, f fMiddlewa
 	return ctx
 }
 
+func (s *AuthenticationSuite) assertRequest(f fMiddleware, code int) (ctx *gin.Context) {
+	recorder := httptest.NewRecorder()
+	ctx, _ = gin.CreateTestContext(recorder)
+	ctx.Request = httptest.NewRequest("GET", "/", nil)
+	f(ctx)
+	assert.Equal(s.T(), code, recorder.Code)
+	return ctx
+}
+
 func (s *AuthenticationSuite) TestCookieCrossOriginProtection() {
 	// httptest sets the request host to example.com.
 	s.assertCsrfRequest(map[string]string{"Origin": "http://example.com"}, "clienttoken", s.auth.RequireClient, 200)

docs/spec.json

@@ -2245,7 +2245,7 @@
             "basicAuth": []
           }
         ],
-        "description": "With enabled registration: non admin users can be created without authentication.\nWith disabled registrations: users can only be created by admin users.\n\nRequires elevated authentication.",
+        "description": "With enabled registration: non admin users can be created without authentication.\nWith disabled registrations: users can only be created by admin users with an elevated session.\n\nRequires elevated authentication.",
         "consumes": [
           "application/json"
         ],

router/router.go

@@ -158,7 +158,7 @@ func Create(db *database.GormDatabase, vInfo *model.VersionInfo, conf *config.Co
 		}
 	}
 
-	g.Group("/user").Use(authentication.Optional).POST("", userHandler.CreateUser)
+	g.Group("/user").Use(authentication.OptionalAdmin).POST("", userHandler.CreateUser)
 
 	g.POST("/auth/local/login", sessionHandler.Login)
 

router/router_test.go

@@ -407,6 +407,33 @@ func (s *IntegrationSuite) TestAuthentication() {
 	assert.Equal(s.T(), "android-client", token.Name)
 }
 
+func (s *IntegrationSuite) TestCreateUser_RequiresElevatedAdmin() {
+	s.db.AdminUser(2).ClientWithToken(1, "Cadminplain").ElevatedClientWithToken(2, "Cadminelevated")
+	s.db.User(3).ElevatedClientWithToken(3, "Cnormalelevated")
+
+	body := `{"name": "newadmin", "pass": "secret", "admin": true}`
+
+	// admin, but not elevated
+	req := s.newRequest("POST", "user", body)
+	req.Header.Set("X-Gotify-Key", "Cadminplain")
+	doRequestAndExpect(s.T(), req, 403, `{"error":"Forbidden", "errorCode":403, "errorDescription":"session not elevated, use basic auth or call /client:elevate"}`)
+	s.db.AssertUsernameNotExist("newadmin")
+
+	// elevated, but not admin
+	req = s.newRequest("POST", "user", body)
+	req.Header.Set("X-Gotify-Key", "Cnormalelevated")
+	doRequestAndExpect(s.T(), req, 403, forbiddenJSON)
+	s.db.AssertUsernameNotExist("newadmin")
+
+	// elevated admin
+	req = s.newRequest("POST", "user", body)
+	req.Header.Set("X-Gotify-Key", "Cadminelevated")
+	doRequestAndExpect(s.T(), req, 200, `{"id": 4, "name": "newadmin", "admin": true, "createdAt":"2020-01-01T00:00:00Z"}`)
+	created, err := s.db.GetUserByName("newadmin")
+	assert.NoError(s.T(), err)
+	assert.NotNil(s.T(), created)
+}
+
 func (s *IntegrationSuite) newRequest(method, url, body string) *http.Request {
 	req, err := http.NewRequest(method, fmt.Sprintf("%s/%s", s.server.URL, url), strings.NewReader(body))
 	req.Header.Add("Content-Type", "application/json")

test/testdb/database.go

@@ -54,6 +54,12 @@ func (d *Database) User(id uint) *AppClientBuilder {
 	return &AppClientBuilder{db: d, userID: id}
 }
 
+// AdminUser creates an admin user and returns a builder for applications and clients.
+func (d *Database) AdminUser(id uint) *AppClientBuilder {
+	d.CreateUser(&model.User{ID: id, Name: "user" + fmt.Sprint(id), Admin: true})
+	return &AppClientBuilder{db: d, userID: id}
+}
+
 // NewUser creates a user and returns the user.
 func (d *Database) NewUser(id uint) *model.User {
 	return d.NewUserWithName(id, "user"+fmt.Sprint(id))
@@ -173,6 +179,14 @@ func (ab *AppClientBuilder) NewClientWithToken(id uint, token string) *model.Cli
 	return client
 }
 
+// ElevatedClientWithToken creates an elevated client.
+func (ab *AppClientBuilder) ElevatedClientWithToken(id uint, token string) *AppClientBuilder {
+	future := time.Date(3000, time.January, 1, 0, 0, 0, 0, time.UTC)
+	client := &model.Client{ID: id, Token: token, UserID: ab.userID, ElevatedUntil: &future}
+	ab.db.CreateClient(client)
+	return ab
+}
+
 // Message creates a message and returns itself.
 func (mb *MessageBuilder) Message(id uint) *MessageBuilder {
 	mb.NewMessage(id)