Skip to content

auth via oauth with zimfarm#1146

Open
elfkuzco wants to merge 3 commits intomainfrom
oauth-with-zimfarm
Open

auth via oauth with zimfarm#1146
elfkuzco wants to merge 3 commits intomainfrom
oauth-with-zimfarm

Conversation

@elfkuzco
Copy link
Copy Markdown
Collaborator

@elfkuzco elfkuzco commented Apr 3, 2026

Rationale

This PR enhances WP1 to optionally authenticate with Zimfarm using ouath (via Ory) instead of just username and password.

Changes

  • create token provider class that generates oauth tokens based on specified auth mode
  • directly raise ZimfarmError when no token can be generated instead of delegating it to callers

This closes #1144

@elfkuzco elfkuzco self-assigned this Apr 3, 2026
@elfkuzco elfkuzco requested review from audiodude and benoit74 April 3, 2026 07:08
@elfkuzco
Copy link
Copy Markdown
Collaborator Author

elfkuzco commented Apr 3, 2026

@audiodude , do you think we should remove storing the tokens via Redis given there's always going to be one instance of the class and it maintains state. There could be restarts of the application or multiple instances but they can all generate newer tokens and continue to function independently as generating a new token doesn't invalidate the other.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 3, 2026

Codecov Report

❌ Patch coverage is 97.22222% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.83%. Comparing base (7d01e46) to head (dfbb9cf).

Files with missing lines Patch % Lines
wp1/zimfarm.py 97.22% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1146      +/-   ##
==========================================
+ Coverage   92.78%   92.83%   +0.04%     
==========================================
  Files          74       74              
  Lines        4311     4324      +13     
==========================================
+ Hits         4000     4014      +14     
+ Misses        311      310       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@benoit74
Copy link
Copy Markdown
Contributor

@audiodude shall we merge and deploy this soon? WP1 seems to be the last system using "old" username/password auth to Zimfarm (cannot say for sure, since all I see is logs using "old" API endpoints).

@audiodude
Copy link
Copy Markdown
Member

Yes I generally feel good about the concept and am eager to merge. However, I do want to do a detailed code review first and I haven't had the time. Mostly just to make sure I understand the changes and the new flow.

@audiodude
Copy link
Copy Markdown
Member

@audiodude , do you think we should remove storing the tokens via Redis given there's always going to be one instance of the class and it maintains state. There could be restarts of the application or multiple instances but they can all generate newer tokens and continue to function independently as generating a new token doesn't invalidate the other.

That's certainly a valid design. But this PR already has it continuing to use Redis, and we need Redis for other parts of the system, so I think it's fine to leave (though you could add an issue if you'd like for the future).

Copy link
Copy Markdown
Member

@audiodude audiodude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thank you so much!

just a few comments.

Comment thread wp1/zimfarm.py Outdated
Comment thread wp1/zimfarm.py
Comment thread docker/zimfarm/zimfarm_ui_dev/config.json
Comment thread docker-compose-dev.yml
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.

Add capability to authenticate with zimfarm based on OAuth

3 participants