Add unit tests for github auth provider

[vit-zikmund]

This PR add 100% (well 99% due to typing clutter) coverage unit tests for the github auth provider.

It fixes a handful of mostly cosmetic things in the original code, that popped up during the work.

One considerably more significant change is the introduction of the authorization "proxy" cache, which ensures that the core giftless code will always get a valid is_authorized() response for the user calling giftless, even if the usual auth proxy is configured too low to withstand some harsh hammering conditions.

Closes: #148

[vit-zikmund]

Hey, @athornton long time no see :) Sorry to keep you busy, but since you proved so much helpful on my previous endeavors... I hope it's pretty much on a sliver plate. All tests clean and tidy. The unit tests aren't hardcore exhaustive, but provide full line&branch coverage.

[athornton]

Ew, but if you need it you need it. Honestly it looks like a bug in threading that something public exposes _RLock.

[vit-zikmund]

Indeed 🤮😞 It's mostly a shutup-treat for mypy which for some reason rejects type(RLock()) as a type. This is the only workaround I found for a dynamic low-level type coming from a factory function RLock(). I'll try to dig a bit more, but my hopes are low.

[vit-zikmund]

Yissss 🐍 I fixed this with a little protocol instead.

[athornton]

Cool! Now I've actually seen Protocol in the wild, and it makes sense.

[athornton]

Not entirely sure I understand the purpose of "authoritative".

[vit-zikmund]

This is related to caching and thread safety, in the boundaries of the current design.

The "problem" is in the way how this whole authentication is performed in BaseView._is_authorized, which first creates the authenticator, __call__s it to get the Identity and only then it verifies the right permissions with Identity.is_authorized.

These two consecutive steps have no locks, so they can get mixed up with other threads simultaneously doing the same thing. Because I'm authorizing the user via GitHub API calls, my goal is to prevent those from repeating not only by caching that for some TTL, but also by blocking concurrent identical auth requests from different threads, so only a single thread gets to do the API calls and other just sit blocking and get only the result.

Because I wanted to do all the API calls within the GithubAuthenticator.__call__ method not to drag the requests Session object around (which is otherwise helpful in reusing a single TLS connection the to API), I'm storing only the resulting permissions on the GithubIdentity object, which can only simply expire.

in a different design with a shared requests.Session, the GithubIdentity would know itself how to get the user's permissions (and refresh its permission cache), but the Session should have a limited lifetime and as such, I'd have to manage it externally, which seemed even more complicated.

With all this, and the fact all the caches not only have configurable TTL, but also a rather low maximum size, I was obsessed about a scenario where (likely) an attacker would DoS the permission cache by doing concurrent calls for the same user, but different org/repo. In that scenario, the cached entry that got stored in the GithubAuthenticator.__call__ gets evicted sooner than it's being finally accessed via Identity.is_authorized.

Therefore I couldn't eventually resist to introduce a tiny in-between cache to remember the permissions for a user obtained in __call__ till it's being successfully read in is_authorized 😮‍💨

This permissions method is called in two contexts:

• in the __call__ to peek if a certain user's permissions are cached
• from is_authorized, which isn't used internally, but mean the obtained permissions have been read by the core logic for good and that I'm free to finally forget it.

The authoritative flag is just for that latter case, telling the logic the permissions have been properly read and now can sit and expire in the regular cache.

I admit this design is covering a really niche scenario, but once I realized it's there, my "OCD" couldn't let it stay. If you think there's a chance to considerably simplify the design, I'm all ears, but since this works and it's already done... 😇

[athornton]

That sounds good to me. My approach would just (if I'd noticed it at all, which I probably wouldn't because I wouldn't have tested high concurrency) have been to throw a mutex around the critical section and accept the performance hit. This seems much more amenable to many callers at once.

[athornton]

This looks great. I wish we didn't have to import a private symbol from threading, but that feels like a problem in threading, not with your code.

[vit-zikmund]

@athornton, is it merge-ready now? ;)

[athornton]

Hey @vit-zikmund post-merge I'm now getting a bunch of failures in the github auth tests.

They're of the form:

    def __post_init__(self, request: flask.Request) -> None:
        org_repo_getter = itemgetter("organization", "repo")
>       self.org, self.repo = org_repo_getter(request.view_args or {})
E       KeyError: 'organization'

I'm starting in on this but I suspect you'll be able to fix it trivially.

[vit-zikmund]

Oh yeah, lemme fix that real quick!

[vit-zikmund]

fixed in #155