From 276f5f4f4908a059328cf441e9ed11993967c758 Mon Sep 17 00:00:00 2001 From: Vitaly Antonenko Date: Fri, 12 Jan 2024 13:46:13 +0300 Subject: [PATCH 1/7] Injects GetUserWithAuthInfo in User domain This commit injects GetUserWithAuthInfo in state and service layers. Adds unit-test for this function. Also it adds unit-test for GetAllUsers func in the service layer. --- core/user/user.go | 12 +++ domain/user/service/service.go | 24 ++++++ domain/user/service/service_test.go | 103 +++++++++++++++++++++++++ domain/user/service/state_mock_test.go | 15 ++++ domain/user/state/state.go | 48 +++++++++++- domain/user/state/state_test.go | 33 ++++++++ domain/user/state/types.go | 21 +++++ 7 files changed, 253 insertions(+), 3 deletions(-) diff --git a/core/user/user.go b/core/user/user.go index c812e2396b7..4ffbb00489d 100644 --- a/core/user/user.go +++ b/core/user/user.go @@ -10,6 +10,7 @@ import ( "github.com/juju/utils/v3" ) +// User represents a user in the system. type User struct { // UUID is the unique identifier for the user. UUID UUID @@ -27,6 +28,17 @@ type User struct { CreatedAt time.Time } +// UserWithAuthInfo represents a user in the system with auth info. +type UserWithAuthInfo struct { + User + + // LastLogin is the last time the user logged in. + LastLogin time.Time + + // Disabled is true if the user is disabled. + Disabled bool +} + // UUID is a unique identifier for a user. type UUID string diff --git a/domain/user/service/service.go b/domain/user/service/service.go index 207820ba46e..1afa3052375 100644 --- a/domain/user/service/service.go +++ b/domain/user/service/service.go @@ -54,6 +54,11 @@ type State interface { // returned. GetAllUsers(context.Context) ([]user.User, error) + // GetUserWithAuthInfo will retrieve the user with authentication information (last login, disabled) + // specified by UUID from the database. If the user does not exist an error that satisfies + // usererrors.NotFound will be returned. + GetUserWithAuthInfo(context.Context, user.UUID) (user.UserWithAuthInfo, error) + // RemoveUser marks the user as removed. This obviates the ability of a user // to function, but keeps the user retaining provenance, i.e. auditing. // RemoveUser will also remove any credentials and activation codes for the @@ -164,6 +169,25 @@ func (s *Service) GetAllUsers(ctx context.Context) ([]user.User, error) { return usr, nil } +// GetUserWithAuthInfo will find and return the user with UUID. If there is no +// user for the UUID then an error that satisfies usererrors.NotFound will +// be returned. +func (s *Service) GetUserWithAuthInfo( + ctx context.Context, + uuid user.UUID, +) (user.UserWithAuthInfo, error) { + if err := uuid.Validate(); err != nil { + return user.UserWithAuthInfo{}, errors.Annotatef(usererrors.UUIDNotValid, "validating uuid %q", uuid) + } + + usr, err := s.st.GetUserWithAuthInfo(ctx, uuid) + if err != nil { + return user.UserWithAuthInfo{}, errors.Annotatef(err, "getting user for uuid %q", uuid) + } + + return usr, nil +} + // ValidateUsername takes a user name and validates that the user name is // conformant to our regex rules defined in usernameValidationRegex. If a // user name is not valid an error is returned that satisfies diff --git a/domain/user/service/service_test.go b/domain/user/service/service_test.go index 932befb2585..416c6f58cff 100644 --- a/domain/user/service/service_test.go +++ b/domain/user/service/service_test.go @@ -31,6 +31,7 @@ type stateUser struct { passwordHash string passwordSalt []byte removed bool + lastLogin time.Time disabled bool } @@ -136,6 +137,45 @@ func (s *serviceSuite) setMockState(c *gc.C) map[user.UUID]stateUser { return user.User{}, usererrors.NotFound }).AnyTimes() + s.state.EXPECT().GetAllUsers( + gomock.Any(), + ).DoAndReturn(func( + _ context.Context) ([]user.User, error) { + var users []user.User + for _, usr := range mockState { + if !usr.removed { + users = append(users, user.User{ + CreatorUUID: usr.creatorUUID, + CreatedAt: usr.createdAt, + DisplayName: usr.displayName, + Name: usr.name, + }) + } + } + return users, nil + }).AnyTimes() + + s.state.EXPECT().GetUserWithAuthInfo( + gomock.Any(), gomock.Any(), + ).DoAndReturn(func( + _ context.Context, + uuid user.UUID) (user.UserWithAuthInfo, error) { + stUser, exists := mockState[uuid] + if !exists { + return user.UserWithAuthInfo{}, usererrors.NotFound + } + return user.UserWithAuthInfo{ + User: user.User{ + CreatorUUID: stUser.creatorUUID, + CreatedAt: stUser.createdAt, + DisplayName: stUser.displayName, + Name: stUser.name, + }, + LastLogin: stUser.lastLogin, + Disabled: stUser.disabled, + }, nil + }).AnyTimes() + s.state.EXPECT().AddUser( gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), ).DoAndReturn(func( @@ -1063,6 +1103,69 @@ func (s *serviceSuite) TestGetUserByNameNotFound(c *gc.C) { c.Assert(len(mockState), gc.Equals, 0) } +// TestGetAllUsers tests the happy path for GetAllUsers. +func (s *serviceSuite) TestGetAllUsers(c *gc.C) { + defer s.setupMocks(c).Finish() + mockState := s.setMockState(c) + uuid1, err := user.NewUUID() + c.Assert(err, jc.ErrorIsNil) + mockState[uuid1] = stateUser{ + name: "Jürgen.test", + createdAt: time.Now().Add(-time.Minute * 5), + displayName: "Old mate 👍", + } + uuid2, err := user.NewUUID() + c.Assert(err, jc.ErrorIsNil) + mockState[uuid2] = stateUser{ + name: "杨-test", + createdAt: time.Now().Add(-time.Minute * 5), + displayName: "test1", + } + + users, err := s.service().GetAllUsers(context.Background()) + c.Assert(err, jc.ErrorIsNil) + c.Assert(len(users), gc.Equals, 2) + c.Assert(users[0].Name, gc.Equals, "Jürgen.test") + c.Assert(users[1].Name, gc.Equals, "杨-test") +} + +// TestGetUserWithAuthInfo tests the happy path for GetUserWithAuthInfo. +func (s *serviceSuite) TestGetUserWithAuthInfo(c *gc.C) { + defer s.setupMocks(c).Finish() + mockState := s.setMockState(c) + uuid, err := user.NewUUID() + c.Assert(err, jc.ErrorIsNil) + lastLogin := time.Now().Add(-time.Minute * 2) + mockState[uuid] = stateUser{ + name: "Jürgen.test", + createdAt: time.Now().Add(-time.Minute * 5), + displayName: "Old mate 👍", + lastLogin: lastLogin, + disabled: true, + } + + user, err := s.service().GetUserWithAuthInfo(context.Background(), uuid) + c.Assert(err, jc.ErrorIsNil) + c.Assert(user.Name, gc.Equals, "Jürgen.test") + c.Assert(user.DisplayName, gc.Equals, "Old mate 👍") + c.Assert(user.LastLogin, gc.Equals, lastLogin) + c.Assert(user.Disabled, gc.Equals, true) +} + +// TestGetUserWithAuthInfoNotFound is testing that if we ask for a user that +// doesn't exist we get back an error that satisfies usererrors.NotFound. +func (s *serviceSuite) TestGetUserWithAuthInfoNotFound(c *gc.C) { + defer s.setupMocks(c).Finish() + mockState := s.setMockState(c) + + nonExistingUserUUID, err := user.NewUUID() + c.Assert(err, jc.ErrorIsNil) + + _, err = s.service().GetUserWithAuthInfo(context.Background(), nonExistingUserUUID) + c.Assert(err, jc.ErrorIs, usererrors.NotFound) + c.Assert(len(mockState), gc.Equals, 0) +} + // TestGetUserByNameInvalidUsername is here to assert that when we ask for a user with // a username that is invalid we get a UsernameNotValid error. We also check // here that the service doesn't let invalid usernames flow through to the state diff --git a/domain/user/service/state_mock_test.go b/domain/user/service/state_mock_test.go index 82a99606ee3..b6c52e8811a 100644 --- a/domain/user/service/state_mock_test.go +++ b/domain/user/service/state_mock_test.go @@ -155,6 +155,21 @@ func (mr *MockStateMockRecorder) GetUserByName(arg0, arg1 any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserByName", reflect.TypeOf((*MockState)(nil).GetUserByName), arg0, arg1) } +// GetUserWithAuthInfo mocks base method. +func (m *MockState) GetUserWithAuthInfo(arg0 context.Context, arg1 user.UUID) (user.UserWithAuthInfo, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetUserWithAuthInfo", arg0, arg1) + ret0, _ := ret[0].(user.UserWithAuthInfo) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetUserWithAuthInfo indicates an expected call of GetUserWithAuthInfo. +func (mr *MockStateMockRecorder) GetUserWithAuthInfo(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserWithAuthInfo", reflect.TypeOf((*MockState)(nil).GetUserWithAuthInfo), arg0, arg1) +} + // RemoveUser mocks base method. func (m *MockState) RemoveUser(arg0 context.Context, arg1 user.UUID) error { m.ctrl.T.Helper() diff --git a/domain/user/state/state.go b/domain/user/state/state.go index a59bcf8f8cf..81e4c2b1a0b 100644 --- a/domain/user/state/state.go +++ b/domain/user/state/state.go @@ -105,7 +105,7 @@ func (st *State) GetUser(ctx context.Context, uuid user.UUID) (user.User, error) var usr user.User err = db.Txn(ctx, func(ctx context.Context, tx *sqlair.TX) error { getUserQuery := ` -SELECT &User.* +SELECT (user.uuid, user.name, user.display_name, user.created_by_uuid, user.created_at) AS (&User.*) FROM user WHERE uuid = $M.uuid ` @@ -146,7 +146,7 @@ func (st *State) GetUserByName(ctx context.Context, name string) (user.User, err var usr user.User err = db.Txn(ctx, func(ctx context.Context, tx *sqlair.TX) error { getUserByNameQuery := ` -SELECT &User.* +SELECT (user.uuid, user.name, user.display_name, user.created_by_uuid, user.created_at) AS (&User.*) FROM user WHERE name = $M.name AND removed = false ` @@ -187,7 +187,7 @@ func (st *State) GetAllUsers(ctx context.Context) ([]user.User, error) { var usrs []user.User err = db.Txn(ctx, func(ctx context.Context, tx *sqlair.TX) error { getAllUsersQuery := ` -SELECT &User.* +SELECT (user.uuid, user.name, user.display_name, user.created_by_uuid, user.created_at) AS (&User.*) FROM user WHERE removed = false ` @@ -216,6 +216,48 @@ WHERE removed = false return usrs, nil } +// GetUserWithAuthInfo will retrieve the user with authentication information (last login, disabled) +// specified by UUID from the database. If the user does not exist an error that satisfies +// usererrors.NotFound will be returned. +func (st *State) GetUserWithAuthInfo(ctx context.Context, uuid user.UUID) (user.UserWithAuthInfo, error) { + db, err := st.DB() + if err != nil { + return user.UserWithAuthInfo{}, errors.Annotate(err, "getting DB access") + } + + var usr user.UserWithAuthInfo + err = db.Txn(ctx, func(ctx context.Context, tx *sqlair.TX) error { + getUserWithAuthInfoQuery := ` +SELECT &User.* +FROM user +JOIN user_authentication ON user.uuid = user_authentication.user_uuid +WHERE user.uuid = $M.uuid +` + + selectGetUserWithAuthInfoStmt, err := sqlair.Prepare(getUserWithAuthInfoQuery, User{}, sqlair.M{}) + if err != nil { + return errors.Annotate(err, "preparing select getUserWithAuthInfo query") + } + + var result User + err = tx.Query(ctx, selectGetUserWithAuthInfoStmt, sqlair.M{"uuid": uuid.String()}).Get(&result) + if err != nil && errors.Is(err, sql.ErrNoRows) { + return errors.Annotatef(usererrors.NotFound, "%q", uuid) + } else if err != nil { + return errors.Annotatef(err, "getting user with uuid %q", uuid) + } + + usr = result.toCoreUserWithAuthInfo() + + return nil + }) + if err != nil { + return user.UserWithAuthInfo{}, errors.Annotatef(err, "getting user with uuid %q", uuid) + } + + return usr, nil +} + // RemoveUser marks the user as removed. This obviates the ability of a user // to function, but keeps the user retaining provenance, i.e. auditing. // RemoveUser will also remove any credentials and activation codes for the diff --git a/domain/user/state/state_test.go b/domain/user/state/state_test.go index 4431c0735e8..dc1246e2005 100644 --- a/domain/user/state/state_test.go +++ b/domain/user/state/state_test.go @@ -452,6 +452,39 @@ func (s *stateSuite) TestGetAllUsers(c *gc.C) { c.Check(users[1].CreatedAt, gc.NotNil) } +// TestUserWithAuthInfo asserts that we can get a user with auth info from the +// database. +func (s *stateSuite) TestUserWithAuthInfo(c *gc.C) { + st := NewState(s.TxnRunnerFactory()) + + // Add admin1 user with password hash. + adminUUID, err := user.NewUUID() + c.Assert(err, jc.ErrorIsNil) + adminUser := user.User{ + Name: "admin", + DisplayName: "admin", + } + salt, err := auth.NewSalt() + c.Assert(err, jc.ErrorIsNil) + err = st.AddUserWithPasswordHash(context.Background(), adminUUID, adminUser, adminUUID, "passwordHash", salt) + c.Assert(err, jc.ErrorIsNil) + + // Disable admin1 user. + err = st.DisableUserAuthentication(context.Background(), adminUUID) + c.Assert(err, jc.ErrorIsNil) + + // Get user with auth info. + u, err := st.GetUserWithAuthInfo(context.Background(), adminUUID) + c.Assert(err, jc.ErrorIsNil) + + c.Check(u.Name, gc.Equals, adminUser.Name) + c.Check(u.DisplayName, gc.Equals, adminUser.DisplayName) + c.Check(u.CreatorUUID, gc.Equals, adminUUID) + c.Check(u.CreatedAt, gc.NotNil) + c.Check(u.LastLogin, gc.NotNil) + c.Check(u.Disabled, gc.Equals, true) +} + // TestSetPasswordHash asserts that we can set a password hash for a user. func (s *stateSuite) TestSetPasswordHash(c *gc.C) { st := NewState(s.TxnRunnerFactory()) diff --git a/domain/user/state/types.go b/domain/user/state/types.go index c1657302bce..b02fcd45d4a 100644 --- a/domain/user/state/types.go +++ b/domain/user/state/types.go @@ -28,6 +28,12 @@ type User struct { // CreatedAt is the time that the user was created at. CreatedAt time.Time `db:"created_at"` + + // LastLogin is the last time the user logged in. + LastLogin time.Time `db:"last_login"` + + // Disabled is true if the user is disabled. + Disabled bool `db:"disabled"` } // toCoreUser converts the state user to a core user. @@ -40,3 +46,18 @@ func (u User) toCoreUser() user.User { CreatedAt: u.CreatedAt, } } + +// toCoreUserWithAuthInfo converts the state user to a core user with auth info. +func (u User) toCoreUserWithAuthInfo() user.UserWithAuthInfo { + return user.UserWithAuthInfo{ + User: user.User{ + UUID: u.UUID, + Name: u.Name, + DisplayName: u.DisplayName, + CreatorUUID: u.CreatorUUID, + CreatedAt: u.CreatedAt, + }, + LastLogin: u.LastLogin, + Disabled: u.Disabled, + } +} From 5c75bd3d1eb2dcc7271c4a0af25ead537e81ccb4 Mon Sep 17 00:00:00 2001 From: Vitaly Antonenko Date: Fri, 12 Jan 2024 16:47:41 +0300 Subject: [PATCH 2/7] Modifies GetAllUsers to GetAllUsersWithAuthInfo This commit modifies GetAllUsers to GetAllUsersWithAuthInfo in user domain, because when auth info in usermanager facade when using this function. --- domain/user/service/service.go | 20 ++++++----- domain/user/service/service_test.go | 29 +++++++++------ domain/user/service/state_mock_test.go | 14 ++++---- domain/user/state/state.go | 15 ++++---- domain/user/state/state_test.go | 49 ++++++++++++++++---------- 5 files changed, 76 insertions(+), 51 deletions(-) diff --git a/domain/user/service/service.go b/domain/user/service/service.go index 1afa3052375..1c4432187f5 100644 --- a/domain/user/service/service.go +++ b/domain/user/service/service.go @@ -49,10 +49,10 @@ type State interface { // returned. GetUserByName(context.Context, string) (user.User, error) - // GetAllUsers will retrieve all users from the database where the user is - // active and has not been removed. If no users exist an empty slice will be - // returned. - GetAllUsers(context.Context) ([]user.User, error) + // GetAllUsersWithAuthInfo will retrieve all users with authentication information + // (last login, disabled) from the database. If no users exist an empty slice + // will be returned. + GetAllUsersWithAuthInfo(context.Context) ([]user.UserWithAuthInfo, error) // GetUserWithAuthInfo will retrieve the user with authentication information (last login, disabled) // specified by UUID from the database. If the user does not exist an error that satisfies @@ -160,13 +160,15 @@ func (s *Service) GetUserByName( return usr, nil } -// GetAllUsers will return all users that have not been removed. -func (s *Service) GetAllUsers(ctx context.Context) ([]user.User, error) { - usr, err := s.st.GetAllUsers(ctx) +// GetAllUsersWithAuthInfo will retrieve all users with authentication information +// (last login, disabled) from the database. If no users exist an empty slice +// will be returned. +func (s *Service) GetAllUsersWithAuthInfo(ctx context.Context) ([]user.UserWithAuthInfo, error) { + usrs, err := s.st.GetAllUsersWithAuthInfo(ctx) if err != nil { - return nil, errors.Annotate(err, "getting all users") + return nil, errors.Annotate(err, "getting all users with auth info") } - return usr, nil + return usrs, nil } // GetUserWithAuthInfo will find and return the user with UUID. If there is no diff --git a/domain/user/service/service_test.go b/domain/user/service/service_test.go index 416c6f58cff..09a473cc1f6 100644 --- a/domain/user/service/service_test.go +++ b/domain/user/service/service_test.go @@ -137,20 +137,22 @@ func (s *serviceSuite) setMockState(c *gc.C) map[user.UUID]stateUser { return user.User{}, usererrors.NotFound }).AnyTimes() - s.state.EXPECT().GetAllUsers( + s.state.EXPECT().GetAllUsersWithAuthInfo( gomock.Any(), ).DoAndReturn(func( - _ context.Context) ([]user.User, error) { - var users []user.User + _ context.Context) ([]user.UserWithAuthInfo, error) { + var users []user.UserWithAuthInfo for _, usr := range mockState { - if !usr.removed { - users = append(users, user.User{ + users = append(users, user.UserWithAuthInfo{ + User: user.User{ CreatorUUID: usr.creatorUUID, CreatedAt: usr.createdAt, DisplayName: usr.displayName, Name: usr.name, - }) - } + }, + LastLogin: usr.lastLogin, + Disabled: usr.disabled, + }) } return users, nil }).AnyTimes() @@ -1103,16 +1105,18 @@ func (s *serviceSuite) TestGetUserByNameNotFound(c *gc.C) { c.Assert(len(mockState), gc.Equals, 0) } -// TestGetAllUsers tests the happy path for GetAllUsers. -func (s *serviceSuite) TestGetAllUsers(c *gc.C) { +// TestGetAllUsersWithAuthInfo tests the happy path for GetAllUsersWithAuthInfo. +func (s *serviceSuite) TestGetAllUsersWithAuthInfo(c *gc.C) { defer s.setupMocks(c).Finish() mockState := s.setMockState(c) uuid1, err := user.NewUUID() c.Assert(err, jc.ErrorIsNil) + lastLogin := time.Now().Add(-time.Minute * 2) mockState[uuid1] = stateUser{ name: "Jürgen.test", createdAt: time.Now().Add(-time.Minute * 5), displayName: "Old mate 👍", + lastLogin: lastLogin, } uuid2, err := user.NewUUID() c.Assert(err, jc.ErrorIsNil) @@ -1120,13 +1124,18 @@ func (s *serviceSuite) TestGetAllUsers(c *gc.C) { name: "杨-test", createdAt: time.Now().Add(-time.Minute * 5), displayName: "test1", + lastLogin: lastLogin, } - users, err := s.service().GetAllUsers(context.Background()) + users, err := s.service().GetAllUsersWithAuthInfo(context.Background()) c.Assert(err, jc.ErrorIsNil) c.Assert(len(users), gc.Equals, 2) c.Assert(users[0].Name, gc.Equals, "Jürgen.test") + c.Assert(users[0].DisplayName, gc.Equals, "Old mate 👍") + c.Assert(users[0].LastLogin, gc.Equals, lastLogin) c.Assert(users[1].Name, gc.Equals, "杨-test") + c.Assert(users[1].DisplayName, gc.Equals, "test1") + c.Assert(users[1].LastLogin, gc.Equals, lastLogin) } // TestGetUserWithAuthInfo tests the happy path for GetUserWithAuthInfo. diff --git a/domain/user/service/state_mock_test.go b/domain/user/service/state_mock_test.go index b6c52e8811a..559b9c5d758 100644 --- a/domain/user/service/state_mock_test.go +++ b/domain/user/service/state_mock_test.go @@ -110,19 +110,19 @@ func (mr *MockStateMockRecorder) EnableUserAuthentication(arg0, arg1 any) *gomoc return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "EnableUserAuthentication", reflect.TypeOf((*MockState)(nil).EnableUserAuthentication), arg0, arg1) } -// GetAllUsers mocks base method. -func (m *MockState) GetAllUsers(arg0 context.Context) ([]user.User, error) { +// GetAllUsersWithAuthInfo mocks base method. +func (m *MockState) GetAllUsersWithAuthInfo(arg0 context.Context) ([]user.UserWithAuthInfo, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetAllUsers", arg0) - ret0, _ := ret[0].([]user.User) + ret := m.ctrl.Call(m, "GetAllUsersWithAuthInfo", arg0) + ret0, _ := ret[0].([]user.UserWithAuthInfo) ret1, _ := ret[1].(error) return ret0, ret1 } -// GetAllUsers indicates an expected call of GetAllUsers. -func (mr *MockStateMockRecorder) GetAllUsers(arg0 any) *gomock.Call { +// GetAllUsersWithAuthInfo indicates an expected call of GetAllUsersWithAuthInfo. +func (mr *MockStateMockRecorder) GetAllUsersWithAuthInfo(arg0 any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetAllUsers", reflect.TypeOf((*MockState)(nil).GetAllUsers), arg0) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetAllUsersWithAuthInfo", reflect.TypeOf((*MockState)(nil).GetAllUsersWithAuthInfo), arg0) } // GetUser mocks base method. diff --git a/domain/user/state/state.go b/domain/user/state/state.go index 81e4c2b1a0b..9798bdd4885 100644 --- a/domain/user/state/state.go +++ b/domain/user/state/state.go @@ -175,20 +175,21 @@ WHERE name = $M.name AND removed = false return usr, nil } -// GetAllUsers will retrieve all users from the database where the user is -// active and has not been removed. If no users exist an empty slice will be -// returned. -func (st *State) GetAllUsers(ctx context.Context) ([]user.User, error) { +// GetAllUsersWithAuthInfo will retrieve all users with authentication information +// (last login, disabled) from the database. If no users exist an empty slice +// will be returned. +func (st *State) GetAllUsersWithAuthInfo(ctx context.Context) ([]user.UserWithAuthInfo, error) { db, err := st.DB() if err != nil { return nil, errors.Annotate(err, "getting DB access") } - var usrs []user.User + var usrs []user.UserWithAuthInfo err = db.Txn(ctx, func(ctx context.Context, tx *sqlair.TX) error { getAllUsersQuery := ` -SELECT (user.uuid, user.name, user.display_name, user.created_by_uuid, user.created_at) AS (&User.*) +SELECT &User.* FROM user +JOIN user_authentication ON user.uuid = user_authentication.user_uuid WHERE removed = false ` @@ -204,7 +205,7 @@ WHERE removed = false } for _, result := range results { - usrs = append(usrs, result.toCoreUser()) + usrs = append(usrs, result.toCoreUserWithAuthInfo()) } return nil diff --git a/domain/user/state/state_test.go b/domain/user/state/state_test.go index dc1246e2005..ed8c75c42fa 100644 --- a/domain/user/state/state_test.go +++ b/domain/user/state/state_test.go @@ -411,45 +411,58 @@ WHERE uuid = ? c.Check(removed, gc.Equals, true) } -// TestGetAllUsers asserts that we can get all users from the database. -func (s *stateSuite) TestGetAllUsers(c *gc.C) { +// GetAllUsersWihAuthInfo asserts that we can get all users with auth info from +// the database. +func (s *stateSuite) TestGetAllUsersWihAuthInfo(c *gc.C) { st := NewState(s.TxnRunnerFactory()) - // Add admin1 user. - adminUUID1, err := user.NewUUID() + // Add admin1 user with password hash. + admin1UUID, err := user.NewUUID() c.Assert(err, jc.ErrorIsNil) - adminUser1 := user.User{ + admin1User := user.User{ Name: "admin1", DisplayName: "admin1", } - err = st.AddUser(context.Background(), adminUUID1, adminUser1, adminUUID1) + salt, err := auth.NewSalt() + c.Assert(err, jc.ErrorIsNil) + err = st.AddUserWithPasswordHash(context.Background(), admin1UUID, admin1User, admin1UUID, "passwordHash", salt) c.Assert(err, jc.ErrorIsNil) - // Add admin1 user. - adminUUID2, err := user.NewUUID() + // Add admin2 user with activation key. + admin2UUID, err := user.NewUUID() c.Assert(err, jc.ErrorIsNil) - adminUser2 := user.User{ + admin2User := user.User{ Name: "admin2", DisplayName: "admin2", } - err = st.AddUser(context.Background(), adminUUID2, adminUser2, adminUUID2) + admin2ActivationKey, err := generateActivationKey() + c.Assert(err, jc.ErrorIsNil) + err = st.AddUserWithActivationKey(context.Background(), admin2UUID, admin2User, admin2UUID, admin2ActivationKey) + c.Assert(err, jc.ErrorIsNil) + + // Disable admin2 user. + err = st.DisableUserAuthentication(context.Background(), admin2UUID) c.Assert(err, jc.ErrorIsNil) - // Get all users. - users, err := st.GetAllUsers(context.Background()) + // Get all users with auth info. + users, err := st.GetAllUsersWithAuthInfo(context.Background()) c.Assert(err, jc.ErrorIsNil) c.Assert(users, gc.HasLen, 2) - c.Check(users[0].Name, gc.Equals, adminUser1.Name) - c.Check(users[0].DisplayName, gc.Equals, adminUser1.DisplayName) - c.Check(users[0].CreatorUUID, gc.Equals, adminUUID1) + c.Check(users[0].Name, gc.Equals, admin1User.Name) + c.Check(users[0].DisplayName, gc.Equals, admin1User.DisplayName) + c.Check(users[0].CreatorUUID, gc.Equals, admin1UUID) c.Check(users[0].CreatedAt, gc.NotNil) + c.Check(users[0].LastLogin, gc.NotNil) + c.Check(users[0].Disabled, gc.Equals, false) - c.Check(users[1].Name, gc.Equals, adminUser2.Name) - c.Check(users[1].DisplayName, gc.Equals, adminUser2.DisplayName) - c.Check(users[1].CreatorUUID, gc.Equals, adminUUID2) + c.Check(users[1].Name, gc.Equals, admin2User.Name) + c.Check(users[1].DisplayName, gc.Equals, admin2User.DisplayName) + c.Check(users[1].CreatorUUID, gc.Equals, admin2UUID) c.Check(users[1].CreatedAt, gc.NotNil) + c.Check(users[1].LastLogin, gc.NotNil) + c.Check(users[1].Disabled, gc.Equals, true) } // TestUserWithAuthInfo asserts that we can get a user with auth info from the From 44e51bab50181ac19899db8bd74ffa1430abab29 Mon Sep 17 00:00:00 2001 From: Vitaly Antonenko Date: Fri, 12 Jan 2024 16:54:23 +0300 Subject: [PATCH 3/7] Explicitly list all needed colunms in queries. - in GetAllUsersWithAuthInfo and GetUserWithAuthInfo functions --- domain/user/state/state.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/domain/user/state/state.go b/domain/user/state/state.go index 9798bdd4885..7fad882bb1b 100644 --- a/domain/user/state/state.go +++ b/domain/user/state/state.go @@ -187,7 +187,7 @@ func (st *State) GetAllUsersWithAuthInfo(ctx context.Context) ([]user.UserWithAu var usrs []user.UserWithAuthInfo err = db.Txn(ctx, func(ctx context.Context, tx *sqlair.TX) error { getAllUsersQuery := ` -SELECT &User.* +SELECT (user.uuid, user.name, user.display_name, user.created_by_uuid, user.created_at, user_authentication.last_login, user_authentication.disabled) AS (&User.*) FROM user JOIN user_authentication ON user.uuid = user_authentication.user_uuid WHERE removed = false @@ -229,7 +229,7 @@ func (st *State) GetUserWithAuthInfo(ctx context.Context, uuid user.UUID) (user. var usr user.UserWithAuthInfo err = db.Txn(ctx, func(ctx context.Context, tx *sqlair.TX) error { getUserWithAuthInfoQuery := ` -SELECT &User.* +SELECT (user.uuid, user.name, user.display_name, user.created_by_uuid, user.created_at, user_authentication.last_login, user_authentication.disabled) AS (&User.*) FROM user JOIN user_authentication ON user.uuid = user_authentication.user_uuid WHERE user.uuid = $M.uuid From bd4db51499fa4270ed86643a7cc6b9b4606247d2 Mon Sep 17 00:00:00 2001 From: Vitaly Antonenko Date: Mon, 15 Jan 2024 14:53:40 +0300 Subject: [PATCH 4/7] Adds GetUserWithAutnInfoByName in User domain. --- domain/user/service/service.go | 27 +++++++++++++ domain/user/service/service_test.go | 56 ++++++++++++++++++++++++++ domain/user/service/state_mock_test.go | 15 +++++++ domain/user/state/state.go | 42 +++++++++++++++++++ domain/user/state/state_test.go | 31 ++++++++++++++ 5 files changed, 171 insertions(+) diff --git a/domain/user/service/service.go b/domain/user/service/service.go index 1c4432187f5..75dfe0cc8ff 100644 --- a/domain/user/service/service.go +++ b/domain/user/service/service.go @@ -59,6 +59,11 @@ type State interface { // usererrors.NotFound will be returned. GetUserWithAuthInfo(context.Context, user.UUID) (user.UserWithAuthInfo, error) + // GetUserWithAuthInfoByName will retrieve the user with authentication information (last login, disabled) + // specified by name from the database. If the user does not exist an error that satisfies + // usererrors.NotFound will be returned. + GetUserWithAuthInfoByName(ctx context.Context, name string) (user.UserWithAuthInfo, error) + // RemoveUser marks the user as removed. This obviates the ability of a user // to function, but keeps the user retaining provenance, i.e. auditing. // RemoveUser will also remove any credentials and activation codes for the @@ -190,6 +195,28 @@ func (s *Service) GetUserWithAuthInfo( return usr, nil } +// GetUserWithAuthInfoByName will find and return the user associated with name. If there is no +// user for the user name then an error that satisfies usererrors.NotFound will +// be returned. If supplied with an invalid user name then an error that satisfies +// usererrors.UsernameNotValid will be returned. +// +// GetUserWithAuthInfoByName will not return users that have been previously removed. +func (s *Service) GetUserWithAuthInfoByName( + ctx context.Context, + name string, +) (user.UserWithAuthInfo, error) { + if err := ValidateUsername(name); err != nil { + return user.UserWithAuthInfo{}, errors.Annotatef(err, "validating username %q", name) + } + + usr, err := s.st.GetUserWithAuthInfoByName(ctx, name) + if err != nil { + return user.UserWithAuthInfo{}, errors.Annotatef(err, "getting user %q", name) + } + + return usr, nil +} + // ValidateUsername takes a user name and validates that the user name is // conformant to our regex rules defined in usernameValidationRegex. If a // user name is not valid an error is returned that satisfies diff --git a/domain/user/service/service_test.go b/domain/user/service/service_test.go index 09a473cc1f6..d6dd396f17b 100644 --- a/domain/user/service/service_test.go +++ b/domain/user/service/service_test.go @@ -178,6 +178,28 @@ func (s *serviceSuite) setMockState(c *gc.C) map[user.UUID]stateUser { }, nil }).AnyTimes() + s.state.EXPECT().GetUserWithAuthInfoByName( + gomock.Any(), gomock.Any(), + ).DoAndReturn(func( + _ context.Context, + name string) (user.UserWithAuthInfo, error) { + for _, usr := range mockState { + if usr.name == name && !usr.removed { + return user.UserWithAuthInfo{ + User: user.User{ + CreatorUUID: usr.creatorUUID, + CreatedAt: usr.createdAt, + DisplayName: usr.displayName, + Name: usr.name, + }, + LastLogin: usr.lastLogin, + Disabled: usr.disabled, + }, nil + } + } + return user.UserWithAuthInfo{}, usererrors.NotFound + }).AnyTimes() + s.state.EXPECT().AddUser( gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), ).DoAndReturn(func( @@ -1187,6 +1209,40 @@ func (s *serviceSuite) TestGetUserByNameInvalidUsername(c *gc.C) { } } +// TestGetUserWithAuthInfoByName tests the happy path for GetUserWithAuthInfoByName. +func (s *serviceSuite) TestGetUserWithAuthInfoByName(c *gc.C) { + defer s.setupMocks(c).Finish() + mockState := s.setMockState(c) + uuid, err := user.NewUUID() + c.Assert(err, jc.ErrorIsNil) + lastLogin := time.Now().Add(-time.Minute * 2) + mockState[uuid] = stateUser{ + name: "Jürgen.test", + createdAt: time.Now().Add(-time.Minute * 5), + displayName: "Old mate 👍", + lastLogin: lastLogin, + disabled: true, + } + + user, err := s.service().GetUserWithAuthInfoByName(context.Background(), "Jürgen.test") + c.Assert(err, jc.ErrorIsNil) + c.Assert(user.Name, gc.Equals, "Jürgen.test") + c.Assert(user.DisplayName, gc.Equals, "Old mate 👍") + c.Assert(user.LastLogin, gc.Equals, lastLogin) + c.Assert(user.Disabled, gc.Equals, true) +} + +// TestGetUserWithAuthInfoByNameNotFound is testing that if we ask for a user that +// doesn't exist we get back an error that satisfies usererrors.NotFound. +func (s *serviceSuite) TestGetUserWithAuthInfoByNameNotFound(c *gc.C) { + defer s.setupMocks(c).Finish() + mockState := s.setMockState(c) + + _, err := s.service().GetUserWithAuthInfoByName(context.Background(), "test") + c.Assert(err, jc.ErrorIs, usererrors.NotFound) + c.Assert(len(mockState), gc.Equals, 0) +} + // TestEnableUserAuthentication tests the happy path for EnableUserAuthentication. func (s *serviceSuite) TestEnableUserAuthentication(c *gc.C) { defer s.setupMocks(c).Finish() diff --git a/domain/user/service/state_mock_test.go b/domain/user/service/state_mock_test.go index 559b9c5d758..28aebda0465 100644 --- a/domain/user/service/state_mock_test.go +++ b/domain/user/service/state_mock_test.go @@ -170,6 +170,21 @@ func (mr *MockStateMockRecorder) GetUserWithAuthInfo(arg0, arg1 any) *gomock.Cal return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserWithAuthInfo", reflect.TypeOf((*MockState)(nil).GetUserWithAuthInfo), arg0, arg1) } +// GetUserWithAuthInfoByName mocks base method. +func (m *MockState) GetUserWithAuthInfoByName(arg0 context.Context, arg1 string) (user.UserWithAuthInfo, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetUserWithAuthInfoByName", arg0, arg1) + ret0, _ := ret[0].(user.UserWithAuthInfo) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetUserWithAuthInfoByName indicates an expected call of GetUserWithAuthInfoByName. +func (mr *MockStateMockRecorder) GetUserWithAuthInfoByName(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserWithAuthInfoByName", reflect.TypeOf((*MockState)(nil).GetUserWithAuthInfoByName), arg0, arg1) +} + // RemoveUser mocks base method. func (m *MockState) RemoveUser(arg0 context.Context, arg1 user.UUID) error { m.ctrl.T.Helper() diff --git a/domain/user/state/state.go b/domain/user/state/state.go index 7fad882bb1b..7827ce1bb1f 100644 --- a/domain/user/state/state.go +++ b/domain/user/state/state.go @@ -259,6 +259,48 @@ WHERE user.uuid = $M.uuid return usr, nil } +// GetUserWithAuthInfoByName will retrieve the user with authentication information (last login, disabled) +// specified by name from the database. If the user does not exist an error that satisfies +// usererrors.NotFound will be returned. +func (st *State) GetUserWithAuthInfoByName(ctx context.Context, name string) (user.UserWithAuthInfo, error) { + db, err := st.DB() + if err != nil { + return user.UserWithAuthInfo{}, errors.Annotate(err, "getting DB access") + } + + var usr user.UserWithAuthInfo + err = db.Txn(ctx, func(ctx context.Context, tx *sqlair.TX) error { + getUserWithAuthInfoByNameQuery := ` +SELECT (user.uuid, user.name, user.display_name, user.created_by_uuid, user.created_at, user_authentication.last_login, user_authentication.disabled) AS (&User.*) +FROM user +JOIN user_authentication ON user.uuid = user_authentication.user_uuid +WHERE user.name = $M.name AND removed = false +` + + selectGetUserWithAuthInfoByNameStmt, err := sqlair.Prepare(getUserWithAuthInfoByNameQuery, User{}, sqlair.M{}) + if err != nil { + return errors.Annotate(err, "preparing select getUserWithAuthInfoByName query") + } + + var result User + err = tx.Query(ctx, selectGetUserWithAuthInfoByNameStmt, sqlair.M{"name": name}).Get(&result) + if err != nil && errors.Is(err, sql.ErrNoRows) { + return errors.Annotatef(usererrors.NotFound, "%q", name) + } else if err != nil { + return errors.Annotatef(err, "getting user with name %q", name) + } + + usr = result.toCoreUserWithAuthInfo() + + return nil + }) + if err != nil { + return user.UserWithAuthInfo{}, errors.Annotatef(err, "getting user with name %q", name) + } + + return usr, nil +} + // RemoveUser marks the user as removed. This obviates the ability of a user // to function, but keeps the user retaining provenance, i.e. auditing. // RemoveUser will also remove any credentials and activation codes for the diff --git a/domain/user/state/state_test.go b/domain/user/state/state_test.go index ed8c75c42fa..37dc838594e 100644 --- a/domain/user/state/state_test.go +++ b/domain/user/state/state_test.go @@ -366,6 +366,37 @@ func (s *stateSuite) TestGetUserByNameNotFound(c *gc.C) { c.Assert(err, jc.ErrorIs, usererrors.NotFound) } +// TestGetUserWithAuthInfoByName asserts that we can get a user with auth info +// by name from the database. +func (s *stateSuite) TestGetUserWithAuthInfoByName(c *gc.C) { + st := NewState(s.TxnRunnerFactory()) + + // Add admin user with password hash. + adminUUID, err := user.NewUUID() + c.Assert(err, jc.ErrorIsNil) + adminUser := user.User{ + Name: "admin", + DisplayName: "admin", + } + + salt, err := auth.NewSalt() + c.Assert(err, jc.ErrorIsNil) + + err = st.AddUserWithPasswordHash(context.Background(), adminUUID, adminUser, adminUUID, "passwordHash", salt) + c.Assert(err, jc.ErrorIsNil) + + // Get the user. + u, err := st.GetUserWithAuthInfoByName(context.Background(), adminUser.Name) + c.Assert(err, jc.ErrorIsNil) + + c.Check(u.Name, gc.Equals, adminUser.Name) + c.Check(u.DisplayName, gc.Equals, adminUser.DisplayName) + c.Check(u.CreatorUUID, gc.Equals, adminUUID) + c.Check(u.CreatedAt, gc.NotNil) + c.Check(u.LastLogin, gc.NotNil) + c.Check(u.Disabled, gc.Equals, false) +} + // TestRemoveUser asserts that we can remove a user from the database. func (s *stateSuite) TestRemoveUser(c *gc.C) { st := NewState(s.TxnRunnerFactory()) From 214a034cca7a19ea2ce60640555e95ba3f250e6b Mon Sep 17 00:00:00 2001 From: Vitaly Antonenko Date: Wed, 17 Jan 2024 16:34:28 +0300 Subject: [PATCH 5/7] Unit GetUser func group with GetUserWithAuthInfo. --- core/user/user.go | 5 - domain/user/service/service.go | 86 ++++------------ domain/user/service/service_test.go | 124 ++++++----------------- domain/user/service/state_mock_test.go | 44 ++------- domain/user/state/state.go | 132 +++++-------------------- domain/user/state/state_test.go | 29 ++++-- domain/user/state/types.go | 17 +--- 7 files changed, 102 insertions(+), 335 deletions(-) diff --git a/core/user/user.go b/core/user/user.go index 4ffbb00489d..ae8767b64b9 100644 --- a/core/user/user.go +++ b/core/user/user.go @@ -26,11 +26,6 @@ type User struct { // CreatedAt is the time that the user was created at. CreatedAt time.Time -} - -// UserWithAuthInfo represents a user in the system with auth info. -type UserWithAuthInfo struct { - User // LastLogin is the last time the user logged in. LastLogin time.Time diff --git a/domain/user/service/service.go b/domain/user/service/service.go index 75dfe0cc8ff..98e8fbe8aa8 100644 --- a/domain/user/service/service.go +++ b/domain/user/service/service.go @@ -38,31 +38,20 @@ type State interface { // usererrors.UserCreatorNotFound will be returned. AddUserWithActivationKey(ctx context.Context, uuid user.UUID, usr user.User, creatorUUID user.UUID, activationKey []byte) error - // GetUser will retrieve the user specified by UUID from the database where - // the user is active. If the user does not exist an error that satisfies - // usererrors.NotFound will be returned. - GetUser(context.Context, user.UUID) (user.User, error) - - // GetUserByName will retrieve the user specified by name from the database - // where the user is active and has not been removed. If the user does not - // exist or is removed an error that satisfies usererrors.NotFound will be - // returned. - GetUserByName(context.Context, string) (user.User, error) - - // GetAllUsersWithAuthInfo will retrieve all users with authentication information + // GetAllUsers will retrieve all users with authentication information // (last login, disabled) from the database. If no users exist an empty slice // will be returned. - GetAllUsersWithAuthInfo(context.Context) ([]user.UserWithAuthInfo, error) + GetAllUsers(context.Context) ([]user.User, error) - // GetUserWithAuthInfo will retrieve the user with authentication information (last login, disabled) + // GetUser will retrieve the user with authentication information (last login, disabled) // specified by UUID from the database. If the user does not exist an error that satisfies // usererrors.NotFound will be returned. - GetUserWithAuthInfo(context.Context, user.UUID) (user.UserWithAuthInfo, error) + GetUser(context.Context, user.UUID) (user.User, error) - // GetUserWithAuthInfoByName will retrieve the user with authentication information (last login, disabled) + // GetUserByName will retrieve the user with authentication information (last login, disabled) // specified by name from the database. If the user does not exist an error that satisfies // usererrors.NotFound will be returned. - GetUserWithAuthInfoByName(ctx context.Context, name string) (user.UserWithAuthInfo, error) + GetUserByName(ctx context.Context, name string) (user.User, error) // RemoveUser marks the user as removed. This obviates the ability of a user // to function, but keeps the user retaining provenance, i.e. auditing. @@ -124,6 +113,17 @@ func NewService(st State) *Service { } } +// GetAllUsers will retrieve all users with authentication information +// (last login, disabled) from the database. If no users exist an empty slice +// will be returned. +func (s *Service) GetAllUsers(ctx context.Context) ([]user.User, error) { + usrs, err := s.st.GetAllUsers(ctx) + if err != nil { + return nil, errors.Annotate(err, "getting all users with auth info") + } + return usrs, nil +} + // GetUser will find and return the user with UUID. If there is no // user for the UUID then an error that satisfies usererrors.NotFound will // be returned. @@ -165,58 +165,6 @@ func (s *Service) GetUserByName( return usr, nil } -// GetAllUsersWithAuthInfo will retrieve all users with authentication information -// (last login, disabled) from the database. If no users exist an empty slice -// will be returned. -func (s *Service) GetAllUsersWithAuthInfo(ctx context.Context) ([]user.UserWithAuthInfo, error) { - usrs, err := s.st.GetAllUsersWithAuthInfo(ctx) - if err != nil { - return nil, errors.Annotate(err, "getting all users with auth info") - } - return usrs, nil -} - -// GetUserWithAuthInfo will find and return the user with UUID. If there is no -// user for the UUID then an error that satisfies usererrors.NotFound will -// be returned. -func (s *Service) GetUserWithAuthInfo( - ctx context.Context, - uuid user.UUID, -) (user.UserWithAuthInfo, error) { - if err := uuid.Validate(); err != nil { - return user.UserWithAuthInfo{}, errors.Annotatef(usererrors.UUIDNotValid, "validating uuid %q", uuid) - } - - usr, err := s.st.GetUserWithAuthInfo(ctx, uuid) - if err != nil { - return user.UserWithAuthInfo{}, errors.Annotatef(err, "getting user for uuid %q", uuid) - } - - return usr, nil -} - -// GetUserWithAuthInfoByName will find and return the user associated with name. If there is no -// user for the user name then an error that satisfies usererrors.NotFound will -// be returned. If supplied with an invalid user name then an error that satisfies -// usererrors.UsernameNotValid will be returned. -// -// GetUserWithAuthInfoByName will not return users that have been previously removed. -func (s *Service) GetUserWithAuthInfoByName( - ctx context.Context, - name string, -) (user.UserWithAuthInfo, error) { - if err := ValidateUsername(name); err != nil { - return user.UserWithAuthInfo{}, errors.Annotatef(err, "validating username %q", name) - } - - usr, err := s.st.GetUserWithAuthInfoByName(ctx, name) - if err != nil { - return user.UserWithAuthInfo{}, errors.Annotatef(err, "getting user %q", name) - } - - return usr, nil -} - // ValidateUsername takes a user name and validates that the user name is // conformant to our regex rules defined in usernameValidationRegex. If a // user name is not valid an error is returned that satisfies diff --git a/domain/user/service/service_test.go b/domain/user/service/service_test.go index d6dd396f17b..310c6bddf61 100644 --- a/domain/user/service/service_test.go +++ b/domain/user/service/service_test.go @@ -102,6 +102,24 @@ func IsUserNameAlreadyExists(name string, m map[user.UUID]stateUser) bool { func (s *serviceSuite) setMockState(c *gc.C) map[user.UUID]stateUser { mockState := map[user.UUID]stateUser{} + s.state.EXPECT().GetAllUsers( + gomock.Any(), + ).DoAndReturn(func( + _ context.Context) ([]user.User, error) { + var users []user.User + for _, usr := range mockState { + users = append(users, user.User{ + CreatorUUID: usr.creatorUUID, + CreatedAt: usr.createdAt, + DisplayName: usr.displayName, + Name: usr.name, + LastLogin: usr.lastLogin, + Disabled: usr.disabled, + }) + } + return users, nil + }).AnyTimes() + s.state.EXPECT().GetUser( gomock.Any(), gomock.Any(), ).DoAndReturn(func( @@ -116,6 +134,8 @@ func (s *serviceSuite) setMockState(c *gc.C) map[user.UUID]stateUser { CreatedAt: stUser.createdAt, DisplayName: stUser.displayName, Name: stUser.name, + LastLogin: stUser.lastLogin, + Disabled: stUser.disabled, }, nil }).AnyTimes() @@ -131,75 +151,14 @@ func (s *serviceSuite) setMockState(c *gc.C) map[user.UUID]stateUser { CreatedAt: usr.createdAt, DisplayName: usr.displayName, Name: usr.name, + LastLogin: usr.lastLogin, + Disabled: usr.disabled, }, nil } } return user.User{}, usererrors.NotFound }).AnyTimes() - s.state.EXPECT().GetAllUsersWithAuthInfo( - gomock.Any(), - ).DoAndReturn(func( - _ context.Context) ([]user.UserWithAuthInfo, error) { - var users []user.UserWithAuthInfo - for _, usr := range mockState { - users = append(users, user.UserWithAuthInfo{ - User: user.User{ - CreatorUUID: usr.creatorUUID, - CreatedAt: usr.createdAt, - DisplayName: usr.displayName, - Name: usr.name, - }, - LastLogin: usr.lastLogin, - Disabled: usr.disabled, - }) - } - return users, nil - }).AnyTimes() - - s.state.EXPECT().GetUserWithAuthInfo( - gomock.Any(), gomock.Any(), - ).DoAndReturn(func( - _ context.Context, - uuid user.UUID) (user.UserWithAuthInfo, error) { - stUser, exists := mockState[uuid] - if !exists { - return user.UserWithAuthInfo{}, usererrors.NotFound - } - return user.UserWithAuthInfo{ - User: user.User{ - CreatorUUID: stUser.creatorUUID, - CreatedAt: stUser.createdAt, - DisplayName: stUser.displayName, - Name: stUser.name, - }, - LastLogin: stUser.lastLogin, - Disabled: stUser.disabled, - }, nil - }).AnyTimes() - - s.state.EXPECT().GetUserWithAuthInfoByName( - gomock.Any(), gomock.Any(), - ).DoAndReturn(func( - _ context.Context, - name string) (user.UserWithAuthInfo, error) { - for _, usr := range mockState { - if usr.name == name && !usr.removed { - return user.UserWithAuthInfo{ - User: user.User{ - CreatorUUID: usr.creatorUUID, - CreatedAt: usr.createdAt, - DisplayName: usr.displayName, - Name: usr.name, - }, - LastLogin: usr.lastLogin, - Disabled: usr.disabled, - }, nil - } - } - return user.UserWithAuthInfo{}, usererrors.NotFound - }).AnyTimes() - s.state.EXPECT().AddUser( gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), ).DoAndReturn(func( @@ -1127,8 +1086,8 @@ func (s *serviceSuite) TestGetUserByNameNotFound(c *gc.C) { c.Assert(len(mockState), gc.Equals, 0) } -// TestGetAllUsersWithAuthInfo tests the happy path for GetAllUsersWithAuthInfo. -func (s *serviceSuite) TestGetAllUsersWithAuthInfo(c *gc.C) { +// TestGetAllUsers tests the happy path for GetAllUsers. +func (s *serviceSuite) TestGetAllUsers(c *gc.C) { defer s.setupMocks(c).Finish() mockState := s.setMockState(c) uuid1, err := user.NewUUID() @@ -1149,7 +1108,7 @@ func (s *serviceSuite) TestGetAllUsersWithAuthInfo(c *gc.C) { lastLogin: lastLogin, } - users, err := s.service().GetAllUsersWithAuthInfo(context.Background()) + users, err := s.service().GetAllUsers(context.Background()) c.Assert(err, jc.ErrorIsNil) c.Assert(len(users), gc.Equals, 2) c.Assert(users[0].Name, gc.Equals, "Jürgen.test") @@ -1160,7 +1119,7 @@ func (s *serviceSuite) TestGetAllUsersWithAuthInfo(c *gc.C) { c.Assert(users[1].LastLogin, gc.Equals, lastLogin) } -// TestGetUserWithAuthInfo tests the happy path for GetUserWithAuthInfo. +// TestGetUserWithAuthInfo tests the happy path for GetUser. func (s *serviceSuite) TestGetUserWithAuthInfo(c *gc.C) { defer s.setupMocks(c).Finish() mockState := s.setMockState(c) @@ -1175,7 +1134,7 @@ func (s *serviceSuite) TestGetUserWithAuthInfo(c *gc.C) { disabled: true, } - user, err := s.service().GetUserWithAuthInfo(context.Background(), uuid) + user, err := s.service().GetUser(context.Background(), uuid) c.Assert(err, jc.ErrorIsNil) c.Assert(user.Name, gc.Equals, "Jürgen.test") c.Assert(user.DisplayName, gc.Equals, "Old mate 👍") @@ -1183,20 +1142,6 @@ func (s *serviceSuite) TestGetUserWithAuthInfo(c *gc.C) { c.Assert(user.Disabled, gc.Equals, true) } -// TestGetUserWithAuthInfoNotFound is testing that if we ask for a user that -// doesn't exist we get back an error that satisfies usererrors.NotFound. -func (s *serviceSuite) TestGetUserWithAuthInfoNotFound(c *gc.C) { - defer s.setupMocks(c).Finish() - mockState := s.setMockState(c) - - nonExistingUserUUID, err := user.NewUUID() - c.Assert(err, jc.ErrorIsNil) - - _, err = s.service().GetUserWithAuthInfo(context.Background(), nonExistingUserUUID) - c.Assert(err, jc.ErrorIs, usererrors.NotFound) - c.Assert(len(mockState), gc.Equals, 0) -} - // TestGetUserByNameInvalidUsername is here to assert that when we ask for a user with // a username that is invalid we get a UsernameNotValid error. We also check // here that the service doesn't let invalid usernames flow through to the state @@ -1209,7 +1154,7 @@ func (s *serviceSuite) TestGetUserByNameInvalidUsername(c *gc.C) { } } -// TestGetUserWithAuthInfoByName tests the happy path for GetUserWithAuthInfoByName. +// TestGetUserWithAuthInfoByName tests the happy path for GetUserByName. func (s *serviceSuite) TestGetUserWithAuthInfoByName(c *gc.C) { defer s.setupMocks(c).Finish() mockState := s.setMockState(c) @@ -1224,7 +1169,7 @@ func (s *serviceSuite) TestGetUserWithAuthInfoByName(c *gc.C) { disabled: true, } - user, err := s.service().GetUserWithAuthInfoByName(context.Background(), "Jürgen.test") + user, err := s.service().GetUserByName(context.Background(), "Jürgen.test") c.Assert(err, jc.ErrorIsNil) c.Assert(user.Name, gc.Equals, "Jürgen.test") c.Assert(user.DisplayName, gc.Equals, "Old mate 👍") @@ -1232,17 +1177,6 @@ func (s *serviceSuite) TestGetUserWithAuthInfoByName(c *gc.C) { c.Assert(user.Disabled, gc.Equals, true) } -// TestGetUserWithAuthInfoByNameNotFound is testing that if we ask for a user that -// doesn't exist we get back an error that satisfies usererrors.NotFound. -func (s *serviceSuite) TestGetUserWithAuthInfoByNameNotFound(c *gc.C) { - defer s.setupMocks(c).Finish() - mockState := s.setMockState(c) - - _, err := s.service().GetUserWithAuthInfoByName(context.Background(), "test") - c.Assert(err, jc.ErrorIs, usererrors.NotFound) - c.Assert(len(mockState), gc.Equals, 0) -} - // TestEnableUserAuthentication tests the happy path for EnableUserAuthentication. func (s *serviceSuite) TestEnableUserAuthentication(c *gc.C) { defer s.setupMocks(c).Finish() diff --git a/domain/user/service/state_mock_test.go b/domain/user/service/state_mock_test.go index 28aebda0465..82a99606ee3 100644 --- a/domain/user/service/state_mock_test.go +++ b/domain/user/service/state_mock_test.go @@ -110,19 +110,19 @@ func (mr *MockStateMockRecorder) EnableUserAuthentication(arg0, arg1 any) *gomoc return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "EnableUserAuthentication", reflect.TypeOf((*MockState)(nil).EnableUserAuthentication), arg0, arg1) } -// GetAllUsersWithAuthInfo mocks base method. -func (m *MockState) GetAllUsersWithAuthInfo(arg0 context.Context) ([]user.UserWithAuthInfo, error) { +// GetAllUsers mocks base method. +func (m *MockState) GetAllUsers(arg0 context.Context) ([]user.User, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetAllUsersWithAuthInfo", arg0) - ret0, _ := ret[0].([]user.UserWithAuthInfo) + ret := m.ctrl.Call(m, "GetAllUsers", arg0) + ret0, _ := ret[0].([]user.User) ret1, _ := ret[1].(error) return ret0, ret1 } -// GetAllUsersWithAuthInfo indicates an expected call of GetAllUsersWithAuthInfo. -func (mr *MockStateMockRecorder) GetAllUsersWithAuthInfo(arg0 any) *gomock.Call { +// GetAllUsers indicates an expected call of GetAllUsers. +func (mr *MockStateMockRecorder) GetAllUsers(arg0 any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetAllUsersWithAuthInfo", reflect.TypeOf((*MockState)(nil).GetAllUsersWithAuthInfo), arg0) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetAllUsers", reflect.TypeOf((*MockState)(nil).GetAllUsers), arg0) } // GetUser mocks base method. @@ -155,36 +155,6 @@ func (mr *MockStateMockRecorder) GetUserByName(arg0, arg1 any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserByName", reflect.TypeOf((*MockState)(nil).GetUserByName), arg0, arg1) } -// GetUserWithAuthInfo mocks base method. -func (m *MockState) GetUserWithAuthInfo(arg0 context.Context, arg1 user.UUID) (user.UserWithAuthInfo, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetUserWithAuthInfo", arg0, arg1) - ret0, _ := ret[0].(user.UserWithAuthInfo) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// GetUserWithAuthInfo indicates an expected call of GetUserWithAuthInfo. -func (mr *MockStateMockRecorder) GetUserWithAuthInfo(arg0, arg1 any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserWithAuthInfo", reflect.TypeOf((*MockState)(nil).GetUserWithAuthInfo), arg0, arg1) -} - -// GetUserWithAuthInfoByName mocks base method. -func (m *MockState) GetUserWithAuthInfoByName(arg0 context.Context, arg1 string) (user.UserWithAuthInfo, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetUserWithAuthInfoByName", arg0, arg1) - ret0, _ := ret[0].(user.UserWithAuthInfo) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// GetUserWithAuthInfoByName indicates an expected call of GetUserWithAuthInfoByName. -func (mr *MockStateMockRecorder) GetUserWithAuthInfoByName(arg0, arg1 any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserWithAuthInfoByName", reflect.TypeOf((*MockState)(nil).GetUserWithAuthInfoByName), arg0, arg1) -} - // RemoveUser mocks base method. func (m *MockState) RemoveUser(arg0 context.Context, arg1 user.UUID) error { m.ctrl.T.Helper() diff --git a/domain/user/state/state.go b/domain/user/state/state.go index 7827ce1bb1f..d10e7e4c603 100644 --- a/domain/user/state/state.go +++ b/domain/user/state/state.go @@ -93,101 +93,19 @@ func (st *State) AddUserWithActivationKey( }) } -// GetUser will retrieve the user specified by UUID from the database. -// If the user does not exist an error that satisfies -// usererrors.NotFound will be returned. -func (st *State) GetUser(ctx context.Context, uuid user.UUID) (user.User, error) { - db, err := st.DB() - if err != nil { - return user.User{}, errors.Annotate(err, "getting DB access") - } - - var usr user.User - err = db.Txn(ctx, func(ctx context.Context, tx *sqlair.TX) error { - getUserQuery := ` -SELECT (user.uuid, user.name, user.display_name, user.created_by_uuid, user.created_at) AS (&User.*) -FROM user -WHERE uuid = $M.uuid -` - selectGetUserStmt, err := sqlair.Prepare(getUserQuery, User{}, sqlair.M{}) - if err != nil { - return errors.Annotate(err, "preparing select getUser query") - } - - var result User - err = tx.Query(ctx, selectGetUserStmt, sqlair.M{"uuid": uuid.String()}).Get(&result) - if err != nil && errors.Is(err, sql.ErrNoRows) { - return errors.Annotatef(usererrors.NotFound, "%q", uuid) - } else if err != nil { - return errors.Annotatef(err, "getting user with uuid %q", uuid) - } - - usr = result.toCoreUser() - - return nil - }) - if err != nil { - return user.User{}, errors.Annotatef(err, "getting user with uuid %q", uuid) - } - - return usr, nil -} - -// GetUserByName will retrieve the user specified by name from the database -// where the user is active and has not been removed. If the user does not -// exist or is removed an error that satisfies usererrors.NotFound will be -// returned. -func (st *State) GetUserByName(ctx context.Context, name string) (user.User, error) { - db, err := st.DB() - if err != nil { - return user.User{}, errors.Annotate(err, "getting DB access") - } - - var usr user.User - err = db.Txn(ctx, func(ctx context.Context, tx *sqlair.TX) error { - getUserByNameQuery := ` -SELECT (user.uuid, user.name, user.display_name, user.created_by_uuid, user.created_at) AS (&User.*) -FROM user -WHERE name = $M.name AND removed = false -` - - selectGetUserByNameStmt, err := sqlair.Prepare(getUserByNameQuery, User{}, sqlair.M{}) - if err != nil { - return errors.Annotate(err, "preparing select getUserByName query") - } - - var result User - err = tx.Query(ctx, selectGetUserByNameStmt, sqlair.M{"name": name}).Get(&result) - if err != nil && errors.Is(err, sql.ErrNoRows) { - return errors.Annotatef(usererrors.NotFound, "%q", name) - } else if err != nil { - return errors.Annotatef(err, "getting user %q", name) - } - - usr = result.toCoreUser() - - return nil - }) - if err != nil { - return user.User{}, errors.Annotatef(err, "getting user %q", name) - } - - return usr, nil -} - -// GetAllUsersWithAuthInfo will retrieve all users with authentication information +// GetAllUsers will retrieve all users with authentication information // (last login, disabled) from the database. If no users exist an empty slice // will be returned. -func (st *State) GetAllUsersWithAuthInfo(ctx context.Context) ([]user.UserWithAuthInfo, error) { +func (st *State) GetAllUsers(ctx context.Context) ([]user.User, error) { db, err := st.DB() if err != nil { return nil, errors.Annotate(err, "getting DB access") } - var usrs []user.UserWithAuthInfo + var usrs []user.User err = db.Txn(ctx, func(ctx context.Context, tx *sqlair.TX) error { getAllUsersQuery := ` -SELECT (user.uuid, user.name, user.display_name, user.created_by_uuid, user.created_at, user_authentication.last_login, user_authentication.disabled) AS (&User.*) +SELECT &User.* FROM user JOIN user_authentication ON user.uuid = user_authentication.user_uuid WHERE removed = false @@ -205,7 +123,7 @@ WHERE removed = false } for _, result := range results { - usrs = append(usrs, result.toCoreUserWithAuthInfo()) + usrs = append(usrs, result.toCoreUser()) } return nil @@ -217,85 +135,85 @@ WHERE removed = false return usrs, nil } -// GetUserWithAuthInfo will retrieve the user with authentication information (last login, disabled) +// GetUser will retrieve the user with authentication information (last login, disabled) // specified by UUID from the database. If the user does not exist an error that satisfies // usererrors.NotFound will be returned. -func (st *State) GetUserWithAuthInfo(ctx context.Context, uuid user.UUID) (user.UserWithAuthInfo, error) { +func (st *State) GetUser(ctx context.Context, uuid user.UUID) (user.User, error) { db, err := st.DB() if err != nil { - return user.UserWithAuthInfo{}, errors.Annotate(err, "getting DB access") + return user.User{}, errors.Annotate(err, "getting DB access") } - var usr user.UserWithAuthInfo + var usr user.User err = db.Txn(ctx, func(ctx context.Context, tx *sqlair.TX) error { - getUserWithAuthInfoQuery := ` -SELECT (user.uuid, user.name, user.display_name, user.created_by_uuid, user.created_at, user_authentication.last_login, user_authentication.disabled) AS (&User.*) + getUserQuery := ` +SELECT &User.* FROM user JOIN user_authentication ON user.uuid = user_authentication.user_uuid WHERE user.uuid = $M.uuid ` - selectGetUserWithAuthInfoStmt, err := sqlair.Prepare(getUserWithAuthInfoQuery, User{}, sqlair.M{}) + selectGetUserStmt, err := sqlair.Prepare(getUserQuery, User{}, sqlair.M{}) if err != nil { return errors.Annotate(err, "preparing select getUserWithAuthInfo query") } var result User - err = tx.Query(ctx, selectGetUserWithAuthInfoStmt, sqlair.M{"uuid": uuid.String()}).Get(&result) + err = tx.Query(ctx, selectGetUserStmt, sqlair.M{"uuid": uuid.String()}).Get(&result) if err != nil && errors.Is(err, sql.ErrNoRows) { return errors.Annotatef(usererrors.NotFound, "%q", uuid) } else if err != nil { return errors.Annotatef(err, "getting user with uuid %q", uuid) } - usr = result.toCoreUserWithAuthInfo() + usr = result.toCoreUser() return nil }) if err != nil { - return user.UserWithAuthInfo{}, errors.Annotatef(err, "getting user with uuid %q", uuid) + return user.User{}, errors.Annotatef(err, "getting user with uuid %q", uuid) } return usr, nil } -// GetUserWithAuthInfoByName will retrieve the user with authentication information (last login, disabled) +// GetUserByName will retrieve the user with authentication information (last login, disabled) // specified by name from the database. If the user does not exist an error that satisfies // usererrors.NotFound will be returned. -func (st *State) GetUserWithAuthInfoByName(ctx context.Context, name string) (user.UserWithAuthInfo, error) { +func (st *State) GetUserByName(ctx context.Context, name string) (user.User, error) { db, err := st.DB() if err != nil { - return user.UserWithAuthInfo{}, errors.Annotate(err, "getting DB access") + return user.User{}, errors.Annotate(err, "getting DB access") } - var usr user.UserWithAuthInfo + var usr user.User err = db.Txn(ctx, func(ctx context.Context, tx *sqlair.TX) error { - getUserWithAuthInfoByNameQuery := ` -SELECT (user.uuid, user.name, user.display_name, user.created_by_uuid, user.created_at, user_authentication.last_login, user_authentication.disabled) AS (&User.*) + getUserByNameQuery := ` +SELECT &User.* FROM user JOIN user_authentication ON user.uuid = user_authentication.user_uuid WHERE user.name = $M.name AND removed = false ` - selectGetUserWithAuthInfoByNameStmt, err := sqlair.Prepare(getUserWithAuthInfoByNameQuery, User{}, sqlair.M{}) + selectGetUserByNameStmt, err := sqlair.Prepare(getUserByNameQuery, User{}, sqlair.M{}) if err != nil { return errors.Annotate(err, "preparing select getUserWithAuthInfoByName query") } var result User - err = tx.Query(ctx, selectGetUserWithAuthInfoByNameStmt, sqlair.M{"name": name}).Get(&result) + err = tx.Query(ctx, selectGetUserByNameStmt, sqlair.M{"name": name}).Get(&result) if err != nil && errors.Is(err, sql.ErrNoRows) { return errors.Annotatef(usererrors.NotFound, "%q", name) } else if err != nil { return errors.Annotatef(err, "getting user with name %q", name) } - usr = result.toCoreUserWithAuthInfo() + usr = result.toCoreUser() return nil }) if err != nil { - return user.UserWithAuthInfo{}, errors.Annotatef(err, "getting user with name %q", name) + return user.User{}, errors.Annotatef(err, "getting user with name %q", name) } return usr, nil diff --git a/domain/user/state/state_test.go b/domain/user/state/state_test.go index 37dc838594e..a05f8505f65 100644 --- a/domain/user/state/state_test.go +++ b/domain/user/state/state_test.go @@ -195,9 +195,12 @@ func (s *stateSuite) TestGetUser(c *gc.C) { Name: "admin", DisplayName: "admin", } - err = st.AddUser(context.Background(), adminUUID, adminUser, adminUUID) + + salt, err := auth.NewSalt() c.Assert(err, jc.ErrorIsNil) + err = st.AddUserWithPasswordHash(context.Background(), adminUUID, adminUser, adminUUID, "passwordHash", salt) + // Get the user. u, err := st.GetUser(context.Background(), adminUUID) c.Assert(err, jc.ErrorIsNil) @@ -229,9 +232,12 @@ func (s *stateSuite) TestGetRemovedUser(c *gc.C) { Name: "userToRemove", DisplayName: "userToRemove", } - err = st.AddUser(context.Background(), userToRemoveUUID, userToRemove, adminUUID) + + salt, err := auth.NewSalt() c.Assert(err, jc.ErrorIsNil) + err = st.AddUserWithPasswordHash(context.Background(), userToRemoveUUID, userToRemove, adminUUID, "passwordHash", salt) + // Remove userToRemove. err = st.RemoveUser(context.Background(), userToRemoveUUID) c.Assert(err, jc.ErrorIsNil) @@ -271,7 +277,11 @@ func (s *stateSuite) TestGetUserByName(c *gc.C) { Name: "admin", DisplayName: "admin", } - err = st.AddUser(context.Background(), adminUUID, adminUser, adminUUID) + + salt, err := auth.NewSalt() + c.Assert(err, jc.ErrorIsNil) + + err = st.AddUserWithPasswordHash(context.Background(), adminUUID, adminUser, adminUUID, "passwordHash", salt) c.Assert(err, jc.ErrorIsNil) // Get the user. @@ -282,6 +292,8 @@ func (s *stateSuite) TestGetUserByName(c *gc.C) { c.Check(u.DisplayName, gc.Equals, adminUser.DisplayName) c.Check(u.CreatorUUID, gc.Equals, adminUUID) c.Check(u.CreatedAt, gc.NotNil) + c.Check(u.LastLogin, gc.NotNil) + c.Check(u.Disabled, gc.Equals, false) } // TestGetRemovedUserByName asserts that we can get only non-removed user by name. @@ -343,9 +355,12 @@ func (s *stateSuite) TestGetUserByNameMultipleUsers(c *gc.C) { Name: "admin", DisplayName: "admin2", } - err = st.AddUser(context.Background(), admin2UUID, admin2User, admin2UUID) + + salt, err := auth.NewSalt() c.Assert(err, jc.ErrorIsNil) + err = st.AddUserWithPasswordHash(context.Background(), admin2UUID, admin2User, admin2UUID, "passwordHash", salt) + // Get the user. u, err := st.GetUserByName(context.Background(), "admin") c.Assert(err, jc.ErrorIsNil) @@ -386,7 +401,7 @@ func (s *stateSuite) TestGetUserWithAuthInfoByName(c *gc.C) { c.Assert(err, jc.ErrorIsNil) // Get the user. - u, err := st.GetUserWithAuthInfoByName(context.Background(), adminUser.Name) + u, err := st.GetUserByName(context.Background(), adminUser.Name) c.Assert(err, jc.ErrorIsNil) c.Check(u.Name, gc.Equals, adminUser.Name) @@ -476,7 +491,7 @@ func (s *stateSuite) TestGetAllUsersWihAuthInfo(c *gc.C) { c.Assert(err, jc.ErrorIsNil) // Get all users with auth info. - users, err := st.GetAllUsersWithAuthInfo(context.Background()) + users, err := st.GetAllUsers(context.Background()) c.Assert(err, jc.ErrorIsNil) c.Assert(users, gc.HasLen, 2) @@ -518,7 +533,7 @@ func (s *stateSuite) TestUserWithAuthInfo(c *gc.C) { c.Assert(err, jc.ErrorIsNil) // Get user with auth info. - u, err := st.GetUserWithAuthInfo(context.Background(), adminUUID) + u, err := st.GetUser(context.Background(), adminUUID) c.Assert(err, jc.ErrorIsNil) c.Check(u.Name, gc.Equals, adminUser.Name) diff --git a/domain/user/state/types.go b/domain/user/state/types.go index b02fcd45d4a..69b355dba50 100644 --- a/domain/user/state/types.go +++ b/domain/user/state/types.go @@ -44,20 +44,7 @@ func (u User) toCoreUser() user.User { DisplayName: u.DisplayName, CreatorUUID: u.CreatorUUID, CreatedAt: u.CreatedAt, - } -} - -// toCoreUserWithAuthInfo converts the state user to a core user with auth info. -func (u User) toCoreUserWithAuthInfo() user.UserWithAuthInfo { - return user.UserWithAuthInfo{ - User: user.User{ - UUID: u.UUID, - Name: u.Name, - DisplayName: u.DisplayName, - CreatorUUID: u.CreatorUUID, - CreatedAt: u.CreatedAt, - }, - LastLogin: u.LastLogin, - Disabled: u.Disabled, + LastLogin: u.LastLogin, + Disabled: u.Disabled, } } From 4cd6dc247d3480b0f60056f5f9158540e9208f18 Mon Sep 17 00:00:00 2001 From: Vitaly Antonenko Date: Wed, 17 Jan 2024 16:47:44 +0300 Subject: [PATCH 6/7] Adds UpdateLastLogin func to User Domain. --- domain/user/service/service.go | 21 ++++++++++++ domain/user/service/service_test.go | 34 ++++++++++++++++++++ domain/user/service/state_mock_test.go | 14 ++++++++ domain/user/state/state.go | 43 +++++++++++++++++++++++-- domain/user/state/state_test.go | 44 ++++++++++++++++++++++++++ 5 files changed, 154 insertions(+), 2 deletions(-) diff --git a/domain/user/service/service.go b/domain/user/service/service.go index 98e8fbe8aa8..f6788976670 100644 --- a/domain/user/service/service.go +++ b/domain/user/service/service.go @@ -79,6 +79,11 @@ type State interface { // If no user is found for the supplied UUID an error is returned that // satisfies usererrors.NotFound. DisableUserAuthentication(context.Context, user.UUID) error + + // UpdateLastLogin will update the last login time for the user. + // If no user is found for the supplied UUID an error is returned that + // satisfies usererrors.NotFound. + UpdateLastLogin(context.Context, user.UUID) error } // Service provides the API for working with users. @@ -404,6 +409,22 @@ func (s *Service) DisableUserAuthentication(ctx context.Context, uuid user.UUID) return nil } +// UpdateLastLogin will update the last login time for the user. +// +// The following error types are possible from this function: +// - usererrors.UUIDNotValid: When the UUID supplied is not valid. +// - usererrors.NotFound: If no user by the given UUID exists. +func (s *Service) UpdateLastLogin(ctx context.Context, uuid user.UUID) error { + if err := uuid.Validate(); err != nil { + return errors.Annotatef(usererrors.UUIDNotValid, "%q", uuid) + } + + if err := s.st.UpdateLastLogin(ctx, uuid); err != nil { + return errors.Annotatef(err, "updating last login for user with uuid %q", uuid) + } + return nil +} + // generateActivationKey is responsible for generating a new activation key that // can be used for supplying to a user. func generateActivationKey() ([]byte, error) { diff --git a/domain/user/service/service_test.go b/domain/user/service/service_test.go index 310c6bddf61..6bed630db07 100644 --- a/domain/user/service/service_test.go +++ b/domain/user/service/service_test.go @@ -317,6 +317,20 @@ func (s *serviceSuite) setMockState(c *gc.C) map[user.UUID]stateUser { return nil }).AnyTimes() + // Implement the contract defined by UpdateLastLogin + s.state.EXPECT().UpdateLastLogin( + gomock.Any(), gomock.Any(), + ).DoAndReturn(func( + _ context.Context, + uuid user.UUID) error { + usr, exists := mockState[uuid] + if !exists || usr.removed { + return usererrors.NotFound + } + usr.lastLogin = time.Now() + mockState[uuid] = usr + return nil + }).AnyTimes() return mockState } @@ -1284,3 +1298,23 @@ func (s *serviceSuite) TestUsernameValidation(c *gc.C) { } } } + +// TestUpdateLastLogin tests the happy path for UpdateLastLogin. +func (s *serviceSuite) TestUpdateLastLogin(c *gc.C) { + defer s.setupMocks(c).Finish() + mockState := s.setMockState(c) + now := time.Now() + uuid, err := user.NewUUID() + c.Assert(err, jc.ErrorIsNil) + + mockState[uuid] = stateUser{ + name: "username", + lastLogin: now, + } + + err = s.service().UpdateLastLogin(context.Background(), uuid) + c.Assert(err, jc.ErrorIsNil) + + userState := mockState[uuid] + c.Assert(userState.lastLogin, gc.NotNil) +} diff --git a/domain/user/service/state_mock_test.go b/domain/user/service/state_mock_test.go index 82a99606ee3..32de47e9d9d 100644 --- a/domain/user/service/state_mock_test.go +++ b/domain/user/service/state_mock_test.go @@ -196,3 +196,17 @@ func (mr *MockStateMockRecorder) SetPasswordHash(arg0, arg1, arg2, arg3 any) *go mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetPasswordHash", reflect.TypeOf((*MockState)(nil).SetPasswordHash), arg0, arg1, arg2, arg3) } + +// UpdateLastLogin mocks base method. +func (m *MockState) UpdateLastLogin(arg0 context.Context, arg1 user.UUID) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "UpdateLastLogin", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// UpdateLastLogin indicates an expected call of UpdateLastLogin. +func (mr *MockStateMockRecorder) UpdateLastLogin(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateLastLogin", reflect.TypeOf((*MockState)(nil).UpdateLastLogin), arg0, arg1) +} diff --git a/domain/user/state/state.go b/domain/user/state/state.go index d10e7e4c603..5165f921b88 100644 --- a/domain/user/state/state.go +++ b/domain/user/state/state.go @@ -155,7 +155,7 @@ WHERE user.uuid = $M.uuid selectGetUserStmt, err := sqlair.Prepare(getUserQuery, User{}, sqlair.M{}) if err != nil { - return errors.Annotate(err, "preparing select getUserWithAuthInfo query") + return errors.Annotate(err, "preparing select getUser query") } var result User @@ -197,7 +197,7 @@ WHERE user.name = $M.name AND removed = false selectGetUserByNameStmt, err := sqlair.Prepare(getUserByNameQuery, User{}, sqlair.M{}) if err != nil { - return errors.Annotate(err, "preparing select getUserWithAuthInfoByName query") + return errors.Annotate(err, "preparing select getUserByName query") } var result User @@ -427,6 +427,45 @@ func AddUserWithPassword( return errors.Trace(setPasswordHash(ctx, tx, uuid, passwordHash, salt)) } +// UpdateLastLogin updates the last login time for the user with the supplied +// uuid. If the user does not exist an error that satisfies +// usererrors.NotFound will be returned. +func (st *State) UpdateLastLogin(ctx context.Context, uuid user.UUID) error { + db, err := st.DB() + if err != nil { + return errors.Annotate(err, "getting DB access") + } + + updateLastLoginQuery := ` +UPDATE user_authentication +SET last_login = $M.last_login +WHERE user_uuid = $M.uuid +` + + updateLastLoginStmt, err := sqlair.Prepare(updateLastLoginQuery, sqlair.M{}) + if err != nil { + return errors.Annotate(err, "preparing update updateLastLogin query") + } + + return db.Txn(ctx, func(ctx context.Context, tx *sqlair.TX) error { + removed, err := st.isRemoved(ctx, tx, uuid) + if err != nil { + return errors.Annotatef(err, "getting user with uuid %q", uuid) + } + if removed { + return errors.Annotatef(usererrors.NotFound, "%q", uuid) + } + + query := tx.Query(ctx, updateLastLoginStmt, sqlair.M{"uuid": uuid.String(), "last_login": time.Now()}) + err = query.Run() + if err != nil { + return errors.Annotatef(err, "updating last login for user with uuid %q", uuid) + } + + return nil + }) +} + // addUser adds a new user to the database. If the user already exists an error // that satisfies usererrors.AlreadyExists will be returned. If the creator does // not exist an error that satisfies usererrors.UserCreatorUUIDNotFound will be diff --git a/domain/user/state/state_test.go b/domain/user/state/state_test.go index a05f8505f65..6b510ffdb36 100644 --- a/domain/user/state/state_test.go +++ b/domain/user/state/state_test.go @@ -200,6 +200,7 @@ func (s *stateSuite) TestGetUser(c *gc.C) { c.Assert(err, jc.ErrorIsNil) err = st.AddUserWithPasswordHash(context.Background(), adminUUID, adminUser, adminUUID, "passwordHash", salt) + c.Assert(err, jc.ErrorIsNil) // Get the user. u, err := st.GetUser(context.Background(), adminUUID) @@ -237,6 +238,7 @@ func (s *stateSuite) TestGetRemovedUser(c *gc.C) { c.Assert(err, jc.ErrorIsNil) err = st.AddUserWithPasswordHash(context.Background(), userToRemoveUUID, userToRemove, adminUUID, "passwordHash", salt) + c.Assert(err, jc.ErrorIsNil) // Remove userToRemove. err = st.RemoveUser(context.Background(), userToRemoveUUID) @@ -360,6 +362,7 @@ func (s *stateSuite) TestGetUserByNameMultipleUsers(c *gc.C) { c.Assert(err, jc.ErrorIsNil) err = st.AddUserWithPasswordHash(context.Background(), admin2UUID, admin2User, admin2UUID, "passwordHash", salt) + c.Assert(err, jc.ErrorIsNil) // Get the user. u, err := st.GetUserByName(context.Background(), "admin") @@ -862,3 +865,44 @@ WHERE user_uuid = ? c.Assert(disabled, gc.Equals, false) } + +// TestUpdateLastLogin asserts that we can update the last login time for a +// user. +func (s *stateSuite) TestUpdateLastLogin(c *gc.C) { + st := NewState(s.TxnRunnerFactory()) + + // Add admin user with activation key. + adminUUID, err := user.NewUUID() + c.Assert(err, jc.ErrorIsNil) + adminUser := user.User{ + Name: "admin", + DisplayName: "admin", + } + + salt, err := auth.NewSalt() + c.Assert(err, jc.ErrorIsNil) + + // Add user with password hash. + err = st.AddUserWithPasswordHash(context.Background(), adminUUID, adminUser, adminUUID, "passwordHash", salt) + c.Assert(err, jc.ErrorIsNil) + + // Update last login. + err = st.UpdateLastLogin(context.Background(), adminUUID) + c.Assert(err, jc.ErrorIsNil) + + // Check that the last login was updated correctly. + db := s.DB() + + row := db.QueryRow(` +SELECT last_login +FROM user_authentication +WHERE user_uuid = ? + `, adminUUID) + c.Assert(row.Err(), jc.ErrorIsNil) + + var lastLogin time.Time + err = row.Scan(&lastLogin) + c.Assert(err, jc.ErrorIsNil) + + c.Assert(lastLogin, gc.NotNil) +} From 17121457d81631675a89c36ae2b0b3b61ec8a72a Mon Sep 17 00:00:00 2001 From: Vitaly Antonenko Date: Thu, 18 Jan 2024 12:52:18 +0300 Subject: [PATCH 7/7] Changes time to UTC for LastLogin. --- domain/user/state/state.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/domain/user/state/state.go b/domain/user/state/state.go index 5165f921b88..0736642fa9e 100644 --- a/domain/user/state/state.go +++ b/domain/user/state/state.go @@ -456,7 +456,7 @@ WHERE user_uuid = $M.uuid return errors.Annotatef(usererrors.NotFound, "%q", uuid) } - query := tx.Query(ctx, updateLastLoginStmt, sqlair.M{"uuid": uuid.String(), "last_login": time.Now()}) + query := tx.Query(ctx, updateLastLoginStmt, sqlair.M{"uuid": uuid.String(), "last_login": time.Now().UTC()}) err = query.Run() if err != nil { return errors.Annotatef(err, "updating last login for user with uuid %q", uuid)