diff --git a/pkg/api/admin_users.go b/pkg/api/admin_users.go index 7daf81be95c..525e42c9c78 100644 --- a/pkg/api/admin_users.go +++ b/pkg/api/admin_users.go @@ -5,6 +5,7 @@ import ( "fmt" "net/http" "strconv" + "strings" "golang.org/x/sync/errgroup" @@ -40,6 +41,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, diff --git a/pkg/api/org_invite.go b/pkg/api/org_invite.go index 53ff2490e8d..0f4eecc4522 100644 --- a/pkg/api/org_invite.go +++ b/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{ diff --git a/pkg/api/signup.go b/pkg/api/signup.go index 37b7ca407e5..c1cd5b5bd57 100644 --- a/pkg/api/signup.go +++ b/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, diff --git a/pkg/api/user.go b/pkg/api/user.go index fbce4f3a51b..15c6fa586a4 100644 --- a/pkg/api/user.go +++ b/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" @@ -118,9 +119,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) @@ -148,13 +154,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) } @@ -183,6 +194,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 { @@ -200,6 +221,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. diff --git a/pkg/api/user_test.go b/pkg/api/user_test.go index 0998a71687c..cc3e8946e5e 100644 --- a/pkg/api/user_test.go +++ b/pkg/api/user_test.go @@ -13,6 +13,8 @@ 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/db" "github.com/grafana/grafana/pkg/infra/usagestats" @@ -20,6 +22,7 @@ import ( 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" @@ -196,3 +199,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 := db.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 := db.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) + }) +} diff --git a/pkg/api/utils.go b/pkg/api/utils.go index d673b03af1b..716e8b98b60 100644 --- a/pkg/api/utils.go +++ b/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 +} diff --git a/pkg/services/login/logintest/logintest.go b/pkg/services/login/logintest/logintest.go index d4a9e37c3c6..5c2ce4005df 100644 --- a/pkg/services/login/logintest/logintest.go +++ b/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 } diff --git a/public/app/core/components/Signup/SignupPage.tsx b/public/app/core/components/Signup/SignupPage.tsx index 3d6a6188475..c757bb753fe 100644 --- a/public/app/core/components/Signup/SignupPage.tsx +++ b/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) => { {...register('email', { required: 'Email is required', pattern: { - value: /^\S+@\S+$/, + value: w3cStandardEmailValidator, message: 'Email is invalid', }, })} diff --git a/public/app/core/components/Signup/VerifyEmail.tsx b/public/app/core/components/Signup/VerifyEmail.tsx index cbb2b7fc7f3..e372217e06c 100644 --- a/public/app/core/components/Signup/VerifyEmail.tsx +++ b/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 = () => { {...register('email', { required: 'Email is required', pattern: { - value: /^\S+@\S+$/, + value: w3cStandardEmailValidator, message: 'Email is invalid', }, })} diff --git a/public/app/features/admin/UserProfile.tsx b/public/app/features/admin/UserProfile.tsx index 11e5d5e6e63..13ac137ae42 100644 --- a/public/app/features/admin/UserProfile.tsx +++ b/public/app/features/admin/UserProfile.tsx @@ -220,7 +220,9 @@ export class UserProfileRow extends PureComponent, status?: LegacyInputStatus) => { @@ -228,7 +230,9 @@ export class UserProfileRow extends PureComponent { diff --git a/public/app/features/admin/utils.ts b/public/app/features/admin/utils.ts index f4bdf531730..246f1279433 100644 --- a/public/app/features/admin/utils.ts +++ b/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); diff --git a/public/app/features/invites/SignupInvited.tsx b/public/app/features/invites/SignupInvited.tsx index 446d15a217a..7435932e347 100644 --- a/public/app/features/invites/SignupInvited.tsx +++ b/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 = ({ match }) => { {...register('email', { required: 'Email is required', pattern: { - value: /^\S+@\S+$/, + value: w3cStandardEmailValidator, message: 'Email is invalid', }, })}