-
Notifications
You must be signed in to change notification settings - Fork 164
refactor(BA-3587): Define DTO types for auth handler's soft migration #8353
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces Pydantic-based DTOs for the auth API and wires the manager auth service to use the new shared AuthTokenType definition, as a preparatory step for migrating API handlers off Trafaret to Pydantic models.
Changes:
- Introduces Pydantic request/response DTOs for auth operations (
authorize,signup,signout, password updates, SSH keypair operations, etc.) underai.backend.common.dto.manager.auth. - Refactors the auth common types into
types.py, definingAuthTokenType,AuthResponseType,TwoFactorType,AuthResponsevariants, andparse_auth_response, and switchesAuthServiceandAuthorizeActionto importAuthTokenTypefrom the new module. - Adds a docstring-only
__init__.pyfor the auth DTO package to document the new structure and intended import style.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/ai/backend/manager/services/auth/service.py |
Updates AuthService to import AuthTokenType from the new dto.manager.auth.types module, aligning service logic with the new DTO type location. |
src/ai/backend/manager/services/auth/actions/authorize.py |
Switches AuthorizeAction to depend on AuthTokenType from dto.manager.auth.types, ensuring the action uses the shared auth enum. |
src/ai/backend/common/dto/manager/auth/types.py |
Defines AuthTokenType, AuthResponseType, TwoFactorType, and Pydantic AuthResponse models plus parse_auth_response, centralizing auth-related types for both client and manager. |
src/ai/backend/common/dto/manager/auth/response.py |
Adds Pydantic response DTOs (AuthorizeResponse, SignupResponse, UpdatePasswordNoAuthResponse, SSH keypair responses, etc.) for auth endpoints. |
src/ai/backend/common/dto/manager/auth/request.py |
Adds Pydantic request DTOs for auth endpoints, including alias handling for legacy parameter names (e.g., stoken/sToken, email/username). |
src/ai/backend/common/dto/manager/auth/__init__.py |
Documents the new auth DTO package layout and suggests importing directly from types, request, and response submodules. |
Comments suppressed due to low confidence (1)
src/ai/backend/common/dto/manager/auth/types.py:21
- Moving
AuthTokenType,AuthResponseType,AuthSuccessResponse, etc. into thistypesmodule without providing a backward-compatible alias breaks existing imports that still referenceai.backend.common.dto.manager.auth.field(for example:src/ai/backend/manager/api/auth.py,src/ai/backend/web/server.py,src/ai/backend/client/func/user.py, andtests/unit/manager/services/auth/test_authorize.py). Sincesrc/ai/backend/common/dto/manager/auth/field.pyno longer exists in the branch, those modules will fail to import; please either add anauth/field.pyshim that re-exports these names from.typesor update all remaining imports to point at.typesin this refactor.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,1174 @@ | |||
| """ | |||
| Legacy auth handlers (function-based). | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nothing but copy-and-pasted version of old auth.py for regression test.
| class UpdatePasswordNoAuthResponse(BaseResponseModel): | ||
| """Response for update password without auth.""" | ||
|
|
||
| password_changed_at: datetime = Field(description="Timestamp when password was changed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original API handler returns string typed password_changed_at. Is this serialize the value to a iso format string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def to_json(
value: Any,
*,
indent: int | None = None,
include: _IncEx | None = None,
exclude: _IncEx | None = None,
# Note: In Pydantic 2.11, the default value of `by_alias` on `SchemaSerializer` was changed from `True` to `None`,
# to be consistent with the Pydantic "dump" methods. However, the default of `True` was kept here for
# backwards compatibility. In Pydantic V3, `by_alias` is expected to default to `True` everywhere:
by_alias: bool = True,
exclude_none: bool = False,
round_trip: bool = False,
timedelta_mode: Literal['iso8601', 'float'] = 'iso8601',
bytes_mode: Literal['utf8', 'base64', 'hex'] = 'utf8',
inf_nan_mode: Literal['null', 'constants', 'strings'] = 'constants',
serialize_unknown: bool = False,
fallback: Callable[[Any], Any] | None = None,
serialize_as_any: bool = False,
context: Any | None = None,
) -> bytes:
From pydantic core, i guess it will detect the type(iso 8601) and serialize into json string,
but there can be some slight difference like
2024-01-28T12:00:00+00:00 with 2024-01-28T12:00:00Z.
I guess it will be better to explicitely transfer with method like isoformat().
as both of the share same class datetime.
Thanks for letting me know!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modifiied response field as string,
And it will be converted from API adapter in following PR(to adapter migration)
(e.g.)
) -> UpdatePasswordNoAuthResponse:
"""Convert UpdatePasswordNoAuthActionResult to UpdatePasswordNoAuthResponse."""
return UpdatePasswordNoAuthResponse(
password_changed_at=result.password_changed_at.isoformat(), // example
)
thank you
0dbf811 to
5def694
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you checked if moving the directory might cause issues? Also, it doesn't seem necessary to change the existing handlers this time. I'd like to clarify the current work direction, and I think declaring it deprecated is too hasty a decision. To mark it deprecated, we need to have a new alternative API already in place to guide users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def _init_subapp(
pkg_name: str,
root_app: web.Application,
subapp: web.Application,
global_middlewares: Iterable[Middleware],
) -> None:
subapp.on_response_prepare.append(on_prepare)
async def _set_root_ctx(subapp: web.Application):
# Allow subapp's access to the root app properties.
# These are the public APIs exposed to plugins as well.
subapp["_root.context"] = root_app["_root.context"]
subapp["_root_app"] = root_app
# We must copy the public interface prior to all user-defined startup signal handlers.
subapp.on_startup.insert(0, _set_root_ctx)
if "prefix" not in subapp:
subapp["prefix"] = pkg_name.split(".")[-1].replace("_", "-")
prefix = subapp["prefix"]
root_app.add_subapp("/" + prefix, subapp)
root_app.middlewares.extend(global_middlewares)
when i had a investigate before migrating,
I thought it could be alright if we retain sub-servers name the same.
But i understand the concern, and it seems subapp would detect ./auth.py faster than /auth/init.py
it will be much more safer to migrate into legacy with handler/adapter pattern fully assured.
thanks you!
5def694 to
b5e471f
Compare
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Temporary file for migrating into pydantic class from following PR.
| } | ||
| whois_timezone_info: Final[Mapping[str, int]] = {k: int(v) for k, v in _whois_timezone_info.items()} | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As there are following PR for defining types for credential information,
I've just marked it as tuple.
resolves #7626 (BA-3587)
fine grained PR from
https://github.com/lablup/backend.ai/pull/7865/commits
implements DTO and actions before using it as api_handler's arguments.
Checklist: (if applicable)
ai.backend.testdocsdirectory