Merge pull request #123 from owncloud/prevent-duplicate-accounts

Prevent creating duplicate accounts

changelog/unreleased/unique-account-fields

@@ -0,0 +1,5 @@
+Bugfix: Don't create account if id/mail/username already taken
+
+We don't allow anymore to create a new account if the provided id/mail/username is already taken.
+
+https://github.com/owncloud/ocis-accounts/pull/123

pkg/proto/v0/accounts.pb.micro_test.go

@@ -208,7 +208,7 @@ func cleanUp(t *testing.T) {
 			continue
 		}
 		_, err := deleteAccount(t, id)
-		checkError(t, err)
+		checkNoError(t, err)
 	}
 
 	datastore = filepath.Join(dataPath, "groups")
@@ -219,7 +219,7 @@ func cleanUp(t *testing.T) {
 			continue
 		}
 		_, err := deleteGroup(t, id)
-		checkError(t, err)
+		checkNoError(t, err)
 	}
 
 	newCreatedAccounts = []string{}
@@ -308,7 +308,7 @@ 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) {
+func checkNoError(t *testing.T, err error) {
 	if err != nil {
 		t.Fatalf("Expected Error to be nil but got %s", err)
 	}
@@ -367,7 +367,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)
+	checkNoError(t, err)
 	return response
 }
 
@@ -393,14 +393,14 @@ func deleteGroup(t *testing.T, id string) (*empty.Empty, error) {
 func TestCreateAccount(t *testing.T) {
 
 	resp, err := createAccount(t, "user1")
-	checkError(t, err)
+	checkNoError(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)
+	checkNoError(t, err)
 	assertUserExists(t, getAccount("user2"))
 	assert.IsType(t, &proto.Account{}, resp)
 	// Account is not returned in response
@@ -409,14 +409,13 @@ func TestCreateAccount(t *testing.T) {
 	cleanUp(t)
 }
 
-// https://github.com/owncloud/ocis-accounts/issues/62
 func TestCreateExistingUser(t *testing.T) {
 	createAccount(t, "user1")
-	_, err := createAccount(t, "user1")
+	res, err := createAccount(t, "user1")
 
-	// Should give error but it does not
-	checkError(t, err)
-	assertUserExists(t, getAccount("user1"))
+	assert.Empty(t, res)
+	assert.Error(t, err)
+	assert.Equal(t, merrors.BadRequest(".", "account already exists"), err)
 
 	cleanUp(t)
 }
@@ -426,7 +425,7 @@ func TestCreateExistingUser(t *testing.T) {
 func TestCreateAccountInvalidUserName(t *testing.T) {
 
 	resp, err := listAccounts(t)
-	checkError(t, err)
+	checkNoError(t, err)
 	numAccounts := len(resp.GetAccounts())
 
 	testData := []string{
@@ -447,7 +446,7 @@ func TestCreateAccountInvalidUserName(t *testing.T) {
 
 	// resp should have the same number of accounts
 	resp, err = listAccounts(t)
-	checkError(t, err)
+	checkNoError(t, err)
 
 	assert.Equal(t, numAccounts, len(resp.GetAccounts()))
 
@@ -528,7 +527,7 @@ func TestUpdateAccount(t *testing.T) {
 			tt.userAccount.IsResourceAccount = false
 			resp, err := updateAccount(t, tt.userAccount, updateMask)
 
-			checkError(t, err)
+			checkNoError(t, err)
 
 			assert.IsType(t, &proto.Account{}, resp)
 			assertAccountsSame(t, tt.userAccount, resp)
@@ -608,7 +607,7 @@ func TestListAccounts(t *testing.T) {
 	createAccount(t, "user2")
 
 	resp, err := listAccounts(t)
-	checkError(t, err)
+	checkNoError(t, err)
 
 	assert.IsType(t, &proto.ListAccountsResponse{}, resp)
 	assert.Equal(t, 8, len(resp.Accounts))
@@ -622,7 +621,7 @@ func TestListAccounts(t *testing.T) {
 func TestListWithoutUserCreation(t *testing.T) {
 	resp, err := listAccounts(t)
 
-	checkError(t, err)
+	checkNoError(t, err)
 
 	// Only 5 default users
 	assert.Equal(t, 6, len(resp.Accounts))
@@ -639,7 +638,7 @@ func TestGetAccount(t *testing.T) {
 
 	resp, err := cl.GetAccount(context.Background(), req)
 
-	checkError(t, err)
+	checkNoError(t, err)
 	assert.IsType(t, &proto.Account{}, resp)
 	assertAccountsSame(t, getAccount("user1"), resp)
 
@@ -656,7 +655,7 @@ func TestDeleteAccount(t *testing.T) {
 	cl := proto.NewAccountsService("com.owncloud.api.accounts", client)
 
 	resp, err := cl.DeleteAccount(context.Background(), req)
-	checkError(t, err)
+	checkNoError(t, err)
 	assert.IsType(t, resp, &empty.Empty{})
 
 	// Check the account doesn't exists anymore
@@ -674,7 +673,7 @@ func TestListGroups(t *testing.T) {
 	cl := proto.NewGroupsService("com.owncloud.api.accounts", client)
 
 	resp, err := cl.ListGroups(context.Background(), req)
-	checkError(t, err)
+	checkNoError(t, err)
 	assert.IsType(t, &proto.ListGroupsResponse{}, resp)
 	assert.Equal(t, len(resp.Groups), 9)
 
@@ -717,7 +716,7 @@ func TestGetGroups(t *testing.T) {
 		req := &proto.GetGroupRequest{Id: group.Id}
 		resp, err := cl.GetGroup(context.Background(), req)
 
-		checkError(t, err)
+		checkNoError(t, err)
 		assert.IsType(t, &proto.Group{}, resp)
 		assertGroupsSame(t, group, resp)
 	}
@@ -732,7 +731,7 @@ func TestCreateGroup(t *testing.T) {
 	}}
 
 	res, err := createGroup(t, group)
-	checkError(t, err)
+	checkNoError(t, err)
 
 	assert.IsType(t, &proto.Group{}, res)
 
@@ -754,7 +753,7 @@ func TestGetGroupInvalidID(t *testing.T) {
 	assert.IsType(t, &proto.Group{}, resp)
 	assert.Empty(t, resp)
 	assert.Error(t, err)
-	assert.Equal(t, "{\"id\":\".\",\"code\":404,\"detail\":\"could not read group: open accounts-store/groups/42: no such file or directory\",\"status\":\"Not Found\"}", err.Error())
+	assert.Equal(t, merrors.NotFound(".", "could not read group: open accounts-store/groups/42: no such file or directory"), err)
 	cleanUp(t)
 }
 
@@ -772,12 +771,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)
+	checkNoError(t, err)
 
 	req = &proto.DeleteGroupRequest{Id: grp2.Id}
 	res, err = cl.DeleteGroup(context.Background(), req)
 	assert.IsType(t, res, &empty.Empty{})
-	checkError(t, err)
+	checkNoError(t, err)
 
 	groupsResponse := listGroups(t)
 	assertResponseNotContainsGroup(t, groupsResponse, grp1)
@@ -804,11 +803,7 @@ func TestDeleteGroupNotExisting(t *testing.T) {
 		assert.IsType(t, &empty.Empty{}, res)
 		assert.Empty(t, res)
 		assert.Error(t, err)
-		assert.Equal(
-			t,
-			fmt.Sprintf("{\"id\":\".\",\"code\":404,\"detail\":\"could not read group: open accounts-store/groups/%v: no such file or directory\",\"status\":\"Not Found\"}", id),
-			err.Error(),
-		)
+		assert.Equal(t, merrors.NotFound(".", "could not read group: open accounts-store/groups/%v: no such file or directory", id), err)
 	}
 	cleanUp(t)
 }
@@ -832,11 +827,7 @@ func TestDeleteGroupInvalidId(t *testing.T) {
 		assert.IsType(t, &empty.Empty{}, res)
 		assert.Empty(t, res)
 		assert.Error(t, err)
-		assert.Equal(
-			t,
-			fmt.Sprintf("{\"id\":\".\",\"code\":500,\"detail\":\"could not clean up group id: invalid id %v\",\"status\":\"Internal Server Error\"}", val),
-			err.Error(),
-		)
+		assert.Equal(t, merrors.InternalServerError(".", "could not clean up group id: invalid id %v", val), err)
 	}
 	cleanUp(t)
 }
@@ -859,11 +850,7 @@ func TestUpdateGroup(t *testing.T) {
 	assert.IsType(t, &proto.Group{}, res)
 	assert.Empty(t, res)
 	assert.Error(t, err)
-	assert.Equal(
-		t,
-		"{\"id\":\".\",\"code\":500,\"detail\":\"not implemented\",\"status\":\"Internal Server Error\"}",
-		err.Error(),
-	)
+	assert.Equal(t, merrors.InternalServerError(".", "not implemented"), err)
 	cleanUp(t)
 }
 
@@ -884,7 +871,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)
+	checkNoError(t, err)
 
 	assert.IsType(t, &proto.Group{}, res)
 
@@ -918,7 +905,7 @@ func TestAddMemberAlreadyInGroup(t *testing.T) {
 	res, err := cl.AddMember(context.Background(), req)
 
 	// Should Give Error
-	checkError(t, err)
+	checkNoError(t, err)
 	assert.IsType(t, &proto.Group{}, res)
 	//assert.Equal(t, proto.Group{}, *res)
 	//assertGroupsSame(t, updatedGroup, res)
@@ -953,11 +940,7 @@ func TestAddMemberNonExisting(t *testing.T) {
 		assert.IsType(t, &proto.Group{}, res)
 		assert.Empty(t, res)
 		assert.Error(t, err)
-		assert.Equal(
-			t,
-			fmt.Sprintf("{\"id\":\".\",\"code\":404,\"detail\":\"could not read account: open accounts-store/accounts/%v: no such file or directory\",\"status\":\"Not Found\"}", id),
-			err.Error(),
-		)
+		assert.Equal(t, merrors.NotFound(".", "could not read account: open accounts-store/accounts/%v: no such file or directory", id), err)
 	}
 
 	// Check group is not changed
@@ -994,7 +977,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)
+	checkNoError(t, err)
 
 	assert.IsType(t, &proto.Group{}, res)
 	//assert.Equal(t, proto.Group{}, *res)
@@ -1029,11 +1012,7 @@ func TestRemoveMemberNonExistingUser(t *testing.T) {
 		assert.IsType(t, &proto.Group{}, res)
 		assert.Empty(t, res)
 		assert.Error(t, err)
-		assert.Equal(
-			t,
-			fmt.Sprintf("{\"id\":\".\",\"code\":404,\"detail\":\"could not read account: open accounts-store/accounts/%v: no such file or directory\",\"status\":\"Not Found\"}", id),
-			err.Error(),
-		)
+		assert.Equal(t, merrors.NotFound(".", "could not read account: open accounts-store/accounts/%v: no such file or directory", id), err)
 	}
 
 	// Check group is not changed
@@ -1058,15 +1037,11 @@ func TestRemoveMemberNotInGroup(t *testing.T) {
 	res, err := cl.RemoveMember(context.Background(), req)
 
 	// Should give an error
-	checkError(t, err)
+	checkNoError(t, err)
 	assert.IsType(t, &proto.Group{}, res)
 
 	//assert.Error(t, err)
-	//assert.Equal(
-	//	t,
-	//	fmt.Sprintf("{\"id\":\".\",\"code\":404,\"detail\":\"User not found in the group\",\"status\":\"Not Found\"}", account.Id),
-	//	err.Error(),
-	//)
+	//assert.Equal(t, merrors.NotFound(".", "User not found in the group"), err)
 
 	// Check group is not changed
 	resp := listGroups(t)
@@ -1095,7 +1070,7 @@ func TestListMembers(t *testing.T) {
 		req := &proto.ListMembersRequest{Id: expectedGroup.Id}
 
 		res, err := cl.ListMembers(context.Background(), req)
-		checkError(t, err)
+		checkNoError(t, err)
 
 		assert.Equal(t, len(res.Members), len(expectedGroup.Members))
 
@@ -1128,7 +1103,7 @@ func TestListMembersEmptyGroup(t *testing.T) {
 
 	res, err := cl.ListMembers(context.Background(), req)
 
-	checkError(t, err)
+	checkNoError(t, err)
 	assert.Empty(t, res.Members)
 
 	cleanUp(t)
@@ -1149,7 +1124,7 @@ func TestAccountUpdateMask(t *testing.T) {
 
 	cl := proto.NewAccountsService("com.owncloud.api.accounts", client)
 	res, err := cl.UpdateAccount(context.Background(), req)
-	checkError(t, err)
+	checkNoError(t, err)
 
 	assert.Equal(t, "ShouldBeUpdated", res.DisplayName)
 	assert.Equal(t, user1.PreferredName, res.PreferredName)

pkg/service/v0/accounts.go

@@ -157,6 +157,41 @@ func (s Service) passwordIsValid(hash string, pwd string) (ok bool) {
 	return c.Verify(hash, []byte(pwd)) == nil
 }
 
+func (s Service) accountExists(ctx context.Context, username, mail, id string) (exists bool, err error) {
+	// only search for accounts
+	tq := bleve.NewTermQuery("account")
+	tq.SetField("bleve_type")
+	query := bleve.NewConjunctionQuery(tq)
+
+	// parse the query like an odata filter
+	var q *godata.GoDataFilterQuery
+	queryUsername := fmt.Sprintf("on_premises_sam_account_name eq '%s'", username)
+	queryMail := fmt.Sprintf("mail eq '%s'", mail)
+	queryID := fmt.Sprintf("id eq '%s'", id)
+	if q, err = godata.ParseFilterString(queryUsername + " or " + queryMail + " or " + queryID); err != nil {
+		s.log.Error().Err(err).Msg("could not parse query")
+		return false, merrors.InternalServerError(s.id, "could not parse query: %v", err.Error())
+	}
+
+	// convert to bleve query
+	bq, err := provider.BuildBleveQuery(q)
+	if err != nil {
+		s.log.Error().Err(err).Msg("could not build bleve query")
+		return false, merrors.InternalServerError(s.id, "could not build bleve query: %v", err.Error())
+	}
+	query.AddQuery(bq)
+
+	searchRequest := bleve.NewSearchRequest(query)
+	var searchResult *bleve.SearchResult
+	searchResult, err = s.index.Search(searchRequest)
+	if err != nil {
+		s.log.Error().Err(err).Msg("could not execute bleve search")
+		return false, merrors.InternalServerError(s.id, "could not execute bleve search: %v", err.Error())
+	}
+
+	return searchResult.Total > 0, nil
+}
+
 func (s Service) hasAccountManagementPermissions(ctx context.Context) bool {
 	// get roles from context
 	roleIDs, ok := roles.ReadRoleIDsFromContext(ctx)
@@ -327,6 +362,14 @@ func (s Service) CreateAccount(ctx context.Context, in *proto.CreateAccountReque
 		return merrors.InternalServerError(s.id, "could not clean up account id: %v", err.Error())
 	}
 
+	exists, err := s.accountExists(ctx, acc.PreferredName, acc.Mail, acc.Id)
+	if err != nil {
+		return merrors.InternalServerError(s.id, "could not check if account exists: %v", err.Error())
+	}
+	if exists {
+		return merrors.BadRequest(s.id, "account already exists")
+	}
+
 	if acc.PasswordProfile != nil {
 		if acc.PasswordProfile.Password != "" {
 			// encrypt password