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

fix(sqlview): When current user in SQL view, do not cache #2911

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

benguaraldi
Copy link
Contributor

@benguaraldi benguaraldi commented Aug 21, 2024

This code implements the request from DHIS2-9300, namely that if ${_current_user_id} or ${_current_username} is in a SQL view, then the SQL view should not be allowed to cache, as caching would keep the same results in the SQL view if one user were to log in and another user were to log out.

@Birkbjo
Copy link
Member

Birkbjo commented Sep 9, 2024

This is probably something that should ideally also be validated by the backend? This would help, but if it can actually be a security issue, then we should follow up with backend as well?

@jimgrace
Copy link
Member

jimgrace commented Sep 9, 2024

@Birkbjo I agree about backend support also. If the frontend is bypassed, and for preexisting SQL Views, the backend shouldn't cache the results when a SQL View specifies the current user. This could be done in either of two ways, whichever seems more reasonable to code:

  1. Force caching off when saving a the SQL view containing one of these constructs, and patch any such preexisting SQL Views, or
  2. Force a result not to be cached when executing a SQL View with one of these constructs

Note: @DavidCKen (or @karolinelien & @stian-sandvold while David is on leave). Also: @netroms

Meanwhile, @Birkbjo, do you think it is reasonable to merge this change in the frontend as well?

@stian-sandvold
Copy link

@Birkbjo I agree about backend support also. If the frontend is bypassed, and for preexisting SQL Views, the backend shouldn't cache the results when a SQL View specifies the current user. This could be done in either of two ways, whichever seems more reasonable to code:

  1. Force caching off when saving a the SQL view containing one of these constructs, and patch any such preexisting SQL Views, or
  2. Force a result not to be cached when executing a SQL View with one of these constructs

Note: @DavidCKen (or @karolinelien & @stian-sandvold while David is on leave). Also: @netroms

Meanwhile, @Birkbjo, do you think it is reasonable to merge this change in the frontend as well?

I never worked on the SQL views code myself, but @jimgrace do you know if there is any use-cases we support today that prevents caching the views? Also, is this a task you would be able to create a solution for @jimgrace?

@Birkbjo
Copy link
Member

Birkbjo commented Sep 10, 2024

Meanwhile, @Birkbjo, do you think it is reasonable to merge this change in the frontend as well?

Yeah I think it makes sense. Sorry I should've been more clear that we can merge this - but I just wanted to mention that we shouldn't only do it on the client if it's indeed a security problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants