additional_tags icon indicating copy to clipboard operation
additional_tags copied to clipboard

Authorization of view tags permission

Open dmakurin opened this issue 3 years ago • 3 comments

In addition to #36

I've tried to achieve strict restriction for issue tags access.

Issue.available_tags explicitly sets the permission to view_issue_tags instead of view_issues (and makes it optional).

Previously, the list of available tags was selected based on the view_issues permission. Now it returns a tags scope based on the view_issue_tags permissions. The result set is a list of tags which user has access to.

Dashboard block Issue tags utilizes IssueQuery to get the totals (filter: tags = any). For the any condition, filtering by permission view_issue_tags has also been added. I changed logic a little bit. Instead of just selecting all issues that have tags now it is split to multiple steps:

  • Get a list of projects where user has permission view_issue_tags
  • Use Issue.available_tags to find all visible to user tags
  • Based on found tags find all tagged issues
  • Then list of issues getting filtered by available projects And the result is a list of issues that resides in projects where user has permission to view issue tags.

There are still might be some cases where manual authorization is required like https://github.com/dmakurin/additional_tags/commit/9bd17917598d33ea10a3974bd65b2a2b26bb657a#diff-352fd2cfc51782002a317bf279a5cac7b794dcdf1b0c4fb66be5371e224df47fL16

I added tests to cover new behavior and fixed broken ones. Let me know if this is enough.

dmakurin avatar Nov 30 '22 14:11 dmakurin

Hi @dmakurin,

thanks for your work! I used your changes as base of some rework for it, which I commited.

Issue.available_tags explicitly sets the permission to view_issue_tags instead of view_issues (and makes it optional).

I accepted this change.

I bigger problem is with build_sql_for_tags_field. This is used by a lot of other plugins, which was not compatible with your changes. At the moment I didn't find any good solution without using a separate method for issues. But also with this separate method I am not sure, if this is working with large data sets. I optimized it a bit for better performance. But I think it needs more work. We have customers with 2000 projects and more than 200.000 issues.

I did not find (or work on it) a solution without the permission check in column_content_with_tags I did not commit this change, but take your test in my rework. This test is broken at the moment because of this reason. We should find a solution without this permission check on view level. This is very expensive related to performance. I hope we could find a solution for it.

Subproject support in combination of view_issue_tags permission is quite complex - this was the reason, because it was not implemented till you pr ;)

alexandermeindl avatar Dec 02 '22 08:12 alexandermeindl

I bigger problem is with build_sql_for_tags_field. This is used by a lot of other plugins, which was not compatible with your changes. At the moment I didn't find any good solution without using a separate method for issues. But also with this separate method I am not sure, if this is working with large data sets. I optimized it a bit for better performance. But I think it needs more work. We have customers with 2000 projects and more than 200.000 issues.

I don't think we can do much about that. It's still a good starting point.

I did not find (or work on it) a solution without the permission check in column_content_with_tags I did not commit this change, but take your test in my rework. This test is broken at the moment because of this reason. We should find a solution without this permission check on view level. This is very expensive related to performance. I hope we could find a solution for it.

Yeah that was straight forward solution. I have some thoughts how to do it. I'll let you know if i come up with something.

dmakurin avatar Dec 02 '22 13:12 dmakurin

I've added preloading of tags for a tag filter. It's working in a same way as other Issue filters that are not a core fields like relations, attachments, time entries and so on.

It allows to avoid n+1 query for every row in result set.

What do you think @alexandermeindl?

dmakurin avatar Dec 05 '22 10:12 dmakurin

Hi @dmakurin,

thanks for the update. This looks better now! I did not found any problems till now -> I merge it.

alexandermeindl avatar Dec 18 '22 14:12 alexandermeindl