Get rid of checkError test function and fix account update validation

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)

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)
 	}