Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(auth): add github proxy authentication #146

Merged
merged 2 commits into from
Feb 28, 2024
Merged

feat(auth): add github proxy authentication #146

merged 2 commits into from
Feb 28, 2024

Conversation

vit-zikmund
Copy link
Collaborator

@vit-zikmund vit-zikmund commented Feb 27, 2024

Hi there, here's a production-grade-aspiring implementation (w/o tests yet) of the GitHub "proxy" authentication, which provides the same access permissions to a Git LFS org/repo as are those for the same GitHub repo. It uses Github's HTTP API personal access tokens, which are used to retrieve the particular user's permissions.

The module has been developed with the focus on minimizing GitHub API traffic where feasible, so it uses a lot of [configurable] API response caching.

The next aim was to keep the new 3rd party dependencies low, which so far worked, except for the use of a small package called cachetools and marshmallow, which both are already (transitively) used by existing code. I looked around for various libs, but eventually ended up doing something that badly feels like inventing the wheel in case of thread-safe caching wrappers for the [idempotent] GitHub API calls. I didn't find any alternative, tho 🤷, so there it is.

Everything is in a single file to avoid back-and-forth merging hassles, but it might be worth splitting some parts into separate files once/if the PR gets merged.

Originally, this was a very simple file, but then came config values, their validation, persistent connection reuse, robust error handling, caching, and ultimately further thread-safety measures.

There is also an option to change the GitHub API URL so it could be used for "enterprise servers", which have a different API endpoint than the public cloud GitHub.

I'm opening this as a draft, because I'm aware that I didn't write any docs or tests, but I also deeply desire someone to review the code, so we'd know we're on the right track and I didn't create some design issues along the way so far. Thank you.

PS: Again, thanks @athornton for letting me know about his own custom implementation which I sure used for inspiration.

Closes: #147

@athornton
Copy link
Collaborator

Do you feel like fixing the typing stuff in the test failures? If you don't, I'll get to that when I can, but I'm kinda swamped with my day job right now.

Other than that, this looks very good.

@vit-zikmund
Copy link
Collaborator Author

Done... that took my last breath today 😩

FYI, it seems the "requirements.txt" dependencies generated by pip-tools are python-version-specific. Did you notice? I guess this should be somehow reflected in the CI scripts/makefiles, right?

@athornton
Copy link
Collaborator

athornton commented Feb 27, 2024

Nice.

Yeah, requirements.txt varies because some versions of packages require Python 3.11 or 3.12 and pip-compile uses the latest available that will work with the python you're using. I generally develop with the mininum version (3.10 in this case) and then have the CI check that it works with 3.11 and 3.12. Except I failed to do that in this case, and I should have. It looks like you fixed it, which is good.

@athornton
Copy link
Collaborator

I'm going to let it sit a bit in case anyone who actually works for Datopian wants to weigh in, but if not, I'll merge it tomorrow and make a new release over in lsstsqre space. I'm still waiting for someone at Datopian to figure out what we want to do about making an official release (and pipeline), but I've been pretty swamped so I haven't been pushing them.

@athornton
Copy link
Collaborator

Well, I see no objections, so let's go ahead and merge it.

@athornton athornton merged commit a28013c into datopian:main Feb 28, 2024
7 checks passed
@rufuspollock
Copy link
Member

Just want to say this looks great and big props @vit-zikmund 👏👏 and thanks for the great PR management and review @athornton 👏

thanks all!

@vit-zikmund vit-zikmund deleted the github-auth branch February 29, 2024 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[github-auth] Implement core functionality
3 participants