From bf66812e227ddc7b233a219c34eb78ffea568c2f Mon Sep 17 00:00:00 2001 From: Caio Teixeira Date: Mon, 8 Jan 2024 14:33:19 -0300 Subject: [PATCH] cmd/httphandler: log user activity when updating user info (#139) What Log the user activity when updating users' info (updating roles, creating users through CLI). Why Security review. --- internal/serve/httphandler/user_handler.go | 30 +++- .../serve/httphandler/user_handler_test.go | 145 ++++++++++++++---- stellar-auth/pkg/cli/add_user.go | 3 +- 3 files changed, 144 insertions(+), 34 deletions(-) diff --git a/internal/serve/httphandler/user_handler.go b/internal/serve/httphandler/user_handler.go index 36bc14b79..5cb61624b 100644 --- a/internal/serve/httphandler/user_handler.go +++ b/internal/serve/httphandler/user_handler.go @@ -149,10 +149,10 @@ func (h UserHandler) UserActivation(rw http.ResponseWriter, req *http.Request) { var activationErr error if *reqBody.IsActive { - log.Ctx(ctx).Infof("[ActivateUserAccount] - Activating user with account ID %s", reqBody.UserID) + log.Ctx(ctx).Infof("[ActivateUserAccount] - User ID %s activating user with account ID %s", userID, reqBody.UserID) activationErr = h.AuthManager.ActivateUser(ctx, token, reqBody.UserID) } else { - log.Ctx(ctx).Infof("[DeactivateUserAccount] - Deactivating user with account ID %s", reqBody.UserID) + log.Ctx(ctx).Infof("[DeactivateUserAccount] - User ID %s deactivating user with account ID %s", userID, reqBody.UserID) activationErr = h.AuthManager.DeactivateUser(ctx, token, reqBody.UserID) } @@ -173,6 +173,13 @@ func (h UserHandler) UserActivation(rw http.ResponseWriter, req *http.Request) { func (h UserHandler) CreateUser(rw http.ResponseWriter, req *http.Request) { ctx := req.Context() + token, ok := ctx.Value(middleware.TokenContextKey).(string) + if !ok { + log.Ctx(ctx).Warn("token not found when updating user activation") + httperror.Unauthorized("", nil, nil).Render(rw) + return + } + var reqBody CreateUserRequest if err := httpdecode.DecodeJSON(req, &reqBody); err != nil { err = fmt.Errorf("decoding the request body: %w", err) @@ -186,6 +193,14 @@ func (h UserHandler) CreateUser(rw http.ResponseWriter, req *http.Request) { return } + authenticatedUserID, err := h.AuthManager.GetUserID(ctx, token) + if err != nil { + err = fmt.Errorf("getting request authenticated user ID: %w", err) + log.Ctx(ctx).Error(err) + httperror.Unauthorized("", err, nil).Render(rw) + return + } + newUser := &auth.User{ FirstName: reqBody.FirstName, LastName: reqBody.LastName, @@ -241,7 +256,7 @@ func (h UserHandler) CreateUser(rw http.ResponseWriter, req *http.Request) { return } - log.Ctx(ctx).Infof("[CreateUserAccount] - Created user with account ID %s", u.ID) + log.Ctx(ctx).Infof("[CreateUserAccount] - User ID %s created user with account ID %s", authenticatedUserID, u.ID) httpjson.RenderStatus(rw, http.StatusCreated, u, httpjson.JSON) } @@ -268,6 +283,14 @@ func (h UserHandler) UpdateUserRoles(rw http.ResponseWriter, req *http.Request) return } + authenticatedUserID, err := h.AuthManager.GetUserID(ctx, token) + if err != nil { + err = fmt.Errorf("getting request authenticated user ID: %w", err) + log.Ctx(ctx).Error(err) + httperror.Unauthorized("", err, nil).Render(rw) + return + } + updateUserRolesErr := h.AuthManager.UpdateUserRoles(ctx, token, reqBody.UserID, data.FromUserRoleArrayToStringArray(reqBody.Roles)) if updateUserRolesErr != nil { if errors.Is(updateUserRolesErr, auth.ErrInvalidToken) { @@ -284,6 +307,7 @@ func (h UserHandler) UpdateUserRoles(rw http.ResponseWriter, req *http.Request) return } + log.Ctx(ctx).Infof("[UpdateUserRoles] - User ID %s updated user with account ID %s roles to %v", authenticatedUserID, reqBody.UserID, reqBody.Roles) httpjson.RenderStatus(rw, http.StatusOK, map[string]string{"message": "user roles were updated successfully"}, httpjson.JSON) } diff --git a/internal/serve/httphandler/user_handler_test.go b/internal/serve/httphandler/user_handler_test.go index 34201069d..94cf11aa5 100644 --- a/internal/serve/httphandler/user_handler_test.go +++ b/internal/serve/httphandler/user_handler_test.go @@ -292,7 +292,7 @@ func Test_UserHandler_UserActivation(t *testing.T) { Return(true, nil). Times(4). On("GetUserFromToken", mock.Anything, token). - Return(&auth.User{}, nil). + Return(&auth.User{ID: "authenticated-user-id"}, nil). Twice() defer mocks.JWTManagerMock.AssertExpectations(t) @@ -332,7 +332,7 @@ func Test_UserHandler_UserActivation(t *testing.T) { assert.JSONEq(t, `{"message": "user activation was updated successfully"}`, string(respBody)) // validate logs - require.Contains(t, getTestEntries()[0].Message, "[ActivateUserAccount] - Activating user with account ID user-id") + require.Contains(t, getTestEntries()[0].Message, "[ActivateUserAccount] - User ID authenticated-user-id activating user with account ID user-id") }) } @@ -394,13 +394,12 @@ func Test_UserHandler_CreateUser(t *testing.T) { r := chi.NewRouter() - authenticatorMock := &auth.AuthenticatorMock{} - authManager := auth.NewAuthManager(auth.WithCustomAuthenticatorOption(authenticatorMock)) + authManagerMock := &auth.AuthManagerMock{} messengerClientMock := &message.MessengerClientMock{} uiBaseURL := "https://sdp.com" handler := &UserHandler{ - AuthManager: authManager, + AuthManager: authManagerMock, MessengerClient: messengerClientMock, UIBaseURL: uiBaseURL, Models: models, @@ -410,9 +409,26 @@ func Test_UserHandler_CreateUser(t *testing.T) { r.Post(url, handler.CreateUser) - t.Run("returns error when request body is invalid", func(t *testing.T) { + t.Run("returns Unauthorized when token is invalid", func(t *testing.T) { req, err := http.NewRequest(http.MethodPost, url, strings.NewReader(`{}`)) require.NoError(t, err) + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + + resp := w.Result() + respBody, err := io.ReadAll(resp.Body) + require.NoError(t, err) + + assert.Equal(t, http.StatusUnauthorized, resp.StatusCode) + assert.JSONEq(t, `{"error":"Not authorized."}`, string(respBody)) + }) + + t.Run("returns error when request body is invalid", func(t *testing.T) { + token := "mytoken" + ctx := context.WithValue(context.Background(), middleware.TokenContextKey, token) + + req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, strings.NewReader(`{}`)) + require.NoError(t, err) w := httptest.NewRecorder() @@ -446,7 +462,7 @@ func Test_UserHandler_CreateUser(t *testing.T) { "roles": ["role1", "role2"] } ` - req, err = http.NewRequest(http.MethodPost, url, strings.NewReader(body)) + req, err = http.NewRequestWithContext(ctx, http.MethodPost, url, strings.NewReader(body)) require.NoError(t, err) w = httptest.NewRecorder() @@ -478,7 +494,7 @@ func Test_UserHandler_CreateUser(t *testing.T) { "roles": ["role1"] } ` - req, err = http.NewRequest(http.MethodPost, url, strings.NewReader(body)) + req, err = http.NewRequestWithContext(ctx, http.MethodPost, url, strings.NewReader(body)) require.NoError(t, err) w = httptest.NewRecorder() @@ -505,7 +521,7 @@ func Test_UserHandler_CreateUser(t *testing.T) { buf := new(strings.Builder) log.DefaultLogger.SetOutput(buf) - req, err = http.NewRequest(http.MethodPost, url, strings.NewReader(`"invalid"`)) + req, err = http.NewRequestWithContext(ctx, http.MethodPost, url, strings.NewReader(`"invalid"`)) require.NoError(t, err) w = httptest.NewRecorder() @@ -528,16 +544,12 @@ func Test_UserHandler_CreateUser(t *testing.T) { }) t.Run("returns error when Auth Manager fails", func(t *testing.T) { - u := &auth.User{ - FirstName: "First", - LastName: "Last", - Email: "email@email.com", - Roles: []string{data.DeveloperUserRole.String()}, - } + token := "mytoken" + ctx := context.WithValue(context.Background(), middleware.TokenContextKey, token) - authenticatorMock. - On("CreateUser", mock.Anything, u, ""). - Return(nil, errors.New("unexpected error")). + authManagerMock. + On("GetUserID", mock.Anything, token). + Return("", errors.New("unexpected error")). Once() body := ` @@ -548,7 +560,8 @@ func Test_UserHandler_CreateUser(t *testing.T) { "roles": ["developer"] } ` - req, err := http.NewRequest(http.MethodPost, url, strings.NewReader(body)) + + req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, strings.NewReader(body)) require.NoError(t, err) w := httptest.NewRecorder() @@ -561,6 +574,42 @@ func Test_UserHandler_CreateUser(t *testing.T) { require.NoError(t, err) wantsBody := ` + { + "error": "Not authorized." + } + ` + + assert.Equal(t, http.StatusUnauthorized, resp.StatusCode) + assert.JSONEq(t, wantsBody, string(respBody)) + + u := &auth.User{ + FirstName: "First", + LastName: "Last", + Email: "email@email.com", + Roles: []string{data.DeveloperUserRole.String()}, + } + + authManagerMock. + On("GetUserID", mock.Anything, token). + Return("authenticated-user-id", nil). + Once(). + On("CreateUser", mock.Anything, u, ""). + Return(nil, errors.New("unexpected error")). + Once() + + req, err = http.NewRequestWithContext(ctx, http.MethodPost, url, strings.NewReader(body)) + require.NoError(t, err) + + w = httptest.NewRecorder() + + r.ServeHTTP(w, req) + + resp = w.Result() + + respBody, err = io.ReadAll(resp.Body) + require.NoError(t, err) + + wantsBody = ` { "error": "Cannot create user" } @@ -571,6 +620,9 @@ func Test_UserHandler_CreateUser(t *testing.T) { }) t.Run("returns Bad Request when user is duplicated", func(t *testing.T) { + token := "mytoken" + ctx := context.WithValue(context.Background(), middleware.TokenContextKey, token) + u := &auth.User{ FirstName: "First", LastName: "Last", @@ -578,7 +630,10 @@ func Test_UserHandler_CreateUser(t *testing.T) { Roles: []string{data.DeveloperUserRole.String()}, } - authenticatorMock. + authManagerMock. + On("GetUserID", mock.Anything, token). + Return("authenticated-user-id", nil). + Once(). On("CreateUser", mock.Anything, u, ""). Return(nil, auth.ErrUserEmailAlreadyExists). Once() @@ -591,7 +646,7 @@ func Test_UserHandler_CreateUser(t *testing.T) { "roles": ["developer"] } ` - req, err := http.NewRequest(http.MethodPost, url, strings.NewReader(body)) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, strings.NewReader(body)) require.NoError(t, err) w := httptest.NewRecorder() @@ -608,6 +663,9 @@ func Test_UserHandler_CreateUser(t *testing.T) { }) t.Run("returns error when sending email fails", func(t *testing.T) { + token := "mytoken" + ctx := context.WithValue(context.Background(), middleware.TokenContextKey, token) + u := &auth.User{ FirstName: "First", LastName: "Last", @@ -623,7 +681,10 @@ func Test_UserHandler_CreateUser(t *testing.T) { Roles: u.Roles, } - authenticatorMock. + authManagerMock. + On("GetUserID", mock.Anything, token). + Return("authenticated-user-id", nil). + Once(). On("CreateUser", mock.Anything, u, ""). Return(expectedUser, nil). Once() @@ -657,7 +718,7 @@ func Test_UserHandler_CreateUser(t *testing.T) { "roles": ["developer"] } ` - req, err := http.NewRequest(http.MethodPost, url, strings.NewReader(body)) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, strings.NewReader(body)) require.NoError(t, err) w := httptest.NewRecorder() @@ -680,6 +741,9 @@ func Test_UserHandler_CreateUser(t *testing.T) { }) t.Run("returns error when joining the forgot password link", func(t *testing.T) { + token := "mytoken" + ctx := context.WithValue(context.Background(), middleware.TokenContextKey, token) + u := &auth.User{ FirstName: "First", LastName: "Last", @@ -695,7 +759,10 @@ func Test_UserHandler_CreateUser(t *testing.T) { Roles: u.Roles, } - authenticatorMock. + authManagerMock. + On("GetUserID", mock.Anything, token). + Return("authenticated-user-id", nil). + Once(). On("CreateUser", mock.Anything, u, ""). Return(expectedUser, nil). Once() @@ -708,13 +775,13 @@ func Test_UserHandler_CreateUser(t *testing.T) { "roles": ["developer"] } ` - req, err := http.NewRequest(http.MethodPost, url, strings.NewReader(body)) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, strings.NewReader(body)) require.NoError(t, err) w := httptest.NewRecorder() http.HandlerFunc(UserHandler{ - AuthManager: authManager, + AuthManager: authManagerMock, MessengerClient: messengerClientMock, UIBaseURL: "%invalid%", Models: models, @@ -736,6 +803,9 @@ func Test_UserHandler_CreateUser(t *testing.T) { }) t.Run("creates user successfully", func(t *testing.T) { + token := "mytoken" + ctx := context.WithValue(context.Background(), middleware.TokenContextKey, token) + buf := new(strings.Builder) log.DefaultLogger.SetOutput(buf) log.SetLevel(log.InfoLevel) @@ -756,7 +826,10 @@ func Test_UserHandler_CreateUser(t *testing.T) { IsActive: true, } - authenticatorMock. + authManagerMock. + On("GetUserID", mock.Anything, token). + Return("authenticated-user-id", nil). + Once(). On("CreateUser", mock.Anything, u, ""). Return(expectedUser, nil). Once() @@ -790,7 +863,7 @@ func Test_UserHandler_CreateUser(t *testing.T) { "roles": ["developer"] } ` - req, err := http.NewRequest(http.MethodPost, url, strings.NewReader(body)) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, strings.NewReader(body)) require.NoError(t, err) w := httptest.NewRecorder() @@ -817,10 +890,10 @@ func Test_UserHandler_CreateUser(t *testing.T) { assert.JSONEq(t, wantsBody, string(respBody)) // validate logs - require.Contains(t, buf.String(), "[CreateUserAccount] - Created user with account ID user-id") + require.Contains(t, buf.String(), "[CreateUserAccount] - User ID authenticated-user-id created user with account ID user-id") }) - authenticatorMock.AssertExpectations(t) + authManagerMock.AssertExpectations(t) messengerClientMock.AssertExpectations(t) } @@ -1043,6 +1116,9 @@ func Test_UserHandler_UpdateUserRoles(t *testing.T) { jwtManagerMock. On("ValidateToken", mock.Anything, token). Return(true, nil). + Twice(). + On("GetUserFromToken", mock.Anything, token). + Return(&auth.User{ID: "authenticated-user-id"}, nil). Once() roleManagerMock. @@ -1078,6 +1154,12 @@ func Test_UserHandler_UpdateUserRoles(t *testing.T) { token := "mytoken" jwtManagerMock. + On("ValidateToken", mock.Anything, token). + Return(true, nil). + Once(). + On("GetUserFromToken", mock.Anything, token). + Return(&auth.User{ID: "authenticated-user-id"}, nil). + Once(). On("ValidateToken", mock.Anything, token). Return(false, errors.New("unexpected error")). Once() @@ -1116,6 +1198,9 @@ func Test_UserHandler_UpdateUserRoles(t *testing.T) { jwtManagerMock. On("ValidateToken", mock.Anything, token). Return(true, nil). + Twice(). + On("GetUserFromToken", mock.Anything, token). + Return(&auth.User{ID: "authenticated-user-id"}, nil). Once() roleManagerMock. diff --git a/stellar-auth/pkg/cli/add_user.go b/stellar-auth/pkg/cli/add_user.go index 24716f56a..761caa35d 100644 --- a/stellar-auth/pkg/cli/add_user.go +++ b/stellar-auth/pkg/cli/add_user.go @@ -158,11 +158,12 @@ func execAddUser(ctx context.Context, dbUrl string, email, firstName, lastName, Roles: roles, } - _, err = authManager.CreateUser(ctx, newUser, password) + u, err := authManager.CreateUser(ctx, newUser, password) if err != nil { return fmt.Errorf("error creating user: %w", err) } + log.Ctx(ctx).Infof("[CLI - CreateUserAccount] - Created user with account ID %s through CLI with roles %v", u.ID, roles) return nil }