Use API Tokens for Worker Auth#2421
Use API Tokens for Worker Auth#2421Disservin wants to merge 13 commits intoofficial-stockfish:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces JWT token-based authentication for workers as an alternative to password-based authentication, addressing performance concerns related to password hashing costs during frequent worker API calls.
Key Changes:
- Added JWT token generation, validation, and refresh logic on the server side using PyJWT library
- Modified worker authentication flow to prefer JWT tokens over passwords when available, with automatic fallback to password authentication
- Updated API schemas to accept either JWT or password authentication
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| worker/worker.py | Added JWT token verification, storage in config file, fallback authentication logic, and updated all function signatures to use auth dict instead of password strings |
| worker/games.py | Introduced add_auth helper function to conditionally add JWT or password to API payloads, updated all API calls to use auth dict |
| worker/tests/test_worker.py | Added test assertion for JWT config option |
| worker/sri.txt | Updated worker file hashes to reflect code changes (version 306) |
| server/fishtest/jwt_token.py | New module implementing JWT token creation, validation, and secret management with configurable TTL |
| server/fishtest/api.py | Added JWT validation logic alongside password validation, automatic JWT generation when authenticating with password, and JWT refresh in responses |
| server/fishtest/schemas.py | Updated API schemas to make password and JWT both optional but require at least one through has_worker_auth validation |
| server/pyproject.toml | Added PyJWT 2.10.1 dependency |
| server/uv.lock | Locked PyJWT dependency and updated package metadata |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Disservin
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ppigazzini
added
enhancement
worker update
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 11 comments.
Comments suppressed due to low confidence (1)
worker/worker.py:369
- The docstring for get_credentials should be updated to reflect that it now returns three values (username, password, api_key) instead of just username and password. This would improve code documentation and help future maintainers understand the function's interface.
def get_credentials(config, options, args):
remote = f"{options.protocol}://{options.host}:{options.port}"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ppigazzini
force-pushed
the
token-auth
branch
from
edae358 to
fb783a3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| api_access_schema = lax({"password": str, "worker_info": {"username": username}}) | ||
| def has_worker_auth(request): | ||
| return "password" in request or "api_key" in request |
There was a problem hiding this comment.
The has_worker_auth validation function checks for the presence of either password or api_key in the request, but it doesn't validate that they are non-empty strings. An empty string would pass the "password" in request check but wouldn't be valid for authentication. Consider adding validation that the auth fields are not empty when present.
| return "password" in request or "api_key" in request | |
| password = request.get("password") | |
| api_key = request.get("api_key") | |
| return ( | |
| (isinstance(password, str) and password.strip() != "") | |
| or (isinstance(api_key, str) and api_key.strip() != "") | |
| ) |
There was a problem hiding this comment.
again i think this is fine and auth should just fail
|
add api_key to users from datetime import datetime, timezone
from fishtest.rundb import RunDb
import secrets
rdb = RunDb()
keys = (
"username",
"password",
"registration_time",
"blocked",
"email",
"groups",
"tests_repo",
"machine_limit",
)
for u in rdb.userdb.get_users():
msg = ""
if u.get("registration_time") is None:
u["registration_time"] = datetime(2013, 1, 1, 0, 0, 0, tzinfo=timezone.utc)
msg += " time"
if u.get("blocked") is None:
u["blocked"] = False
msg += " blocked"
if u.get("email") is None:
u["email"] = ""
msg += " email"
if u.get("groups") is None:
u["groups"] = []
msg += " groups"
if u.get("tests_repo") is None:
u["tests_repo"] = ""
msg += " repo"
if u.get("machine_limit") is None:
u["machine_limit"] = 16
msg += " machine"
if u["registration_time"].tzinfo is None:
u["registration_time"] = u["registration_time"].replace(tzinfo=timezone.utc)
msg += " utc"
if u.get("api_key") is None:
u["api_key"] = f"ft_{secrets.token_urlsafe(32)}"
msg += " api_key"
if msg:
rdb.userdb.save_user(u)
print(f"{u['username']}:{msg}") |
|
I think this good for another test run |
Disservin
force-pushed
the
token-auth
branch
from
d9bda5f to
e435116
Compare
|
updated to the new jinja/fastapi version, lightly tested on the server without a worker yet |
delete jwt_token update update sri update fucntion check for restricted user
Disservin
force-pushed
the
token-auth
branch
from
7b513ce to
78d4ce7
Compare
PoC: Alternative to the username & pw authentication, which caused much discussion regarding the
cost of pw hashing during Worker API calls.
Also updated the profile page to include a way to reset the api token, and refactored the page sttructure a bit, to make the dependencies a bit more clear.. also previously the github token was sent to the server which is wrong since we store it in the users localstorage..