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

refactor: remove get_current_user and replace with direct header read #1834

Merged
merged 3 commits into from
Oct 7, 2024

Conversation

cpacker
Copy link
Collaborator

@cpacker cpacker commented Oct 6, 2024

tl;dr refactor the way we get user_id from the header to be stateless (fixing a race condition bug)


Currently incoming requests get the user_id by polling the server for the current user with get_current_user, which reads from a variable:

    # TODO(ethan) wire back to real method in future ORM PR
    def get_current_user(self) -> User:
        """Returns the currently authed user.

        Since server is the core gateway this needs to pass through server as the
        first touchpoint.
        """

        # Check if _current_user is set and if it's non-null:
        if hasattr(self, "_current_user") and self._current_user is not None:
            current_user = self.get_user(self._current_user)
            if not current_user:
                warnings.warn(f"Provided user '{self._current_user}' not found, using default user")
            else:
                return current_user

        return self.get_default_user()

This variable is set by middleware inside of rest_api.app:

    @app.middleware("http")
    async def set_current_user_middleware(request: Request, call_next):
        user_id = request.headers.get("user_id")
        if user_id:
            try:
                server.set_current_user(user_id)
            except ValueError as e:
                # Return an HTTP 401 Unauthorized response
                # raise HTTPException(status_code=401, detail=str(e))
                return JSONResponse(status_code=401, content={"detail": str(e)})
        else:
            server.set_current_user(None)
        response = await call_next(request)
        return response

This code is pretty bad since it clearly allows for race conditions (this was meant to be temporary code during the integration refactor).

This PR refactors the routes to instead do a simple read of the user_id from the request headers. If the client did not ser the user_id in the header, then the server will simply use the default user.

… uses the user_id passed in the request header (if there is nothing, it asks the server for the default user
…me that get_user returns Optional[User] (it returns User or throws ValueError)
@cpacker
Copy link
Collaborator Author

cpacker commented Oct 6, 2024

Tagging @4shub as an FYI

@cpacker cpacker removed the request for review from 4shub October 6, 2024 18:37
@cpacker cpacker merged commit a959e63 into main Oct 7, 2024
11 checks passed
@cpacker cpacker deleted the fix-get-current-user branch October 7, 2024 22:23
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

Successfully merging this pull request may close these issues.

Security issue when create agent is called from different user simultaneously
2 participants