alphanodes / additional_tags

Redmine Plugin for adding tags functionality to issues and wiki pages.
https://www.redmine.org/plugins/additional_tags
GNU General Public License v2.0
64 stars 26 forks source link

Authorization of view tags permission #40

Closed dmakurin closed 1 year ago

dmakurin commented 1 year ago

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:

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.

alexandermeindl commented 1 year ago

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_tagsfield. 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_withtags 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 ;)

dmakurin commented 1 year ago

I bigger problem is with _build_sql_for_tagsfield. 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_withtags 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 commented 1 year ago

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?

alexandermeindl commented 1 year ago

Hi @dmakurin,

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