This looks great to me, but there's no test results from CI?

Hm, this doesn't seem to be used for anything (it's filled but we don't seem to do anything with its content) + I'm not sure I understand what we're trying to do :)

Since the next release is 5.0, I think we should make it abstract and document this in the UPGRADING doc clearly.

The content of a JSON file could be a simple boolean?

Should we return something?

Yes, it's either a boolean or a set of more complicated branch-specific rules.

Should we deny by default otherwise?

Pretty sure you should be here ;-)

No, I've removed this, as this was a test during debugging the tests.

Maybe I'm looking wrong, but I can't find any docs for the TEST_AUTH_STATUS configuration variable and I can't figure out what the file that it points to is supposed to look like. I think this should be documented.

not there there is a self.get_csrf() method you can use as well

Maybe I'm looking wrong, but I can't find any docs for the TEST_AUTH_STATUS configuration variable and I can't figure out what the file that it points to is supposed to look like. I think this should be documented.

The TEST_AUTH_STATUS is supposed to only be used during testing, and not in any production deployments.
If it's not set, it will do the exact same thing as previously.

Except that this is an external plugin for some people (like src.fp.o), and this would entirely break upgrading until they fix that.
I personally think that we should not break backwards compatibility for external APIs that trivially, but if you still prefer to just make it abstract, I can.

Could we add a test where this is False and the PR fails as well?

That happens automatically, because if nothing is returned, it denies.

ok, I see it now :)

That would be the same as the "all_denied" test?
Note that I'm not trying to test the test_auth plugin itself.

Oh, I see. So there's no default implementation of check_acl, right?

Oh, I see. So there's no default implementation of check_acl, right?

There is one: one that raises NotImplementedError.
But because is_dynamic = False is the default in the GitAuthHelper base class, it won't get called unless a subclass sets that property to True.

That would be the same as the "all_denied" test?

It would be very similar, but just via a PR rather than an online edit

But the codepath in the auth hook is the exact same?

Tests are passing locally, let's merge :)