Skip to content
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

Use @dataclass(frozen=True) on APIUser dataclass #409

Closed
olevski opened this issue Sep 16, 2024 · 3 comments
Closed

Use @dataclass(frozen=True) on APIUser dataclass #409

olevski opened this issue Sep 16, 2024 · 3 comments

Comments

@olevski
Copy link
Member

olevski commented Sep 16, 2024

Currently we do a bit of unecessary gymnastics to prevent someone simply flipping the is_admin flag on an api user instance in the code.

But we can eliminate all of this code by simply making the dataclass frozen.

@dataclass(kw_only=True)
class APIUser:
    """The model for a user of the API, used for authentication."""

    id: Optional[str] = None  # the sub claim in the access token - i.e. the Keycloak user ID
    access_token: Optional[str] = field(repr=False, default=None)
    refresh_token: Optional[str] = field(repr=False, default=None)
    full_name: Optional[str] = None
    first_name: Optional[str] = None
    last_name: Optional[str] = None
    email: Optional[str] = None
    access_token_expires_at: datetime | None = None
    is_admin_init: InitVar[bool] = False
    __is_admin: bool = field(init=False, repr=False)

    def __post_init__(self, is_admin_init: bool) -> None:
        self.__is_admin: bool = is_admin_init

    @property
    def is_authenticated(self) -> bool:
        """Indicates whether the user has successfully logged in."""
        return self.id is not None

    @property
    def is_admin(self) -> bool:
        """Indicates whether the user is a Renku platform administrator."""
        return self.__is_admin

Instead of the above we can simply use the stuff below.

@dataclass(kw_only=True, frozen=True)
class APIUser:
    """The model for a user of the API, used for authentication."""

    id: Optional[str] = None  # the sub claim in the access token - i.e. the Keycloak user ID
    access_token: Optional[str] = field(repr=False, default=None)
    refresh_token: Optional[str] = field(repr=False, default=None)
    full_name: Optional[str] = None
    first_name: Optional[str] = None
    last_name: Optional[str] = None
    email: Optional[str] = None
    access_token_expires_at: datetime | None = None
    is_admin: bool = False

    @property
    def is_authenticated(self) -> bool:
        """Indicates whether the user has successfully logged in."""
        return self.id is not None
@olevski
Copy link
Member Author

olevski commented Sep 16, 2024

Thanks to @sgaist for calling this out and also for suggesting how to fix the problem.

@sgaist
Copy link
Collaborator

sgaist commented Sep 17, 2024

After a quick test and some fixes, frozen dataclass are working but mypy start screaming.

@sgaist
Copy link
Collaborator

sgaist commented Sep 27, 2024

Fixed in #375

@sgaist sgaist closed this as completed Sep 27, 2024
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

No branches or pull requests

2 participants