Skip to content
This repository was archived by the owner on Oct 6, 2025. It is now read-only.

[NO MERGE] fake PR for comments - #2

Draft
devincowan wants to merge 1 commit into
mainfrom
develop
Draft

[NO MERGE] fake PR for comments#2
devincowan wants to merge 1 commit into
mainfrom
develop

Conversation

@devincowan

Copy link
Copy Markdown

No description provided.

@devincowan
devincowan marked this pull request as draft June 20, 2025 17:15
Comment thread requirements.txt
psycopg2
pydantic>=2.7.0,<3.0.0
redis
sqlmodel No newline at end of file

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually use sqlmodel?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use sqlalchemy which is a part of sqlmodel. We could reduce this dependency to sqlalchemy instead, good catch.

Comment thread api/routers/access_control_changed.py
Comment thread api/routers/access_control_changed.py
Comment thread api/routers/service_accounts.py
Comment thread tests/test_minio_authorization_webhook.py
@devincowan
devincowan requested a review from sblack-usu June 20, 2025 17:38
Comment thread Dockerfile

COPY ./api /app/api

CMD ["fastapi", "run", "api/main.py", "--port", "80", "--workers", "4"]

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sblack-usu why 4 workers? Might we need this to be more flexible for deployment into our cluster, depending on available resources?

Comment thread api/database.py
Comment on lines -70 to -96
def user_has_edit_access(user_id: int, resource_id: str):
query = """SELECT DISTINCT hs_core_genericresource.short_id
FROM hs_core_genericresource
LEFT OUTER JOIN hs_access_control_userresourceprivilege
ON (hs_core_genericresource.page_ptr_id = hs_access_control_userresourceprivilege.resource_id)
LEFT OUTER JOIN hs_access_control_resourceaccess
ON (hs_core_genericresource.page_ptr_id = hs_access_control_resourceaccess.resource_id)
LEFT OUTER JOIN hs_access_control_groupresourceprivilege
ON (hs_core_genericresource.page_ptr_id = hs_access_control_groupresourceprivilege.resource_id)
LEFT OUTER JOIN auth_group
ON (hs_access_control_groupresourceprivilege.group_id = auth_group.id)
LEFT OUTER JOIN hs_access_control_usergroupprivilege
ON (auth_group.id = hs_access_control_usergroupprivilege.group_id)
LEFT OUTER JOIN hs_access_control_groupaccess
ON (auth_group.id = hs_access_control_groupaccess.group_id)
INNER JOIN pages_page
ON (hs_core_genericresource.page_ptr_id = pages_page.id)
WHERE ((hs_access_control_userresourceprivilege.privilege = 1
AND hs_access_control_userresourceprivilege.user_id = :user_id)
OR (hs_access_control_userresourceprivilege.privilege <= 2
AND hs_access_control_userresourceprivilege.user_id = :user_id
AND NOT hs_access_control_resourceaccess.immutable)
OR (hs_access_control_usergroupprivilege.user_id = :user_id
AND hs_access_control_groupaccess.active
AND hs_access_control_groupresourceprivilege.privilege = 2
AND NOT hs_access_control_resourceaccess.immutable))
AND hs_core_genericresource.short_id = :resource_id"""

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know our User and Resource models are pretty much set in stone, and we don't really anticipate drift between these queries and our models in HydroShare...so I'm going to ignore my distaste for these raw queries. 😄

I'm sure that we chose raw SQL queries like this for performance reasons, right?

@devincowan

Copy link
Copy Markdown
Author

@sblack-usu other than removing the unnecessary dependency and addressing the Q about workers, this looks pretty great!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants