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: Handle permission clause for result permissions. #2132

Merged
merged 2 commits into from
Feb 9, 2024

Conversation

jhelmold
Copy link
Member

@jhelmold jhelmold commented Feb 9, 2024

What

Results of a task could not bee seen any more by a user that has read permission
to the task. This problem is fixed now.

Why

This is a bug-fix

References

GEA-477

Checklist

Tested manually on my development system.

  • Tests

@jhelmold jhelmold requested a review from mattmundell as a code owner February 9, 2024 08:46
@jhelmold jhelmold requested a review from a team February 9, 2024 08:46
Copy link

github-actions bot commented Feb 9, 2024

Conventional Commits Report

Type Number
Bug Fixes 1

🚀 Conventional commits found.

@jhelmold jhelmold merged commit e9a6d99 into main Feb 9, 2024
9 checks passed
@jhelmold jhelmold deleted the GEA-477_user_cant_see_results branch February 9, 2024 09:55
@@ -1143,13 +1143,14 @@ acl_where_owned_user (const char *user_id, const char *user_sql,
else if (strcmp (type, "result") == 0)
permission_clause
= g_strdup_printf ("%s"
" OR results%s.task IN"
" OR EXISTS"
" (SELECT id FROM %spermissions_subject"
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels like this should be IN to match the other cases. In 28054e8 in PR #2118 @timopollmeier changed the EXISTS to IN, but he forgot to change id to resource. Maybe messed up his performance measurements, but maybe he was measuring the other cases.

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.

3 participants