Ludovic Courtès writes: > Hi, > > Christopher Baines skribis: > >> Rather than having valid-narinfo? evaluate to #t if >> %allow-unauthenticated-substitutes? is set to #t, just use (const #t) for >> valid-narinfo? when %allow-unauthenticated-substitutes? is set to #t. This >> will allow moving valid-narinfo? in to a (guix substitutes) module. >> >> * guix/scripts/substitute.scm (process-query, process-substitution): Change >> the authorized? argument to lookup-narinfo and lookup-narinfos/diverse based >> on %allow-unauthenticated-substitutes?. >> (valid-narinfo?): Remove use of %allow-unauthenticated-substitutes?. > > Bummer that there are two call sites. > > What about doing away with ‘%allow-unauthenticated-substitutes?’ and > instead changing its only user, ‘tests/substitute.scm’, like so: > > diff --git a/tests/substitute.scm b/tests/substitute.scm > index 542aaf603f..1827ffe8d4 100644 > --- a/tests/substitute.scm > +++ b/tests/substitute.scm > @@ -178,10 +178,10 @@ a file for NARINFO." > (call-with-output-file > (string-append narinfo-directory "/example.nar") > (cute write-file > - (string-append narinfo-directory "/example.out") <>)) > - > - (%allow-unauthenticated-substitutes? #f)) > - thunk > + (string-append narinfo-directory "/example.out") <>))) > + (lambda () > + (mock ((guix narinfo) valid-narinfo?) (const #t) > + (thunk))) > (lambda () > (when (file-exists? cache-directory) > (delete-file-recursively cache-directory)))))) > > That change would have to be made in the patch that creates (guix > narinfo). > > WDYT? I don't know what's up with these tests in particular, adding peek in places makes tests fail... not using Guile debugging helpers and outputting to (current-error-port) seems to not change the result though. I didn't really understand this code, but looking at it more, I'm thinking now that what it actually does is affects all the tests, and for some tests in the (tests substitute) module, the %allow-unauthenticated-substitutes? behaviour is turned off. Commenting out the relevant code in the script seems to support this, the substitute tests still pass, but tests in the store, derivation and guix-daemon modules fail. The substitute tests are actually fine, and break if you disable substitute authentication. The mock approach is probably feasible, but it would need to be done in those modules/tests. I haven't looked at the details, but I'd be a little concerned that it might require mocking in each of the individual 15 failing tests, maybe that's good for being explicit though? Back to the use of %allow-unauthenticated-substitutes? in the code, there are two call sites, for the two separate code paths, but it would be pretty easy to move to one call site. Both process-query and process-substitution take an acl, but they could instead take some (valid? obj) procedure. That would either call (valid-narinfo? obj acl) or just evaluate to #t in the allow unauthorized case. This effectively moves the logic and call site to the command.