From 79a4b811470943c0d82a70da96e89288b57b5a87 Mon Sep 17 00:00:00 2001 From: Benedikt Kulmann Date: Thu, 15 Oct 2020 22:58:07 +0200 Subject: [PATCH] Get rid of `checkError` test function and fix account update validation --- .../pkg/proto/v0/accounts.pb.micro_test.go | 51 ++++++++----------- accounts/pkg/service/v0/accounts.go | 40 ++++++++++++++- 2 files changed, 60 insertions(+), 31 deletions(-) diff --git a/accounts/pkg/proto/v0/accounts.pb.micro_test.go b/accounts/pkg/proto/v0/accounts.pb.micro_test.go index 1b45c665a..a8b82b724 100644 --- a/accounts/pkg/proto/v0/accounts.pb.micro_test.go +++ b/accounts/pkg/proto/v0/accounts.pb.micro_test.go @@ -210,7 +210,6 @@ func cleanUp(t *testing.T) { continue } _, _ = deleteAccount(t, id) - //checkError(t, err) } datastore = filepath.Join(dataPath, "groups") @@ -309,12 +308,6 @@ func assertGroupHasMember(t *testing.T, grp *proto.Group, memberId string) { t.Fatalf("Member with id %s expected to be in group '%s', but not found", memberId, grp.DisplayName) } -func checkError(t *testing.T, err error) { - if err != nil { - t.Fatalf("Expected Error to be nil but got %s", err) - } -} - func createAccount(t *testing.T, user string) (*proto.Account, error) { client := service.Client() cl := proto.NewAccountsService("com.owncloud.api.accounts", client) @@ -368,7 +361,7 @@ func listGroups(t *testing.T) *proto.ListGroupsResponse { cl := proto.NewGroupsService("com.owncloud.api.accounts", client) response, err := cl.ListGroups(context.Background(), request) - checkError(t, err) + assert.NoError(t, err) return response } @@ -403,14 +396,14 @@ func createTmpDir() string { // https://github.com/owncloud/ocis/accounts/issues/61 func TestCreateAccount(t *testing.T) { resp, err := createAccount(t, "user1") - checkError(t, err) + assert.NoError(t, err) assertUserExists(t, getAccount("user1")) assert.IsType(t, &proto.Account{}, resp) // Account is not returned in response // assertAccountsSame(t, getAccount("user1"), resp) resp, err = createAccount(t, "user2") - checkError(t, err) + assert.NoError(t, err) assertUserExists(t, getAccount("user2")) assert.IsType(t, &proto.Account{}, resp) // Account is not returned in response @@ -438,7 +431,7 @@ func TestCreateExistingUser(t *testing.T) { func TestCreateAccountInvalidUserName(t *testing.T) { resp, err := listAccounts(t) - checkError(t, err) + assert.NoError(t, err) numAccounts := len(resp.GetAccounts()) testData := []string{ @@ -459,7 +452,7 @@ func TestCreateAccountInvalidUserName(t *testing.T) { // resp should have the same number of accounts resp, err = listAccounts(t) - checkError(t, err) + assert.NoError(t, err) assert.Equal(t, numAccounts, len(resp.GetAccounts())) @@ -513,7 +506,7 @@ func TestUpdateAccount(t *testing.T) { &proto.Account{ DisplayName: "12345", PreferredName: "a12345", - OnPremisesSamAccountName: "54321", + OnPremisesSamAccountName: "a54321", UidNumber: 1000, GidNumber: 1000, Mail: "1.2@3.c_@", @@ -628,7 +621,7 @@ func TestListAccounts(t *testing.T) { createAccount(t, "user2") resp, err := listAccounts(t) - checkError(t, err) + assert.NoError(t, err) assert.IsType(t, &proto.ListAccountsResponse{}, resp) assert.Equal(t, 8, len(resp.Accounts)) @@ -658,7 +651,7 @@ func TestGetAccount(t *testing.T) { resp, err := cl.GetAccount(context.Background(), req) - checkError(t, err) + assert.NoError(t, err) assert.IsType(t, &proto.Account{}, resp) assertAccountsSame(t, getAccount("user1"), resp) @@ -677,7 +670,7 @@ func TestDeleteAccount(t *testing.T) { cl := proto.NewAccountsService("com.owncloud.api.accounts", client) resp, err := cl.DeleteAccount(context.Background(), req) - checkError(t, err) + assert.NoError(t, err) assert.IsType(t, resp, &empty.Empty{}) // Check the account doesn't exists anymore @@ -695,7 +688,7 @@ func TestListGroups(t *testing.T) { cl := proto.NewGroupsService("com.owncloud.api.accounts", client) resp, err := cl.ListGroups(context.Background(), req) - checkError(t, err) + assert.NoError(t, err) assert.IsType(t, &proto.ListGroupsResponse{}, resp) assert.Equal(t, 9, len(resp.Groups)) @@ -738,7 +731,7 @@ func TestGetGroups(t *testing.T) { req := &proto.GetGroupRequest{Id: group.Id} resp, err := cl.GetGroup(context.Background(), req) - checkError(t, err) + assert.NoError(t, err) assert.IsType(t, &proto.Group{}, resp) assertGroupsSame(t, group, resp) } @@ -753,7 +746,7 @@ func TestCreateGroup(t *testing.T) { }} res, err := createGroup(t, group) - checkError(t, err) + assert.NoError(t, err) assert.IsType(t, &proto.Group{}, res) @@ -792,12 +785,12 @@ func TestDeleteGroup(t *testing.T) { req := &proto.DeleteGroupRequest{Id: grp1.Id} res, err := cl.DeleteGroup(context.Background(), req) assert.IsType(t, res, &empty.Empty{}) - checkError(t, err) + assert.NoError(t, err) req = &proto.DeleteGroupRequest{Id: grp2.Id} res, err = cl.DeleteGroup(context.Background(), req) assert.IsType(t, res, &empty.Empty{}) - checkError(t, err) + assert.NoError(t, err) groupsResponse := listGroups(t) assertResponseNotContainsGroup(t, groupsResponse, grp1) @@ -890,7 +883,7 @@ func TestAddMember(t *testing.T) { req := &proto.AddMemberRequest{GroupId: grp1.Id, AccountId: account.Id} res, err := cl.AddMember(context.Background(), req) - checkError(t, err) + assert.NoError(t, err) assert.IsType(t, &proto.Group{}, res) @@ -924,7 +917,7 @@ func TestAddMemberAlreadyInGroup(t *testing.T) { res, err := cl.AddMember(context.Background(), req) // Should Give Error - checkError(t, err) + assert.NoError(t, err) assert.IsType(t, &proto.Group{}, res) //assert.Equal(t, proto.Group{}, *res) //assertGroupsSame(t, updatedGroup, res) @@ -995,7 +988,7 @@ func TestRemoveMember(t *testing.T) { req := &proto.RemoveMemberRequest{GroupId: grp1.Id, AccountId: account.Id} res, err := cl.RemoveMember(context.Background(), req) - checkError(t, err) + assert.NoError(t, err) assert.IsType(t, &proto.Group{}, res) //assert.Equal(t, proto.Group{}, *res) @@ -1054,7 +1047,7 @@ func TestRemoveMemberNotInGroup(t *testing.T) { res, err := cl.RemoveMember(context.Background(), req) // Should give an error - checkError(t, err) + assert.NoError(t, err) assert.IsType(t, &proto.Group{}, res) //assert.Error(t, err) @@ -1091,7 +1084,7 @@ func TestListMembers(t *testing.T) { req := &proto.ListMembersRequest{Id: expectedGroup.Id} res, err := cl.ListMembers(context.Background(), req) - checkError(t, err) + assert.NoError(t, err) assert.Equal(t, len(res.Members), len(expectedGroup.Members)) @@ -1124,7 +1117,7 @@ func TestListMembersEmptyGroup(t *testing.T) { res, err := cl.ListMembers(context.Background(), req) - checkError(t, err) + assert.NoError(t, err) assert.Empty(t, res.Members) cleanUp(t) @@ -1140,12 +1133,12 @@ func TestAccountUpdateMask(t *testing.T) { Account: &proto.Account{ Id: user1.Id, DisplayName: "ShouldBeUpdated", - PreferredName: "ShouldStaySame", + PreferredName: "ShouldStaySame And Is Invalid Anyway", }} cl := proto.NewAccountsService("com.owncloud.api.accounts", client) res, err := cl.UpdateAccount(context.Background(), req) - checkError(t, err) + assert.NoError(t, err) assert.Equal(t, "ShouldBeUpdated", res.DisplayName) assert.Equal(t, user1.PreferredName, res.PreferredName) diff --git a/accounts/pkg/service/v0/accounts.go b/accounts/pkg/service/v0/accounts.go index d1b979e43..18f2fd3c4 100644 --- a/accounts/pkg/service/v0/accounts.go +++ b/accounts/pkg/service/v0/accounts.go @@ -439,8 +439,20 @@ func (s Service) UpdateAccount(ctx context.Context, in *proto.UpdateAccountReque return merrors.BadRequest(s.id, "%s", err) } - if err = validateAccount(s.id, *in.Account); err != nil { - return err + if _, exists := validMask.Filter("PreferredName"); exists { + if err = validateAccountPreferredName(s.id, *in.Account); err != nil { + return err + } + } + if _, exists := validMask.Filter("OnPremisesSamAccountName"); exists { + if err = validateAccountOnPremisesSamAccountName(s.id, *in.Account); err != nil { + return err + } + } + if _, exists := validMask.Filter("Mail"); exists { + if err = validateAccountEmail(s.id, *in.Account); err != nil { + return err + } } if err := fieldmask_utils.StructToStruct(validMask, in.Account, out); err != nil { @@ -578,9 +590,33 @@ func (s Service) DeleteAccount(ctx context.Context, in *proto.DeleteAccountReque } func validateAccount(serviceID string, a proto.Account) error { + if err := validateAccountPreferredName(serviceID, a); err != nil { + return err + } + if err := validateAccountOnPremisesSamAccountName(serviceID, a); err != nil { + return err + } + if err := validateAccountEmail(serviceID, a); err != nil { + return err + } + return nil +} + +func validateAccountPreferredName(serviceID string, a proto.Account) error { if !isValidUsername(a.PreferredName) { return merrors.BadRequest(serviceID, "preferred_name '%s' must be at least the local part of an email", a.PreferredName) } + return nil +} + +func validateAccountOnPremisesSamAccountName(serviceID string, a proto.Account) error { + if !isValidUsername(a.OnPremisesSamAccountName) { + return merrors.BadRequest(serviceID, "on_premises_sam_account_name '%s' must be at least the local part of an email", a.OnPremisesSamAccountName) + } + return nil +} + +func validateAccountEmail(serviceID string, a proto.Account) error { if !isValidEmail(a.Mail) { return merrors.BadRequest(serviceID, "mail '%s' must be a valid email", a.Mail) }