feat(auth): add device flow and token auth for headless/CI#75
feat(auth): add device flow and token auth for headless/CI#75Adarsha1999 wants to merge 1 commit intoceph:mainfrom
Conversation
Adarsha1999
force-pushed
the
feature/headless-api
branch
from
2155147 to
c77e168
Compare
Signed-off-by: Adarsha Dinda <[email protected]>
Adarsha1999
force-pushed
the
feature/headless-api
branch
from
c77e168 to
7c41b79
Compare
|
GH issue - #74 |
| "access_token": token, | ||
| } | ||
| request.session["user"] = data | ||
| from teuthology_api.services.helpers import isAdmin |
There was a problem hiding this comment.
Why did we move the import statement here?
| import gevent.monkey # noqa: E402 - before teuthology | ||
| _orig_patch_all = gevent.monkey.patch_all | ||
|
|
||
| def _patch_all_ssl_off(**kwargs): | ||
| kwargs["ssl"] = False | ||
| return _orig_patch_all(**kwargs) | ||
|
|
||
| gevent.monkey.patch_all = _patch_all_ssl_off | ||
| import teuthology # noqa: E402 | ||
| teuthology.setup_log_file(log_file) | ||
| if isinstance(func, str): | ||
| import importlib | ||
| mod_path, _, attr = func.rpartition(".") | ||
| mod = importlib.import_module(mod_path) | ||
| func = getattr(mod, attr) |
There was a problem hiding this comment.
Why do we need to monkeypatch here? Also we can probably import import importlib on top of the file instead of doing it conditionally here? Same with gevent and teuthology imports if possible.
| async def _resolve_auth(request: Request): | ||
| """ | ||
| Get access token from request.session | ||
| Resolve auth from session or Authorization: Bearer header. | ||
| Returns (username, token_dict). Validates Bearer tokens against GitHub. | ||
|
|
||
| Deprecated: Use resolve_access_token() instead for new code. |
There was a problem hiding this comment.
We can probably remove this function? I'm most probably missing something here but it seems to me that we introduced this function and then made it deprecated in same commit? 🤔
| raise HTTPException( | ||
| status_code=401, | ||
| detail="You need to be logged in", | ||
| detail="You need to be logged in. Use X-Access-Token header, Authorization: Bearer header, or browser login.", | ||
| headers={"WWW-Authenticate": "Bearer"}, |
There was a problem hiding this comment.
nit: the header probably needs to be changed since it could be any kind of auth. Same goes for all HTTPExceptions raised in _validate_token_with_github()
| token = (direct_token or "").strip() | ||
| if token: | ||
| log.info("Using X-Access-Token header for auth") | ||
| username, token_dict = await _validate_token_with_github(token) |
There was a problem hiding this comment.
If _validate_token_with_github() fails on any of the validations, then it would throw a HTTPException but we don't catch it anywhere in resolve_access_token()?
There was a problem hiding this comment.
Oh I see, we let it be raised in create_run().
| membership_resp = await client.get( | ||
| url=GH_FETCH_MEMBERSHIP_URL, headers=headers | ||
| ) |
There was a problem hiding this comment.
It looks like this membership check is done thrice (once here and two times in routes/login.py) - probably better to extract it as a separate function and simply call that everywhere.
| detail="Failed to verify organization membership", | ||
| headers={"WWW-Authenticate": "Bearer"}, | ||
| ) | ||
| return username, {"access_token": token, "token_type": "bearer"} |
There was a problem hiding this comment.
Is it okay to return "token_type": "bearer" in token_dict here for direct token too? Or I guess we can return the token and create token_dict later in resolve_access_token for each type?
3 participants
Merge #51 first before this PR
Contribution Guidelines
To sign and test your commits, please refer to Contibution guidelines.
Checklist