Security: Omit error from http response when user does not exists (#639)

* Trim leading and trailing whitespaces from email and username on signup

* Check whether the provided email address is the same as where the invitation sent

Co-authored-by: Mihaly Gyongyosi <mgyongyosi@users.noreply.github.com>

pkg/api/admin_users.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"net/http"
 	"strconv"
+	"strings"
 
 	"github.com/grafana/grafana/pkg/api/dtos"
 	"github.com/grafana/grafana/pkg/api/response"
@@ -37,6 +38,10 @@ func (hs *HTTPServer) AdminCreateUser(c *models.ReqContext) response.Response {
 	if err := web.Bind(c.Req, &form); err != nil {
 		return response.Error(http.StatusBadRequest, "bad request data", err)
 	}
+
+	form.Email = strings.TrimSpace(form.Email)
+	form.Login = strings.TrimSpace(form.Login)
+
 	cmd := user.CreateUserCommand{
 		Login:    form.Login,
 		Email:    form.Email,

pkg/api/org_invite.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"net/http"
 	"strconv"
+	"strings"
 
 	"github.com/grafana/grafana/pkg/api/dtos"
 	"github.com/grafana/grafana/pkg/api/response"
@@ -217,21 +218,37 @@ func (hs *HTTPServer) GetInviteInfoByCode(c *models.ReqContext) response.Respons
 
 func (hs *HTTPServer) CompleteInvite(c *models.ReqContext) response.Response {
 	completeInvite := dtos.CompleteInviteForm{}
-	if err := web.Bind(c.Req, &completeInvite); err != nil {
+	var err error
+	if err = web.Bind(c.Req, &completeInvite); err != nil {
 		return response.Error(http.StatusBadRequest, "bad request data", err)
 	}
-	query := models.GetTempUserByCodeQuery{Code: completeInvite.InviteCode}
 
+	completeInvite.Email, err = ValidateAndNormalizeEmail(completeInvite.Email)
+	if err != nil {
+		return response.Error(http.StatusBadRequest, "Invalid email address provided", nil)
+	}
+
+	completeInvite.Username = strings.TrimSpace(completeInvite.Username)
+
+	query := models.GetTempUserByCodeQuery{Code: completeInvite.InviteCode}
 	if err := hs.tempUserService.GetTempUserByCode(c.Req.Context(), &query); err != nil {
 		if errors.Is(err, models.ErrTempUserNotFound) {
-			return response.Error(404, "Invite not found", nil)
+			return response.Error(http.StatusNotFound, "Invite not found", nil)
 		}
-		return response.Error(500, "Failed to get invite", err)
+		return response.Error(http.StatusInternalServerError, "Failed to get invite", err)
 	}
 
 	invite := query.Result
 	if invite.Status != models.TmpUserInvitePending {
-		return response.Error(412, fmt.Sprintf("Invite cannot be used in status %s", invite.Status), nil)
+		return response.Error(http.StatusPreconditionFailed, fmt.Sprintf("Invite cannot be used in status %s", invite.Status), nil)
+	}
+
+	// In case the user is invited by email address
+	if inviteMail, err := ValidateAndNormalizeEmail(invite.Email); err == nil {
+		// Make sure that the email address is not amended
+		if completeInvite.Email != inviteMail {
+			return response.Error(http.StatusBadRequest, "The provided email is different from the address that is found in the invite", nil)
+		}
 	}
 
 	cmd := user.CreateUserCommand{

pkg/api/password.go

@@ -28,8 +28,8 @@ func (hs *HTTPServer) SendResetPasswordEmail(c *models.ReqContext) response.Resp
 
 	usr, err := hs.userService.GetByLogin(c.Req.Context(), &userQuery)
 	if err != nil {
-		c.Logger.Info("Requested password reset for user that was not found", "user", userQuery.LoginOrEmail)
-		return response.Error(http.StatusOK, "Email sent", err)
+		c.Logger.Info("Requested password reset for user that was not found", "user", userQuery.LoginOrEmail, "error", err)
+		return response.Error(http.StatusOK, "Email sent", nil)
 	}
 
 	if usr.IsDisabled {

pkg/api/signup.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"net/http"
+	"strings"
 
 	"github.com/grafana/grafana/pkg/api/dtos"
 	"github.com/grafana/grafana/pkg/api/response"
@@ -27,15 +28,21 @@ func GetSignUpOptions(c *models.ReqContext) response.Response {
 // POST /api/user/signup
 func (hs *HTTPServer) SignUp(c *models.ReqContext) response.Response {
 	form := dtos.SignUpForm{}
-	if err := web.Bind(c.Req, &form); err != nil {
+	var err error
+	if err = web.Bind(c.Req, &form); err != nil {
 		return response.Error(http.StatusBadRequest, "bad request data", err)
 	}
 	if !setting.AllowUserSignUp {
 		return response.Error(401, "User signup is disabled", nil)
 	}
 
+	form.Email, err = ValidateAndNormalizeEmail(form.Email)
+	if err != nil {
+		return response.Error(http.StatusBadRequest, "Invalid email address", nil)
+	}
+
 	existing := user.GetUserByLoginQuery{LoginOrEmail: form.Email}
-	_, err := hs.userService.GetByLogin(c.Req.Context(), &existing)
+	_, err = hs.userService.GetByLogin(c.Req.Context(), &existing)
 	if err == nil {
 		return response.Error(422, "User with same email address already exists", nil)
 	}
@@ -76,6 +83,9 @@ func (hs *HTTPServer) SignUpStep2(c *models.ReqContext) response.Response {
 		return response.Error(401, "User signup is disabled", nil)
 	}
 
+	form.Email = strings.TrimSpace(form.Email)
+	form.Username = strings.TrimSpace(form.Username)
+
 	createUserCmd := user.CreateUserCommand{
 		Email:    form.Email,
 		Login:    form.Username,

pkg/api/user.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"net/http"
 	"strconv"
+	"strings"
 
 	"github.com/grafana/grafana/pkg/api/dtos"
 	"github.com/grafana/grafana/pkg/api/response"
@@ -116,9 +117,14 @@ func (hs *HTTPServer) GetUserByLoginOrEmail(c *models.ReqContext) response.Respo
 // 500: internalServerError
 func (hs *HTTPServer) UpdateSignedInUser(c *models.ReqContext) response.Response {
 	cmd := user.UpdateUserCommand{}
-	if err := web.Bind(c.Req, &cmd); err != nil {
+	var err error
+	if err = web.Bind(c.Req, &cmd); err != nil {
 		return response.Error(http.StatusBadRequest, "bad request data", err)
 	}
+
+	cmd.Email = strings.TrimSpace(cmd.Email)
+	cmd.Login = strings.TrimSpace(cmd.Login)
+
 	if setting.AuthProxyEnabled {
 		if setting.AuthProxyHeaderProperty == "email" && cmd.Email != c.Email {
 			return response.Error(400, "Not allowed to change email when auth proxy is using email property", nil)
@@ -146,13 +152,18 @@ func (hs *HTTPServer) UpdateSignedInUser(c *models.ReqContext) response.Response
 func (hs *HTTPServer) UpdateUser(c *models.ReqContext) response.Response {
 	cmd := user.UpdateUserCommand{}
 	var err error
-	if err := web.Bind(c.Req, &cmd); err != nil {
+	if err = web.Bind(c.Req, &cmd); err != nil {
 		return response.Error(http.StatusBadRequest, "bad request data", err)
 	}
+
+	cmd.Email = strings.TrimSpace(cmd.Email)
+	cmd.Login = strings.TrimSpace(cmd.Login)
+
 	cmd.UserID, err = strconv.ParseInt(web.Params(c.Req)[":id"], 10, 64)
 	if err != nil {
 		return response.Error(http.StatusBadRequest, "id is invalid", err)
 	}
+
 	return hs.handleUpdateUser(c.Req.Context(), cmd)
 }
 
@@ -181,6 +192,16 @@ func (hs *HTTPServer) UpdateUserActiveOrg(c *models.ReqContext) response.Respons
 }
 
 func (hs *HTTPServer) handleUpdateUser(ctx context.Context, cmd user.UpdateUserCommand) response.Response {
+	// external user -> user data cannot be updated
+	isExternal, err := hs.isExternalUser(ctx, cmd.UserID)
+	if err != nil {
+		return response.Error(http.StatusInternalServerError, "Failed to validate User", err)
+	}
+
+	if isExternal {
+		return response.Error(http.StatusForbidden, "User info cannot be updated for external Users", nil)
+	}
+
 	if len(cmd.Login) == 0 {
 		cmd.Login = cmd.Email
 		if len(cmd.Login) == 0 {
@@ -198,6 +219,20 @@ func (hs *HTTPServer) handleUpdateUser(ctx context.Context, cmd user.UpdateUserC
 	return response.Success("User updated")
 }
 
+func (hs *HTTPServer) isExternalUser(ctx context.Context, userID int64) (bool, error) {
+	getAuthQuery := models.GetAuthInfoQuery{UserId: userID}
+	var err error
+	if err = hs.authInfoService.GetAuthInfo(ctx, &getAuthQuery); err == nil {
+		return true, nil
+	}
+
+	if errors.Is(err, user.ErrUserNotFound) {
+		return false, nil
+	}
+
+	return false, err
+}
+
 // swagger:route GET /user/orgs signed_in_user getSignedInUserOrgList
 //
 // Organizations of the actual User.

pkg/api/user_test.go

@@ -13,12 +13,15 @@ import (
 	"golang.org/x/oauth2"
 
 	"github.com/grafana/grafana/pkg/api/dtos"
+	"github.com/grafana/grafana/pkg/api/response"
+	"github.com/grafana/grafana/pkg/api/routing"
 	"github.com/grafana/grafana/pkg/components/simplejson"
 	"github.com/grafana/grafana/pkg/infra/usagestats"
 	"github.com/grafana/grafana/pkg/models"
 	acmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock"
 	"github.com/grafana/grafana/pkg/services/login/authinfoservice"
 	authinfostore "github.com/grafana/grafana/pkg/services/login/authinfoservice/database"
+	"github.com/grafana/grafana/pkg/services/login/logintest"
 	"github.com/grafana/grafana/pkg/services/searchusers"
 	"github.com/grafana/grafana/pkg/services/searchusers/filters"
 	"github.com/grafana/grafana/pkg/services/secrets/database"
@@ -194,3 +197,117 @@ func TestUserAPIEndpoint_userLoggedIn(t *testing.T) {
 		assert.Equal(t, 10, respJSON.Get("perPage").MustInt())
 	}, mock)
 }
+
+func TestHTTPServer_UpdateUser(t *testing.T) {
+	settings := setting.NewCfg()
+	sqlStore := sqlstore.InitTestDB(t)
+
+	hs := &HTTPServer{
+		Cfg:           settings,
+		SQLStore:      sqlStore,
+		AccessControl: acmock.New(),
+	}
+
+	updateUserCommand := user.UpdateUserCommand{
+		Email:  fmt.Sprint("admin", "@test.com"),
+		Name:   "admin",
+		Login:  "admin",
+		UserID: 1,
+	}
+
+	updateUserScenario(t, updateUserContext{
+		desc:         "Should return 403 when the current User is an external user",
+		url:          "/api/users/1",
+		routePattern: "/api/users/:id",
+		cmd:          updateUserCommand,
+		fn: func(sc *scenarioContext) {
+			sc.authInfoService.ExpectedUserAuth = &models.UserAuth{}
+			sc.fakeReqWithParams("PUT", sc.url, map[string]string{"id": "1"}).exec()
+			assert.Equal(t, 403, sc.resp.Code)
+		},
+	}, hs)
+}
+
+type updateUserContext struct {
+	desc         string
+	url          string
+	routePattern string
+	cmd          user.UpdateUserCommand
+	fn           scenarioFunc
+}
+
+func updateUserScenario(t *testing.T, ctx updateUserContext, hs *HTTPServer) {
+	t.Run(fmt.Sprintf("%s %s", ctx.desc, ctx.url), func(t *testing.T) {
+		sc := setupScenarioContext(t, ctx.url)
+
+		sc.authInfoService = &logintest.AuthInfoServiceFake{}
+		hs.authInfoService = sc.authInfoService
+
+		sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response {
+			c.Req.Body = mockRequestBody(ctx.cmd)
+			c.Req.Header.Add("Content-Type", "application/json")
+			sc.context = c
+			sc.context.OrgID = testOrgID
+			sc.context.UserID = testUserID
+
+			return hs.UpdateUser(c)
+		})
+
+		sc.m.Put(ctx.routePattern, sc.defaultHandler)
+
+		ctx.fn(sc)
+	})
+}
+
+func TestHTTPServer_UpdateSignedInUser(t *testing.T) {
+	settings := setting.NewCfg()
+	sqlStore := sqlstore.InitTestDB(t)
+
+	hs := &HTTPServer{
+		Cfg:           settings,
+		SQLStore:      sqlStore,
+		AccessControl: acmock.New(),
+	}
+
+	updateUserCommand := user.UpdateUserCommand{
+		Email:  fmt.Sprint("admin", "@test.com"),
+		Name:   "admin",
+		Login:  "admin",
+		UserID: 1,
+	}
+
+	updateSignedInUserScenario(t, updateUserContext{
+		desc:         "Should return 403 when the current User is an external user",
+		url:          "/api/users/",
+		routePattern: "/api/users/",
+		cmd:          updateUserCommand,
+		fn: func(sc *scenarioContext) {
+			sc.authInfoService.ExpectedUserAuth = &models.UserAuth{}
+			sc.fakeReqWithParams("PUT", sc.url, map[string]string{"id": "1"}).exec()
+			assert.Equal(t, 403, sc.resp.Code)
+		},
+	}, hs)
+}
+
+func updateSignedInUserScenario(t *testing.T, ctx updateUserContext, hs *HTTPServer) {
+	t.Run(fmt.Sprintf("%s %s", ctx.desc, ctx.url), func(t *testing.T) {
+		sc := setupScenarioContext(t, ctx.url)
+
+		sc.authInfoService = &logintest.AuthInfoServiceFake{}
+		hs.authInfoService = sc.authInfoService
+
+		sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response {
+			c.Req.Body = mockRequestBody(ctx.cmd)
+			c.Req.Header.Add("Content-Type", "application/json")
+			sc.context = c
+			sc.context.OrgID = testOrgID
+			sc.context.UserID = testUserID
+
+			return hs.UpdateSignedInUser(c)
+		})
+
+		sc.m.Put(ctx.routePattern, sc.defaultHandler)
+
+		ctx.fn(sc)
+	})
+}

pkg/api/utils.go

@@ -1,9 +1,25 @@
 package api
 
-import "encoding/json"
+import (
+	"encoding/json"
+	"net/mail"
+)
 
 func jsonMap(data []byte) (map[string]string, error) {
 	jsonMap := make(map[string]string)
 	err := json.Unmarshal(data, &jsonMap)
 	return jsonMap, err
 }
+
+func ValidateAndNormalizeEmail(email string) (string, error) {
+	if email == "" {
+		return "", nil
+	}
+
+	e, err := mail.ParseAddress(email)
+	if err != nil {
+		return "", err
+	}
+
+	return e.Address, nil
+}

pkg/services/login/logintest/logintest.go

@@ -23,6 +23,7 @@ func (l *LoginServiceFake) SetTeamSyncFunc(login.TeamSyncFunc) {}
 
 type AuthInfoServiceFake struct {
 	LatestUserID         int64
+	ExpectedUserAuth     *models.UserAuth
 	ExpectedUser         *user.User
 	ExpectedExternalUser *models.ExternalUserInfo
 	ExpectedError        error
@@ -39,6 +40,7 @@ func (a *AuthInfoServiceFake) LookupAndUpdate(ctx context.Context, query *models
 
 func (a *AuthInfoServiceFake) GetAuthInfo(ctx context.Context, query *models.GetAuthInfoQuery) error {
 	a.LatestUserID = query.UserId
+	query.Result = a.ExpectedUserAuth
 	return a.ExpectedError
 }
 

pkg/web/macaron.go

@@ -144,8 +144,14 @@ func mwFromHandler(handler Handler) Middleware {
 }
 
 func (m *Macaron) createContext(rw http.ResponseWriter, req *http.Request) *Context {
+	// NOTE: we have to explicitly copy the middleware chain here to avoid
+	// passing a shared slice to the *Context, which leads to racy behavior in
+	// case of later appends
+	mws := make([]Middleware, len(m.mws))
+	copy(mws, m.mws)
+
 	c := &Context{
-		mws:  m.mws,
+		mws:  mws,
 		Resp: NewResponseWriter(req.Method, rw),
 	}
 

public/app/core/components/Signup/SignupPage.tsx

@@ -5,6 +5,7 @@ import { Form, Field, Input, Button, HorizontalGroup, LinkButton, FormAPI } from
 import { getConfig } from 'app/core/config';
 import { useAppNotification } from 'app/core/copy/appNotification';
 import { GrafanaRouteComponentProps } from 'app/core/navigation/types';
+import { w3cStandardEmailValidator } from 'app/features/admin/utils';
 
 import { InnerBox, LoginLayout } from '../Login/LoginLayout';
 import { PasswordField } from '../PasswordField/PasswordField';
@@ -74,7 +75,7 @@ export const SignupPage: FC<Props> = (props) => {
                   {...register('email', {
                     required: 'Email is required',
                     pattern: {
-                      value: /^\S+@\S+$/,
+                      value: w3cStandardEmailValidator,
                       message: 'Email is invalid',
                     },
                   })}

public/app/core/components/Signup/VerifyEmail.tsx

@@ -4,6 +4,7 @@ import { getBackendSrv } from '@grafana/runtime';
 import { Form, Field, Input, Button, Legend, Container, HorizontalGroup, LinkButton } from '@grafana/ui';
 import { getConfig } from 'app/core/config';
 import { useAppNotification } from 'app/core/copy/appNotification';
+import { w3cStandardEmailValidator } from 'app/features/admin/utils';
 
 interface EmailDTO {
   email: string;
@@ -53,7 +54,7 @@ export const VerifyEmail: FC = () => {
               {...register('email', {
                 required: 'Email is required',
                 pattern: {
-                  value: /^\S+@\S+$/,
+                  value: w3cStandardEmailValidator,
                   message: 'Email is invalid',
                 },
               })}

public/app/features/admin/UserProfile.tsx

@@ -225,7 +225,9 @@ export class UserProfileRow extends PureComponent<UserProfileRowProps, UserProfi
       return;
     }
 
-    this.setState({ value: event.target.value });
+    this.setState({
+      value: event.target.value,
+    });
   };
 
   onInputBlur = (event: React.FocusEvent<HTMLInputElement>, status?: LegacyInputStatus) => {
@@ -233,7 +235,9 @@ export class UserProfileRow extends PureComponent<UserProfileRowProps, UserProfi
       return;
     }
 
-    this.setState({ value: event.target.value });
+    this.setState({
+      value: event.target.value,
+    });
   };
 
   focusInput = () => {

public/app/features/admin/utils.ts

@@ -1,5 +1,9 @@
 import { config } from '@grafana/runtime/src';
 
+// https://html.spec.whatwg.org/multipage/input.html#valid-e-mail-address
+export const w3cStandardEmailValidator =
+  /^[a-zA-Z0-9.!#$%&'*+\/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*$/;
+
 export function isTrial() {
   const expiry = config.licenseInfo?.trialExpiry;
   return !!(expiry && expiry > 0);

public/app/features/invites/SignupInvited.tsx

@@ -8,6 +8,8 @@ import { getConfig } from 'app/core/config';
 import { contextSrv } from 'app/core/core';
 import { GrafanaRouteComponentProps } from 'app/core/navigation/types';
 
+import { w3cStandardEmailValidator } from '../admin/utils';
+
 interface FormModel {
   email: string;
   name?: string;
@@ -77,7 +79,7 @@ export const SignupInvitedPage: FC<Props> = ({ match }) => {
                   {...register('email', {
                     required: 'Email is required',
                     pattern: {
-                      value: /^\S+@\S+$/,
+                      value: w3cStandardEmailValidator,
                       message: 'Email is invalid',
                     },
                   })}