[PATCH 4/4] promisor-remote: allow a client to check extra fields

[Christian Couder <christian.couder@gmail.com>]

A previous commit allowed a server to pass extra fields through the
"promisor-remote" protocol capability after the "name" and "url"
fields.

Let's make it possible for a client to check if these extra fields
match what it expects before accepting a promisor remote.

We allow this by introducing a new "promisor.checkExtraFields"
configuration variable. It should contain a comma or space
separated list of fields that will be checked.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config/promisor.adoc    |  17 +++++
 promisor-remote.c                     | 102 +++++++++++++++++++++++---
 t/t5710-promisor-remote-capability.sh |  35 +++++++++
 3 files changed, 143 insertions(+), 11 deletions(-)

diff --git a/Documentation/config/promisor.adoc b/Documentation/config/promisor.adoc
index bc08999733..75207de2ac 100644
--- a/Documentation/config/promisor.adoc
+++ b/Documentation/config/promisor.adoc
@@ -43,3 +43,20 @@ promisor.acceptFromServer::
 	lazily fetchable from this promisor remote from its responses
 	to "fetch" and "clone" requests from the client. Name and URL
 	comparisons are case sensitive. See linkgit:gitprotocol-v2[5].
+
+promisor.checkExtraFields::
+	A comma or space separated list of additional remote related
+	fields that a client will check before accepting a promisor
+	remote. When a field named "bar" is part of this list and a
+	corresponding "remote.foo.bar" config variable is set for
+	locally for remote "foo", then the value of the
+	"remote.foo.bar" config variable will be checked against the
+	value of the "bar" field passed by the server through the
+	"promisor-remote" capability for the remote "foo". The remote
+	"foo" will be rejected if the values don't match. The fields
+	should be passed by the server through the "promisor-remote"
+	capability by using the `promisor.sendExtraFields` config
+	variable, see above. The extra fields will be checked only if
+	the `promisor.acceptFromServer` config variable, see above, is
+	not set to "None". If is set to "None", this config variable
+	will have no effect. See linkgit:gitprotocol-v2[5].
diff --git a/promisor-remote.c b/promisor-remote.c
index 424d88d4a1..3645093f2f 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -358,6 +358,19 @@ static struct string_list *extra_fields_sent(void)
 	return &fields_list;
 }
 
+static struct string_list *extra_fields_checked(void)
+{
+	static struct string_list fields_list = STRING_LIST_INIT_NODUP;
+	static int initialized = 0;
+
+	if (!initialized) {
+		fields_from_config(&fields_list, "promisor.checkExtraFields");
+		initialized = 1;
+	}
+
+	return &fields_list;
+}
+
 static void append_extra_fields(struct string_list *fields,
 				struct string_list *extra_fields,
 				const char *name)
@@ -497,15 +510,65 @@ enum accept_promisor {
 	ACCEPT_ALL
 };
 
+static int check_extra_field_one(struct string_list_item *item_value,
+				 struct promisor_info *p)
+{
+	struct string_list_item *item;
+
+	item = unsorted_string_list_lookup(&p->fields, item_value->string);
+	if (!item)
+		return 0;
+
+	return !strcmp(item->util, item_value->util);
+}
+
+
+static int check_extra_field(struct string_list_item *item_value,
+			     struct promisor_info *p, int in_list)
+{
+	if (!in_list)
+		return check_extra_field_one(item_value, p);
+
+	for (; p; p = p->next)
+		if (check_extra_field_one(item_value, p))
+			return 1;
+
+	return 0;
+}
+
+static int check_all_extra_fields(struct string_list* extra_values,
+				  struct promisor_info *p,
+				  int in_list)
+{
+	struct string_list* fields_checked = extra_fields_checked();
+	struct string_list_item *item_checked;
+
+	string_list_sort(extra_values);
+
+	for_each_string_list_item(item_checked, fields_checked) {
+		struct string_list_item *item_value;
+
+		item_value = string_list_lookup(extra_values, item_checked->string);
+		if (!item_value)
+			return 0;
+		if (!check_extra_field(item_value, p, in_list))
+			return 0;
+	}
+
+	return 1;
+}
+
 static int should_accept_remote(enum accept_promisor accept,
-				const char *remote_name, const char *remote_url,
+				const char *remote_name,
+				const char *remote_url,
+				struct string_list* extras,
 				struct promisor_info *info_list)
 {
 	struct promisor_info *p;
 	const char *local_url;
 
 	if (accept == ACCEPT_ALL)
-		return 1;
+		return check_all_extra_fields(extras, info_list, 1);
 
 	p = remote_nick_find(info_list, remote_name);
 
@@ -514,7 +577,7 @@ static int should_accept_remote(enum accept_promisor accept,
 		return 0;
 
 	if (accept == ACCEPT_KNOWN_NAME)
-		return 1;
+		return check_all_extra_fields(extras, p, 0);
 
 	if (accept != ACCEPT_KNOWN_URL)
 		BUG("Unhandled 'enum accept_promisor' value '%d'", accept);
@@ -530,7 +593,7 @@ static int should_accept_remote(enum accept_promisor accept,
 	local_url = p->fields.items[1].util;
 
 	if (!strcmp(local_url, remote_url))
-		return 1;
+		return check_all_extra_fields(extras, p, 0);
 
 	warning(_("known remote named '%s' but with URL '%s' instead of '%s'"),
 		remote_name, local_url, remote_url);
@@ -564,9 +627,6 @@ static void filter_promisor_remote(struct repository *repo,
 	if (accept == ACCEPT_NONE)
 		return;
 
-	if (accept != ACCEPT_ALL)
-		info_list = promisor_info_list(repo, NULL);
-
 	/* Parse remote info received */
 
 	remotes = strbuf_split_str(info, ';', 0);
@@ -577,14 +637,27 @@ static void filter_promisor_remote(struct repository *repo,
 		const char *remote_url = NULL;
 		char *decoded_name = NULL;
 		char *decoded_url = NULL;
+		struct string_list extras = STRING_LIST_INIT_NODUP;
 
 		strbuf_strip_suffix(remotes[i], ";");
 		elems = strbuf_split(remotes[i], ',');
 
 		for (size_t j = 0; elems[j]; j++) {
+			char *p;
+
 			strbuf_strip_suffix(elems[j], ",");
-			if (!skip_prefix(elems[j]->buf, "name=", &remote_name))
-				skip_prefix(elems[j]->buf, "url=", &remote_url);
+			if (skip_prefix(elems[j]->buf, "name=", &remote_name) ||
+			    skip_prefix(elems[j]->buf, "url=", &remote_url))
+				continue;
+
+			p = strchr(elems[j]->buf, '=');
+			if (p) {
+				*p = '\0';
+				string_list_append(&extras, elems[j]->buf)->util = p + 1;
+			} else {
+				warning(_("invalid element '%s' from remote info"),
+					elems[j]->buf);
+			}
 		}
 
 		if (remote_name)
@@ -592,9 +665,16 @@ static void filter_promisor_remote(struct repository *repo,
 		if (remote_url)
 			decoded_url = url_percent_decode(remote_url);
 
-		if (decoded_name && should_accept_remote(accept, decoded_name, decoded_url, info_list))
-			strvec_push(accepted, decoded_name);
+		if (decoded_name) {
+			if (!info_list)
+				info_list = promisor_info_list(repo, extra_fields_checked());
+
+			if (should_accept_remote(accept, decoded_name, decoded_url,
+						 &extras, info_list))
+				strvec_push(accepted, decoded_name);
+		}
 
+		string_list_clear(&extras, 0);
 		strbuf_list_free(elems);
 		free(decoded_name);
 		free(decoded_url);
diff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh
index 26f3c63112..858b34587d 100755
--- a/t/t5710-promisor-remote-capability.sh
+++ b/t/t5710-promisor-remote-capability.sh
@@ -321,6 +321,41 @@ test_expect_success "clone with promisor.sendExtraFields" '
 	check_missing_objects server 1 "$oid"
 '
 
+test_expect_success "clone with promisor.checkExtraFields" '
+	git -C server config promisor.advertise true &&
+	test_when_finished "rm -rf client" &&
+
+	git -C server remote add otherLop "https://invalid.invalid"  &&
+	git -C server config remote.otherLop.id "fooBar" &&
+	git -C server config remote.otherLop.stuff "baz" &&
+	git -C server config remote.otherLop.partialCloneFilter "blob:limit=10k" &&
+	test_when_finished "git -C server remote remove otherLop" &&
+	git -C server config promisor.sendExtraFields "id, partialCloneFilter" &&
+	test_when_finished "git -C server config unset promisor.sendExtraFields" &&
+	test_when_finished "rm trace" &&
+
+	# Clone from server to create a client
+	GIT_TRACE_PACKET="$(pwd)/trace" GIT_NO_LAZY_FETCH=0 git clone \
+		-c remote.lop.promisor=true \
+		-c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \
+		-c remote.lop.url="file://$(pwd)/lop" \
+		-c remote.lop.partialCloneFilter="blob:none" \
+		-c promisor.acceptfromserver=All \
+		-c promisor.checkExtraFields=partialCloneFilter \
+		--no-local --filter="blob:limit=5k" server client &&
+
+	# Check that extra fields are properly transmitted
+	ENCODED_URL=$(echo "file://$(pwd)/lop" | sed -e "s/ /%20/g") &&
+	PR1="name=lop,url=$ENCODED_URL,partialCloneFilter=blob:none" &&
+	PR2="name=otherLop,url=https://invalid.invalid,id=fooBar,partialCloneFilter=blob:limit=10k" &&
+	test_grep "clone< promisor-remote=$PR1;$PR2" trace &&
+	test_grep "clone> promisor-remote=lop" trace &&
+	test_grep ! "clone> promisor-remote=lop;otherLop" trace &&
+
+	# Check that the largest object is still missing on the server
+	check_missing_objects server 1 "$oid"
+'
+
 test_expect_success "clone with promisor.advertise set to 'true' but don't delete the client" '
 	git -C server config promisor.advertise true &&
 
-- 
2.49.0.158.g6ac6832dc3.dirty