sts-sponsors team mailing list archive

[Alberto Donato <mp+440870@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> diff --git a/Makefile b/Makefile
> index ad5452d..86cccb6 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -54,7 +54,7 @@ ci-backend-build:  # nothing to do since everything is run in tox envs
>  .PHONY: ci-backend-build
>  
>  ci-backend-lint:
> -	env -C backend tox -e lint,check
> +	env -C backend tox -e format,lint,check

CI shouldn't cal format, as it could potentially fix stuff and make lint pass, but the change wouldn't be committed

>  .PHONY: ci-backend-lint
>  
>  ci-backend-test:
> diff --git a/backend/msm/schema/_models.py b/backend/msm/schema/_models.py
> index 6128eec..754f54e 100644
> --- a/backend/msm/schema/_models.py
> +++ b/backend/msm/schema/_models.py
> @@ -14,19 +14,31 @@ from pydantic.fields import Field
>  from ._pagination import PaginatedResults
>  
>  
> -class CreateUser(BaseModel):
> +class ReadUser(BaseModel):
>      """
>      A MAAS Site Manager User
> +    We never want to sent the password (hash) around
>      """
>  
>      email: EmailStr = Field(title="email@xxxxxxxxxxx")
>      full_name: str
> -    # use password.get_secret_value() to retrieve the value
> -    password: SecretStr = Field(min_length=8, max_length=50)
>      disabled: bool

What's the idea with disabling users?

>  
>  
> -class User(CreateUser):
> +class UserWithPassword(ReadUser):
> +    """
> +    To create a user we need a password as well.
> +    """
> +
> +    # use password.get_secret_value() to retrieve the value
> +    password: SecretStr = Field(min_length=8, max_length=100)
> +
> +
> +class User(ReadUser):
> +    """
> +    To read a user from the DB it comes with an ID
> +    """
> +
>      id: int
>  
>  
> @@ -107,6 +119,15 @@ class Token(CreateToken):
>      id: int
>  
>  
> +class JWTToken(BaseModel):
> +    access_token: str
> +    token_type: str
> +
> +
> +class JWTTokenData(BaseModel):
> +    email: str

please add docstrings to these classes

> +
> +
>  class PaginatedTokens(PaginatedResults):
>      items: list[Token]
>  
> diff --git a/backend/msm/user_api/_base.py b/backend/msm/user_api/_base.py
> index 38d645c..c65ef31 100644
> --- a/backend/msm/user_api/_base.py
> +++ b/backend/msm/user_api/_base.py
> @@ -9,15 +17,34 @@ from ..db import (
>  from ..schema import (
>      CreateTokensRequest,
>      CreateTokensResponse,
> +    JWTToken,
>      PaginatedSites,
>      PaginatedTokens,
>      pagination_params,
>      PaginationParams,
> +    User,
>  )
>  from ._forms import (
>      site_filter_parameters,
>      SiteFilterParams,
>  )
> +from ._jwt import (
> +    ACCESS_TOKEN_EXPIRE_MINUTES,
> +    authenticate_user,
> +    create_access_token,
> +    get_current_active_user,
> +)
> +
> +
> +def protected(func):
> +    """Protect an endpoint, requiring the active user be authenticated."""
> +
> +    async def wrapper(*args, **kwargs):
> +        return await func(
> +            *args, current_user=Depends(get_current_active_user), **kwargs

I'd call the parameter just "user", maybe also rename the function to "get_authenticated_user" for clarity

> +        )
> +
> +    return wrapper
>  
>  
>  async def root() -> dict[str, str]:
> diff --git a/backend/msm/user_api/_jwt.py b/backend/msm/user_api/_jwt.py
> new file mode 100644
> index 0000000..2ae62ff
> --- /dev/null
> +++ b/backend/msm/user_api/_jwt.py
> @@ -0,0 +1,114 @@
> +from __future__ import annotations
> +
> +from datetime import (
> +    datetime,
> +    timedelta,
> +)
> +from os import getenv
> +from typing import (
> +    Annotated,
> +    Any,
> +)
> +
> +from fastapi import (
> +    Depends,
> +    HTTPException,
> +    status,
> +)
> +from fastapi.security import OAuth2PasswordBearer
> +from jose import (
> +    jwt,
> +    JWTError,
> +)
> +from passlib.context import CryptContext
> +from sqlalchemy.ext.asyncio import AsyncSession
> +
> +# from ..db import db_session
> +from ..db.queries import get_user
> +from ..schema import (
> +    JWTTokenData,
> +    User,
> +    UserWithPassword,
> +)
> +
> +# to get a string like this run:
> +# openssl rand -hex 32
> +SECRET_KEY = getenv(
> +    "SECRET_KEY",
> +    "09d25e094faa6ca2556c818166b7a9563b93f7099f6f0f4caa6cf63b88e8d3e7",

please add an XXX comment that we need to require this in the config, maybe also log a message if it's unset in the env, so we don't forget

> +)
> +ALGORITHM = "HS256"
> +ACCESS_TOKEN_EXPIRE_MINUTES = getenv("TOKEN_EXPIRATION_TIME", 30)
> +
> +pwd_context = CryptContext(schemes=["bcrypt"], deprecated="auto")
> +oauth2_scheme = OAuth2PasswordBearer(tokenUrl="token")
> +
> +
> +def verify_password(
> +    plain_password: str | bytes, hashed_password: str | bytes | None

why can these be bytes as well?

> +) -> bool:
> +    """
> +    Verify a plain password against a password hash created by passlib
> +    """
> +    return pwd_context.verify(plain_password, hashed_password)
> +
> +
> +def get_password_hash(password: Any) -> str:

shouldn't password always be a str?

> +    """
> +    Get a hash for a password
> +    """
> +    return pwd_context.hash(password)
> +
> +
> +def create_access_token(
> +    data: dict[str, Any], expires_delta: timedelta | None = None
> +) -> str:
> +    to_encode = data.copy()
> +    if expires_delta:
> +        expire = datetime.utcnow() + expires_delta
> +    else:
> +        expire = datetime.utcnow() + timedelta(minutes=15)
> +    to_encode.update({"exp": expire})
> +    encoded_jwt = jwt.encode(to_encode, SECRET_KEY, algorithm=ALGORITHM)
> +    return encoded_jwt
> +
> +
> +async def authenticate_user(
> +    session: AsyncSession, email: str, password: str
> +) -> UserWithPassword | None:
> +    user = await get_user(session, email)
> +    if not user or user.disabled:
> +        return None
> +    if not verify_password(password, user.password.get_secret_value()):
> +        return None
> +    return user
> +
> +
> +async def get_current_user(
> +    token: Annotated[str, Depends(oauth2_scheme)], session: Any
> +) -> UserWithPassword | None:
> +    credentials_exception = HTTPException(
> +        status_code=status.HTTP_401_UNAUTHORIZED,
> +        detail="Could not validate credentials",
> +        headers={"WWW-Authenticate": "Bearer"},
> +    )
> +    try:
> +        payload = jwt.decode(token, SECRET_KEY, algorithms=[ALGORITHM])
> +        email: str | None = payload.get("sub")
> +        if email is None:
> +            raise credentials_exception
> +        token_data = JWTTokenData(email=email)
> +    except JWTError:
> +        raise credentials_exception
> +    user = await get_user(session, email=token_data.email)
> +    if user is None:
> +        raise credentials_exception
> +    return user
> +
> +
> +async def get_current_active_user(
> +    current_user: Annotated[User, Depends(get_current_user)]
> +) -> User | None:
> +    if current_user.disabled:
> +        raise HTTPException(status_code=400, detail="Inactive user")
> +    return current_user
> diff --git a/backend/tests/fixtures/app.py b/backend/tests/fixtures/app.py
> index d325e81..907a99a 100644
> --- a/backend/tests/fixtures/app.py
> +++ b/backend/tests/fixtures/app.py
> @@ -1,15 +1,78 @@
>  from typing import (
> +    Any,
>      AsyncIterable,
>      Iterable,
>  )
>  
>  from fastapi import FastAPI
> -from httpx import AsyncClient
> +from httpx import (
> +    AsyncClient,
> +    Response,
> +)
>  import pytest
>  
>  from msm.db import Database
>  from msm.user_api import create_app
>  
> +from .db import Fixture
> +
> +
> +class AuthAsyncClient(AsyncClient):
> +    def __init__(self, **kwargs) -> None:
> +        super().__init__(**kwargs)
> +        self.authed = False

I'd drop the field and make it a property:

@property
def authorized(self) -> bool:
    return self._token is not None

(also self._token and self._token_type should be set as None in the __init__

> +
> +    async def login(self, email: str, password: str) -> None:
> +        response = await self.post(
> +            "/login", data={"username": email, "password": password}
> +        )
> +        assert response.status_code == 200
> +        self.email = email
> +        self._token = response.json()["access_token"]
> +        self._token_type = response.json()["token_type"].capitalize()
> +        self.authed = True
> +
> +    @property
> +    def auth(self) -> dict[str, Any]:
> +        # TODO: Fix.
> +        # Seems to require 'params' with a session field, but that overrides
> +        # the session object in queries.get_user()
> +        auth_params = {
> +            "headers": {"Authorization": f"{self._token_type} {self._token}"},
> +            # "params": {"session": None},
> +        }
> +        return auth_params if self.authed else {}
> +
> +    def request(self, *args, **kwargs) -> Response:
> +        if self.authed:
> +            for k, v in self.auth.items():
> +                if kwargs.get(k) is None:
> +                    kwargs[k] = v
> +                else:
> +                    kwargs.update({k: v})

I think you can just kwargs.update(self.auth) here and skip the if/else

> +        return super().request(*args, **kwargs)
> +
> +
> +@pytest.fixture
> +async def authenticated_user_app_client(
> +    user_app: FastAPI, fixture: Fixture
> +) -> AsyncIterable[AuthAsyncClient]:
> +    """Authenticated Client for the user API."""
> +    phash = "$2b$12$F5sgrhRNtWAOehcoVO.XK.oSvupmcg8.0T2jCHOTg15M8N8LrpRwS"
> +    async with AuthAsyncClient(app=user_app, base_url="http://test";) as client:
> +        await fixture.create(
> +            "user",
> +            {
> +                "id": 1,
> +                "email": "admin@xxxxxxxxxxx",
> +                "full_name": "Admin",
> +                "disabled": False,
> +                "password": phash,
> +            },
> +        )
> +        await client.login("admin@xxxxxxxxxxx", "admin")
> +        yield client
> +
>  
>  @pytest.fixture
>  def user_app(


-- 
https://code.launchpad.net/~lloydwaltersj/maas-site-manager/+git/site-manager/+merge/440870
Your team MAAS Committers is requested to review the proposed merge of ~lloydwaltersj/maas-site-manager:add-login into maas-site-manager:main.