Feature/privacy policy#196
Conversation
|
Modification includes asking consent from user after OIDC login. This way the user is not created and added to the database before they have given required consent (privacy policy and terms & conditions). Until consent is given the user's information (sub and email from OIDC login) is held temporarily in Redis as part of their session. If the user never gives consent, their data is automatically deleted when their session expires. The database now stores timestamps when the user has given consent as well as information to which version of the privacy policy / terms and conditions they have consented. This way we may present the modal for consent again in case either one is updated. I also added a button to turn on/off the option to share anonymized data for research purposes in Settings - Account. This can be modified. Also updated existing tests so that they pass. |
|
Before the fix the SSE event stream was global, meaning users could see each others notifications regarding job progress and file uploads. The change includes replacing the single global redis channel with per user-channels. Now each browser subscribes only to its own user's channel. This also means multiple tabs for the same user all receive events correctly. Also added owner status check to a few of the tests that lacked it. |
| async def fetch_project_by_uuid(self, uuid: UUID) -> ProjectRead | None: | ||
| async def fetch_project_by_uuid(self, uuid: UUID, owner_uuid: UUID | None = None) -> ProjectRead | None: | ||
| stmt = select(Project).where(Project.uuid == uuid) | ||
| if owner_uuid is not None: |
There was a problem hiding this comment.
Shouldn't this be enforced that owner must be always checked?
There was a problem hiding this comment.
Yes. I just noticed this in another place while trying to run a mock task. The problem is that owner_uuid is optional because the same schema is used for both HTTP requests (where the frontend doesn't send it) and internal code like Celery (where it's always needed). The cleanest fix would be to split request schemas from internal ones so we can require owner_uuid in the CRUD/service layers.
There was a problem hiding this comment.
Yeah I think some clear divide would be smart. Like fetch_<thing>_by_owner, would at least make it a bit clearer where the owner ship is checked.
Also I'm not sure what the best flow for checking ownership is. E.g. in controllers/paper.py
@router.get(
"/paper/{project_uuid}/with_model_evaluations",
status_code=status.HTTP_200_OK,
tags=["Paper"],
)
async def get_project_papers_with_model_evals(
project_uuid: UUID,
db_ctx: DBContext = Depends(get_db_ctx),
current_user: User = Depends(get_current_user),
):
papers = create_paper_service(db_ctx)
project_service = create_project_service(db_ctx)
try:
project = await project_service.fetch_by_uuid(project_uuid, owner_uuid=current_user.uuid)
if not project:
raise HTTPException(
status_code=status.HTTP_404_NOT_FOUND, detail="Project not found"
)
return await papers.fetch_papers_with_model_evals(project_uuid)
except HTTPException:
raise
except Exception as e:
raise HTTPException(
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
detail=f"Failed to fetch papers: {str(e)}",
) from e
We fetch the project by uuid and check the ownership, and if project is not found we return exception, else fetch by the project_uuid.
But this seems maybe a bit flaky to depend on this kind of flow, kind of reminds of TOCTOU.
|
Fixes include: implemented a shared Redis client to fix connection leaks. Added Redis as a FastAPI dependency that is used for auth endpoints. Also added AbortController to useEffect in App.tsx and AccountSettingsPage.tsx. Added reset_shared_redis to conftest to close the shared client between tests. |
This branch is for continuing work with the privacy policy and some extra stuff.
Things completed include adding owner status so that each api key are personal and stored in the db. Users can also delete their added api key. The register and privacy policy are now added to the bottom of the pages as well as a disclaimer regarding LLM use when a user is logged in.