credential-cache: respect request capabilities
PATCH 1/1 credential-cache: respect request capabilities
From: M Hickford <[email protected]>
Previously, credential-cache responded with capability[]=authtype regardless of request.
The capabilities in a credential helper response should be a subset of the capabilities in the request.
---
credential-cache: respect request capabilities
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1842%2Fhickford%2Fcache-capability-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1842/hickford/cache-capability-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1842
builtin/credential-cache--daemon.c | 11 +++++------ t/lib-credential.sh | 15 +++++++++++++++ t/t0303-credential-external.sh | 1 + 3 files changed, 21 insertions(+), 6 deletions(-)
builtin/credential-cache--daemon.c +5 -6
@@ -134,17 +134,16 @@ static void serve_one_client(FILE *in, FILE *out)else if (!strcmp (action.buf,"get" )) {struct credential_cache_entry *e =lookup_credential (&c);if (e) { - e->item.capa_authtype.request_initial =1 ; - e->item.capa_authtype.request_helper =1 ; - -fprintf (out,"capability[]=authtype\n" ); +if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_RESPONSE )) { +fprintf (out,"capability[]=authtype\n" ); + }if (e->item.username)fprintf (out,"username=%s\n" , e->item.username);if (e->item.password)fprintf (out,"password=%s\n" , e->item.password); -if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_HELPER ) && e->item.authtype) +if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_RESPONSE ) && e->item.authtype)fprintf (out,"authtype=%s\n" , e->item.authtype); -if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_HELPER ) && e->item.credential) +if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_RESPONSE ) && e->item.credential)fprintf (out,"credential=%s\n" , e->item.credential);if (e->item.password_expiry_utc !=TIME_MAX ) fprintf(out, "password_expiry_utc=%"PRItime"\n",
t/lib-credential.sh +15 -0
@@ -566,6 +566,21 @@ helper_test_authtype() { EOF ' + test_expect_success "helper ($HELPER) does not get authtype and credential without authtype capability" ' + check fill $HELPER <<-\EOF + protocol=https + host=git.example.com + -- + protocol=https + host=git.example.com + username=askpass-username + password=askpass-password + -- + askpass: Username for '\''https://git.example.com'\'': + askpass: Password for '\''https://[email protected]'\'': + EOF + ' + test_expect_success "helper ($HELPER) stores authtype and credential with username" ' check approve $HELPER <<-\EOF capability[]=authtype
t/t0303-credential-external.sh +1 -0
@@ -63,6 +63,7 @@ helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER" helper_test "$GIT_TEST_CREDENTIAL_HELPER" helper_test_password_expiry_utc "$GIT_TEST_CREDENTIAL_HELPER" helper_test_oauth_refresh_token "$GIT_TEST_CREDENTIAL_HELPER" +helper_test_authtype "$GIT_TEST_CREDENTIAL_HELPER" if test -z "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"; then say "# skipping timeout tests (GIT_TEST_CREDENTIAL_HELPER_TIMEOUT not set)"
base-commit: 92999a42db1c5f43f330e4f2bca4026b5b81576f
Signature · 1 line
gitgitgadget
PATCH v2 1/1 credential-cache: respect request capabilities
From: M Hickford <[email protected]>
Previously, credential-cache responded with capability[]=authtype regardless of request.
The capabilities in a credential helper response should be a subset of the capabilities in the request.
---
credential-cache: respect request capabilities
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1842%2Fhickford%2Fcache-capability-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1842/hickford/cache-capability-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1842
Range-diff vs v1:
1: 9197941029f ! 1: 696780d4782 credential-cache: respect request capabilities
@@ t/lib-credential.sh: helper_test_authtype() {
EOF
'
-+ test_expect_success "helper ($HELPER) does not get authtype and credential without authtype capability" '
++ test_expect_success "helper ($HELPER) get authtype only if request has authtype capability" '
+ check fill $HELPER <<-\EOF
+ protocol=https
+ host=git.example.com
@@ t/lib-credential.sh: helper_test_authtype() {
test_expect_success "helper ($HELPER) stores authtype and credential with username" '
check approve $HELPER <<-\EOF
capability[]=authtype
-
- ## t/t0303-credential-external.sh ##
-@@ t/t0303-credential-external.sh: helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
- helper_test "$GIT_TEST_CREDENTIAL_HELPER"
- helper_test_password_expiry_utc "$GIT_TEST_CREDENTIAL_HELPER"
- helper_test_oauth_refresh_token "$GIT_TEST_CREDENTIAL_HELPER" -+helper_test_authtype "$GIT_TEST_CREDENTIAL_HELPER"
-
- if test -z "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"; then
- say "# skipping timeout tests (GIT_TEST_CREDENTIAL_HELPER_TIMEOUT not set)"
builtin/credential-cache--daemon.c | 11 +++++------ t/lib-credential.sh | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 6 deletions(-)
builtin/credential-cache--daemon.c +5 -6
@@ -134,17 +134,16 @@ static void serve_one_client(FILE *in, FILE *out)else if (!strcmp (action.buf,"get" )) {struct credential_cache_entry *e =lookup_credential (&c);if (e) { - e->item.capa_authtype.request_initial =1 ; - e->item.capa_authtype.request_helper =1 ; - -fprintf (out,"capability[]=authtype\n" ); +if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_RESPONSE )) { +fprintf (out,"capability[]=authtype\n" ); + }if (e->item.username)fprintf (out,"username=%s\n" , e->item.username);if (e->item.password)fprintf (out,"password=%s\n" , e->item.password); -if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_HELPER ) && e->item.authtype) +if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_RESPONSE ) && e->item.authtype)fprintf (out,"authtype=%s\n" , e->item.authtype); -if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_HELPER ) && e->item.credential) +if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_RESPONSE ) && e->item.credential)fprintf (out,"credential=%s\n" , e->item.credential);if (e->item.password_expiry_utc !=TIME_MAX ) fprintf(out, "password_expiry_utc=%"PRItime"\n",
t/lib-credential.sh +15 -0
@@ -566,6 +566,21 @@ helper_test_authtype() { EOF ' + test_expect_success "helper ($HELPER) get authtype only if request has authtype capability" ' + check fill $HELPER <<-\EOF + protocol=https + host=git.example.com + -- + protocol=https + host=git.example.com + username=askpass-username + password=askpass-password + -- + askpass: Username for '\''https://git.example.com'\'': + askpass: Password for '\''https://[email protected]'\'': + EOF + ' + test_expect_success "helper ($HELPER) stores authtype and credential with username" ' check approve $HELPER <<-\EOF capability[]=authtype
base-commit: 92999a42db1c5f43f330e4f2bca4026b5b81576f
Signature · 1 line
gitgitgadget
Quoted 4 lines · M Hickford via GitGitGadget
From: M Hickford <[email protected]>
Previously, credential-cache responded with capability[]=authtype regardless of request.
That's the correct behaviour.
Quoted 2 lines
The capabilities in a credential helper response should be a subset of the capabilities in the request.
No, it should not. Otherwise, it's impossible for Git to know whether the helper does or does not support the capability. We rely on that information to correctly pass data back when saving data.
Quoted 5 lines
builtin/credential-cache--daemon.c +0
-0
builtin/credential-cache--daemon.c static void serve_one_client(FILE *in, FILE *out) · line 139
Quoted 10 lines
builtin/credential-cache--daemon.c +3
-4@@ -134,17 +134,16 @@ static void serve_one_client(FILE *in, FILE *out)else if (!strcmp (action.buf,"get" )) {struct credential_cache_entry *e =lookup_credential (&c);if (e) { - e->item.capa_authtype.request_initial =1 ; - e->item.capa_authtype.request_helper =1 ; - -fprintf (out,"capability[]=authtype\n" ); +if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_RESPONSE )) { +fprintf (out,"capability[]=authtype\n" ); + }
This part is not correct.
builtin/credential-cache--daemon.c static void serve_one_client(FILE *in, FILE *out) · line 146
Quoted 9 lines
builtin/credential-cache--daemon.c +2
-2if (e->item.username)fprintf (out,"username=%s\n" , e->item.username);if (e->item.password)fprintf (out,"password=%s\n" , e->item.password); -if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_HELPER ) && e->item.authtype) +if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_RESPONSE ) && e->item.authtype)fprintf (out,"authtype=%s\n" , e->item.authtype); -if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_HELPER ) && e->item.credential) +if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_RESPONSE ) && e->item.credential)
This part may very well be correct.
Quoted 33 lines
builtin/credential-cache--daemon.c +0
-0fprintf (out,"credential=%s\n" , e->item.credential);if (e->item.password_expiry_utc !=TIME_MAX ) fprintf(out, "password_expiry_utc=%"PRItime"\n",t/lib-credential.sh +15
-0@@ -566,6 +566,21 @@ helper_test_authtype() { EOF ' + test_expect_success "helper ($HELPER) get authtype only if request has authtype capability" ' + check fill $HELPER <<-\EOF + protocol=https + host=git.example.com + -- + protocol=https + host=git.example.com + username=askpass-username + password=askpass-password + -- + askpass: Username for '\''https://git.example.com'\'': + askpass: Password for '\''https://[email protected]'\'': + EOF + ' + test_expect_success "helper ($HELPER) stores authtype and credential with username" ' check approve $HELPER <<-\EOF capability[]=authtypebase-commit:
92999a42db1c5f43f330e4f2bca4026b5b81576fSignature · 1 line
gitgitgadget
Signature · 2 lines
brian m. carlson (they/them or he/him) Toronto, Ontario, CA
Quoted 25 lines · brian m. carlson
Quoted 4 lines · M Hickford via GitGitGadget
From: M Hickford <[email protected]>
Previously, credential-cache responded with capability[]=authtype regardless of request.
That's the correct behaviour.
Quoted 2 lines
The capabilities in a credential helper response should be a subset of the capabilities in the request.
No, it should not. Otherwise, it's impossible for Git to know whether the helper does or does not support the capability. We rely on that information to correctly pass data back when saving data.
Quoted 9 lines
builtin/credential-cache--daemon.c +0
-1@@ -134,17 +134,16 @@ static void serve_one_client(FILE *in, FILE *out)else if (!strcmp (action.buf,"get" )) {struct credential_cache_entry *e =lookup_credential (&c);if (e) { - e->item.capa_authtype.request_initial =1 ;
Quoted 8 lines
builtin/credential-cache--daemon.c static void serve_one_client(FILE *in, FILE *out) · line 139
Quoted 6 lines
builtin/credential-cache--daemon.c +3
-3- e->item.capa_authtype.request_helper =1 ; - -fprintf (out,"capability[]=authtype\n" ); +if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_RESPONSE )) { +fprintf (out,"capability[]=authtype\n" ); + }This part is not correct.
Thanks for the review. I'll revert this part and amend the commit message.
Quoted 50 lines
builtin/credential-cache--daemon.c static void serve_one_client(FILE *in, FILE *out) · line 146
Quoted 10 lines
builtin/credential-cache--daemon.c +2
-2if (e->item.username)fprintf (out,"username=%s\n" , e->item.username);if (e->item.password)fprintf (out,"password=%s\n" , e->item.password); -if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_HELPER ) && e->item.authtype) +if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_RESPONSE ) && e->item.authtype)fprintf (out,"authtype=%s\n" , e->item.authtype); -if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_HELPER ) && e->item.credential) +if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_RESPONSE ) && e->item.credential)This part may very well be correct.
Quoted 30 lines
builtin/credential-cache--daemon.c +0
-0fprintf (out,"credential=%s\n" , e->item.credential);if (e->item.password_expiry_utc !=TIME_MAX ) fprintf(out, "password_expiry_utc=%"PRItime"\n",t/lib-credential.sh +15
-0@@ -566,6 +566,21 @@ helper_test_authtype() { EOF ' + test_expect_success "helper ($HELPER) get authtype only if request has authtype capability" ' + check fill $HELPER <<-\EOF + protocol=https + host=git.example.com + -- + protocol=https + host=git.example.com + username=askpass-username + password=askpass-password + -- + askpass: Username for '\''https://git.example.com'\'': + askpass: Password for '\''https://[email protected]'\'': + EOF + ' + test_expect_success "helper ($HELPER) stores authtype and credential with username" ' check approve $HELPER <<-\EOF capability[]=authtypeQuoted 3 lines
base-commit:
92999a42db1c5f43f330e4f2bca4026b5b81576f--
gitgitgadget--
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA
Quoted 23 lines · M Hickford
Quoted 12 lines · brian m. carlson
Quoted 10 lines · M Hickford via GitGitGadget
From: M Hickford <[email protected]>
builtin/credential-cache--daemon.c +0
-1@@ -134,17 +134,16 @@ static void serve_one_client(FILE *in, FILE *out)else if (!strcmp (action.buf,"get" )) {struct credential_cache_entry *e =lookup_credential (&c);if (e) { - e->item.capa_authtype.request_initial =1 ;Quoted 8 lines
builtin/credential-cache--daemon.c static void serve_one_client(FILE *in, FILE *out) · line 139
Quoted 6 lines
builtin/credential-cache--daemon.c +3
-3- e->item.capa_authtype.request_helper =1 ; - -fprintf (out,"capability[]=authtype\n" ); +if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_RESPONSE )) { +fprintf (out,"capability[]=authtype\n" ); + }This part is not correct.
Quoted 1 line
Thanks for the review. I'll revert this part and amend the commit message.
I applied this without that change and it does still pass the test, which I think is good and shows that can be omitted. If I have some time, I may send a follow-up patch to add some additional tests.
Signature · 2 lines
brian m. carlson (they/them or he/him) Toronto, Ontario, CA
PATCH v3 1/1 credential-cache: respect request capabilities
In reply to M Hickford via GitGitGadgetFrom: M Hickford <[email protected]>
Previously, credential-cache populated authtype regardless of request.
---
credential-cache: respect request capabilities
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1842%2Fhickford%2Fcache-capability-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1842/hickford/cache-capability-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1842
Range-diff vs v2:
1: 696780d4782 ! 1: e9851c5c4ac credential-cache: respect request capabilities
@@ Metadata
## Commit message ##
credential-cache: respect request capabilities
- Previously, credential-cache responded with capability[]=authtype
- regardless of request.
-
- The capabilities in a credential helper response should be a subset of
- the capabilities in the request.
+ Previously, credential-cache populated authtype regardless of request.
Signed-off-by: M Hickford <[email protected]>
## builtin/credential-cache--daemon.c ##
@@ builtin/credential-cache--daemon.c: static void serve_one_client(FILE *in, FILE *out)
- else if (!strcmp(action.buf, "get")) {
- struct credential_cache_entry *e = lookup_credential(&c);
- if (e) {
-- e->item.capa_authtype.request_initial = 1;
-- e->item.capa_authtype.request_helper = 1;
--
-- fprintf(out, "capability[]=authtype\n");
-+ if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE)) {
-+ fprintf(out, "capability[]=authtype\n");
-+ }
- if (e->item.username)
fprintf(out, "username=%s\n", e->item.username);
if (e->item.password)
fprintf(out, "password=%s\n", e->item.password);
@@ t/lib-credential.sh: helper_test_authtype() {
EOF
'
-+ test_expect_success "helper ($HELPER) get authtype only if request has authtype capability" '
++ test_expect_success "helper ($HELPER) gets authtype and credential only if request has authtype capability" '
+ check fill $HELPER <<-\EOF
+ protocol=https
+ host=git.example.com
+ --
++ capability[]=authtype
+ protocol=https
+ host=git.example.com
+ username=askpass-usernamebuiltin/credential-cache--daemon.c | 4 ++-- t/lib-credential.sh | 16 ++++++++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-)
builtin/credential-cache--daemon.c +2 -2
@@ -142,9 +142,9 @@ static void serve_one_client(FILE *in, FILE *out)fprintf (out,"username=%s\n" , e->item.username);if (e->item.password)fprintf (out,"password=%s\n" , e->item.password); -if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_HELPER ) && e->item.authtype) +if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_RESPONSE ) && e->item.authtype)fprintf (out,"authtype=%s\n" , e->item.authtype); -if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_HELPER ) && e->item.credential) +if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_RESPONSE ) && e->item.credential)fprintf (out,"credential=%s\n" , e->item.credential);if (e->item.password_expiry_utc !=TIME_MAX ) fprintf(out, "password_expiry_utc=%"PRItime"\n",
t/lib-credential.sh +16 -0
@@ -566,6 +566,22 @@ helper_test_authtype() { EOF ' + test_expect_success "helper ($HELPER) gets authtype and credential only if request has authtype capability" ' + check fill $HELPER <<-\EOF + protocol=https + host=git.example.com + -- + capability[]=authtype + protocol=https + host=git.example.com + username=askpass-username + password=askpass-password + -- + askpass: Username for '\''https://git.example.com'\'': + askpass: Password for '\''https://[email protected]'\'': + EOF + ' + test_expect_success "helper ($HELPER) stores authtype and credential with username" ' check approve $HELPER <<-\EOF capability[]=authtype
base-commit: 92999a42db1c5f43f330e4f2bca4026b5b81576f
Signature · 1 line
gitgitgadget
PATCH v4 1/1 credential-cache: respect request capabilities
From: M Hickford <[email protected]>
Previously, credential-cache populated authtype regardless of request.
---
credential-cache: respect request capabilities
Patch v4 fixes test
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1842%2Fhickford%2Fcache-capability-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1842/hickford/cache-capability-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1842
Range-diff vs v3:
1: e9851c5c4ac ! 1: 23942f9fa47 credential-cache: respect request capabilities
@@ t/lib-credential.sh: helper_test_authtype() {
+ protocol=https
+ host=git.example.com
+ --
-+ capability[]=authtype
+ protocol=https
+ host=git.example.com
+ username=askpass-usernamebuiltin/credential-cache--daemon.c | 4 ++-- t/lib-credential.sh | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-)
builtin/credential-cache--daemon.c +2 -2
@@ -142,9 +142,9 @@ static void serve_one_client(FILE *in, FILE *out)fprintf (out,"username=%s\n" , e->item.username);if (e->item.password)fprintf (out,"password=%s\n" , e->item.password); -if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_HELPER ) && e->item.authtype) +if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_RESPONSE ) && e->item.authtype)fprintf (out,"authtype=%s\n" , e->item.authtype); -if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_HELPER ) && e->item.credential) +if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_RESPONSE ) && e->item.credential)fprintf (out,"credential=%s\n" , e->item.credential);if (e->item.password_expiry_utc !=TIME_MAX ) fprintf(out, "password_expiry_utc=%"PRItime"\n",
t/lib-credential.sh +15 -0
@@ -566,6 +566,21 @@ helper_test_authtype() { EOF ' + test_expect_success "helper ($HELPER) gets authtype and credential only if request has authtype capability" ' + check fill $HELPER <<-\EOF + protocol=https + host=git.example.com + -- + protocol=https + host=git.example.com + username=askpass-username + password=askpass-password + -- + askpass: Username for '\''https://git.example.com'\'': + askpass: Password for '\''https://[email protected]'\'': + EOF + ' + test_expect_success "helper ($HELPER) stores authtype and credential with username" ' check approve $HELPER <<-\EOF capability[]=authtype
base-commit: 92999a42db1c5f43f330e4f2bca4026b5b81576f
Signature · 1 line
gitgitgadget
Quoted 3 lines · M Hickford via GitGitGadget
From: M Hickford <[email protected]>
Previously, credential-cache populated authtype regardless of request.
OK, that may be a correct statement of the fact, but it does not tell readers any of the following:
- If it is a bad thing to populate authtype regardless of request, and if so why?
- What is the (negative) consequence of doing so, if any? What breaks because it populates authtype regardless of request?
- What is the remedy? Instead of unconditionally populating the authtype, how does the new code decide when to populate it and with what value?
- Can there be downsides of fixing this? Are there use cases where this unconditional population of authtype was relied upon?
- Where did the bug come from and what is its fix? We used to look at OP_HELPER to decide when to emit authtype, but the updated code checks OP_RESPONSE, which readers can see in the patch.
It would be nice if the proposed log message helped them by briefly explaining their differences, for example.
which would help future "git log" readers what this fix was about.
Will queue for now, but the log message would want to be a bit more helpful to the readers.
Thanks.
Quoted 73 lines
Signed-off-by M Hickford <[email protected]>---
credential-cache: respect request capabilitiesPatch v4 fixes test
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1842%2Fhickford%2Fcache-capability-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1842/hickford/cache-capability-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1842Range-diff vs v3:
1:
e9851c5c4ac! 1:23942f9fa47credential-cache: respect request capabilities@@ t/lib-credential.sh: helper_test_authtype() { + protocol=https + host=git.example.com + -- -+ capability[]=authtype + protocol=https + host=git.example.com + username=askpass-usernamebuiltin/credential-cache--daemon.c | 4 ++-- t/lib-credential.sh | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-)builtin/credential-cache--daemon.c +2
-2@@ -142,9 +142,9 @@ static void serve_one_client(FILE *in, FILE *out)fprintf (out,"username=%s\n" , e->item.username);if (e->item.password)fprintf (out,"password=%s\n" , e->item.password); -if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_HELPER ) && e->item.authtype) +if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_RESPONSE ) && e->item.authtype)fprintf (out,"authtype=%s\n" , e->item.authtype); -if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_HELPER ) && e->item.credential) +if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_RESPONSE ) && e->item.credential)fprintf (out,"credential=%s\n" , e->item.credential);if (e->item.password_expiry_utc !=TIME_MAX ) fprintf(out, "password_expiry_utc=%"PRItime"\n",t/lib-credential.sh +15
-0@@ -566,6 +566,21 @@ helper_test_authtype() { EOF ' + test_expect_success "helper ($HELPER) gets authtype and credential only if request has authtype capability" ' + check fill $HELPER <<-\EOF + protocol=https + host=git.example.com + -- + protocol=https + host=git.example.com + username=askpass-username + password=askpass-password + -- + askpass: Username for '\''https://git.example.com'\'': + askpass: Password for '\''https://[email protected]'\'': + EOF + ' + test_expect_success "helper ($HELPER) stores authtype and credential with username" ' check approve $HELPER <<-\EOF capability[]=authtypebase-commit:
92999a42db1c5f43f330e4f2bca4026b5b81576f
PATCH v5 1/1 credential-cache: respect authtype capability
In reply to M Hickford via GitGitGadgetFrom: M Hickford <[email protected]>
Previously, credential-cache populated authtype regardless whether "get" request had authtype capability. As documented in git-credential.txt, authtype "should not be sent unless the appropriate capability ... is provided".
Add test. Without this change, the test failed because "credential fill" printed an incomplete credential with only protocol and host attributes (the unexpected authtype attribute was discarded by credential.c).
---
credential-cache: respect request capabilities
Patch v5 adds details to the commit message
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1842%2Fhickford%2Fcache-capability-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1842/hickford/cache-capability-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1842
Range-diff vs v4:
1: 23942f9fa47 ! 1: db575d9d116 credential-cache: respect request capabilities
@@ Metadata
Author: M Hickford <[email protected]>
## Commit message ##
- credential-cache: respect request capabilities
+ credential-cache: respect authtype capability
- Previously, credential-cache populated authtype regardless of request.
+ Previously, credential-cache populated authtype regardless whether
+ "get" request had authtype capability. As documented in
+ git-credential.txt, authtype "should not be sent unless the appropriate
+ capability ... is provided".
+
+ Add test. Without this change, the test failed because "credential fill"
+ printed an incomplete credential with only protocol and host attributes
+ (the unexpected authtype attribute was discarded by credential.c).
builtin/credential-cache--daemon.c | 4 ++-- t/lib-credential.sh | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-)
builtin/credential-cache--daemon.c +2 -2
@@ -142,9 +142,9 @@ static void serve_one_client(FILE *in, FILE *out)fprintf (out,"username=%s\n" , e->item.username);if (e->item.password)fprintf (out,"password=%s\n" , e->item.password); -if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_HELPER ) && e->item.authtype) +if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_RESPONSE ) && e->item.authtype)fprintf (out,"authtype=%s\n" , e->item.authtype); -if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_HELPER ) && e->item.credential) +if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_RESPONSE ) && e->item.credential)fprintf (out,"credential=%s\n" , e->item.credential);if (e->item.password_expiry_utc !=TIME_MAX ) fprintf(out, "password_expiry_utc=%"PRItime"\n",
t/lib-credential.sh +15 -0
@@ -566,6 +566,21 @@ helper_test_authtype() { EOF ' + test_expect_success "helper ($HELPER) gets authtype and credential only if request has authtype capability" ' + check fill $HELPER <<-\EOF + protocol=https + host=git.example.com + -- + protocol=https + host=git.example.com + username=askpass-username + password=askpass-password + -- + askpass: Username for '\''https://git.example.com'\'': + askpass: Password for '\''https://[email protected]'\'': + EOF + ' + test_expect_success "helper ($HELPER) stores authtype and credential with username" ' check approve $HELPER <<-\EOF capability[]=authtype
base-commit: 92999a42db1c5f43f330e4f2bca4026b5b81576f
Signature · 1 line
gitgitgadget
Re: [PATCH v5] credential-cache: respect authtype capability
Quoted 94 lines · M Hickford via GitGitGadget
From: M Hickford <[email protected]>
Previously, credential-cache populated authtype regardless whether "get" request had authtype capability. As documented in git-credential.txt, authtype "should not be sent unless the appropriate capability ... is provided".
Add test. Without this change, the test failed because "credential fill" printed an incomplete credential with only protocol and host attributes (the unexpected authtype attribute was discarded by
credential.c).Signed-off-by M Hickford <[email protected]>---
credential-cache: respect request capabilitiesPatch v5 adds details to the commit message
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1842%2Fhickford%2Fcache-capability-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1842/hickford/cache-capability-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1842Range-diff vs v4:
1:
23942f9fa47! 1:db575d9d116credential-cache: respect request capabilities
@@ Metadata
Author: M Hickford <[email protected]>## Commit message ##
- credential-cache: respect request capabilities
+ credential-cache: respect authtype capability- Previously, credential-cache populated authtype regardless of request.
+ Previously, credential-cache populated authtype regardless whether
+ "get" request had authtype capability. As documented in
+ git-credential.txt, authtype "should not be sent unless the appropriate
+ capability ... is provided".
+
+ Add test. Without this change, the test failed because "credential fill"
+ printed an incomplete credential with only protocol and host attributes
+ (the unexpected authtype attribute was discarded bycredential.c).Signed-off-by M Hickford <[email protected]>builtin/credential-cache--daemon.c | 4 ++-- t/lib-credential.sh | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-)builtin/credential-cache--daemon.c +2
-2@@ -142,9 +142,9 @@ static void serve_one_client(FILE *in, FILE *out)fprintf (out,"username=%s\n" , e->item.username);if (e->item.password)fprintf (out,"password=%s\n" , e->item.password); -if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_HELPER ) && e->item.authtype) +if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_RESPONSE ) && e->item.authtype)fprintf (out,"authtype=%s\n" , e->item.authtype); -if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_HELPER ) && e->item.credential) +if (credential_has_capability (&c.capa_authtype,CREDENTIAL_OP_RESPONSE ) && e->item.credential)fprintf (out,"credential=%s\n" , e->item.credential);if (e->item.password_expiry_utc !=TIME_MAX ) fprintf(out, "password_expiry_utc=%"PRItime"\n",t/lib-credential.sh +15
-0@@ -566,6 +566,21 @@ helper_test_authtype() { EOF ' + test_expect_success "helper ($HELPER) gets authtype and credential only if request has authtype capability" ' + check fill $HELPER <<-\EOF + protocol=https + host=git.example.com + -- + protocol=https + host=git.example.com + username=askpass-username + password=askpass-password + -- + askpass: Username for '\''https://git.example.com'\'': + askpass: Password for '\''https://[email protected]'\'': + EOF + ' + test_expect_success "helper ($HELPER) stores authtype and credential with username" ' check approve $HELPER <<-\EOF capability[]=authtype
Quoted 1 line
base-commit:
92999a42db1c5f43f330e4f2bca4026b5b81576f
Hi Brian. Any further comments on patch v5? This addresses your comments on v2 and expands the commit message as encouraged by Junio. (Thank you both for the review so far.)
https://lore.kernel.org/git/[email protected]/
https://lore.kernel.org/git/[email protected]/
Re: [PATCH v5] credential-cache: respect authtype capability
Quoted 3 lines · M Hickford
Hi Brian. Any further comments on patch v5? This addresses your comments on v2 and expands the commit message as encouraged by Junio. (Thank you both for the review so far.)
I think this looks fine.
Signature · 2 lines
brian m. carlson (they/them or he/him) Toronto, Ontario, CA