https://www.redmine.org/https://www.redmine.org/favicon.ico?16793021292018-02-28T21:37:45ZRedmineRedmine - Defect #28264: Global and public custom queries are shown as editable to non administrators in projectshttps://www.redmine.org/issues/28264?journal_id=838152018-02-28T21:37:45ZMarius BĂLTEANU
<ul><li><strong>Related to</strong> <i><a class="issue tracker-1 status-5 priority-4 priority-default closed" href="/issues/17669">Defect #17669</a>: Non admin users can't modify public queries for all project</i> added</li></ul> Redmine - Defect #28264: Global and public custom queries are shown as editable to non administrators in projectshttps://www.redmine.org/issues/28264?journal_id=838172018-02-28T21:38:16ZMarius BĂLTEANU
<ul><li><strong>Related to</strong> <i><a class="issue tracker-1 status-5 priority-4 priority-default closed" href="/issues/14239">Defect #14239</a>: Error 403 when trying to edit custom query</i> added</li></ul> Redmine - Defect #28264: Global and public custom queries are shown as editable to non administrators in projectshttps://www.redmine.org/issues/28264?journal_id=838192018-02-28T22:05:38ZMarius BĂLTEANU
<ul><li><strong>File</strong> <i>fix_edit_delete_query_links_for_users_without_permissions.patch</i> added</li><li><strong>File</strong> <i>tests_for_28264.patch</i> added</li><li><strong>Status</strong> changed from <i>New</i> to <i>Confirmed</i></li></ul><p><a class="user active" href="https://www.redmine.org/users/12917">Bernhard Ganslmeier</a>, thanks for the detailed defect report.</p>
<p>I can confirm that the Edit / Delete links are shown even if the query is not editable by the respective user. This is confusing for the users.</p>
<p>I made 2 two tests to catch this issue: <br />- <code>test_edit_global_public_query_should_not_be_allowed_for_non_admin_users</code> which will pass in the current trunk and confirms that a non admin user doesn't has access to edit a public global query. <br />- <code>test_index_with_global_public_query_id_for_should_not_show_edit_delete_links_for_non_admin_users</code> which will fail because the links are rendered.</p>
<p>Also, I'm attaching a potential fix, but I'm not sure if is the correct solution because I don't know the reason behind overriding the <code>@query.project</code> in <code>query_helper#retrieve_query</code>.</p> Redmine - Defect #28264: Global and public custom queries are shown as editable to non administrators in projectshttps://www.redmine.org/issues/28264?journal_id=838202018-02-28T22:07:40ZMarius BĂLTEANU
<ul><li><strong>File</strong> deleted (<del><i>tests_for_28264.patch</i></del>)</li></ul> Redmine - Defect #28264: Global and public custom queries are shown as editable to non administrators in projectshttps://www.redmine.org/issues/28264?journal_id=838212018-02-28T22:07:49ZMarius BĂLTEANU
<ul><li><strong>File</strong> <a href="/attachments/20203">tests_for_28264.patch</a> <a class="icon-only icon-download" title="Download" href="/attachments/download/20203/tests_for_28264.patch">tests_for_28264.patch</a> added</li></ul> Redmine - Defect #28264: Global and public custom queries are shown as editable to non administrators in projectshttps://www.redmine.org/issues/28264?journal_id=838262018-03-01T06:51:47ZBernhard Rohloff
<ul></ul><p><code>Marius Thank you for confirming this issue.<br />I also dipped my toe into the code and the repository. I can confirm that the line you modify in your patch is the cause of that issue. You could also remove the entire line, which has the same effect as your attached conditional statement. The annotation of @queries_helper.rb</code> reveals that this line (<a class="source" href="https://www.redmine.org/projects/redmine/repository/svn/entry/trunk/app/helpers/queries_helper.rb#L297">source:trunk/app/helpers/queries_helper.rb#L297</a>) was introduced in <a class="changeset" title="Reverted removal of project assignment to query (#9108)." href="https://www.redmine.org/projects/redmine/repository/svn/revisions/7656">r7656</a> to fix <a class="issue tracker-1 status-5 priority-4 priority-default closed" title="Defect: Custom query not saving status filter (Closed)" href="https://www.redmine.org/issues/9108">#9108</a>.</p>
<p>Applying your patch (or removing the line, as I did) breaks the fix for <a class="issue tracker-1 status-5 priority-4 priority-default closed" title="Defect: Custom query not saving status filter (Closed)" href="https://www.redmine.org/issues/9108">#9108</a> and causes failing tests in <code>issues_controller_test.rb</code>.</p>
<p>I seems to me that overriding the queries project id in <a class="changeset" title="Reverted removal of project assignment to query (#9108)." href="https://www.redmine.org/projects/redmine/repository/svn/revisions/7656">r7656</a> was a small and cheap hack to fight the symptoms but not the cause of the problem.<br />The actual problem of <a class="issue tracker-1 status-5 priority-4 priority-default closed" title="Defect: Custom query not saving status filter (Closed)" href="https://www.redmine.org/issues/9108">#9108</a> is the fact that the project id for the session is extracted from the query what doesn't work if the query is global and has no project.</p>
<p>So I think if we get <a class="issue tracker-1 status-5 priority-4 priority-default closed" title="Defect: Custom query not saving status filter (Closed)" href="https://www.redmine.org/issues/9108">#9108</a> properly fixed, the issue described here is solved, too.</p> Redmine - Defect #28264: Global and public custom queries are shown as editable to non administrators in projectshttps://www.redmine.org/issues/28264?journal_id=838272018-03-01T07:25:06ZMarius BĂLTEANU
<ul><li><strong>Related to</strong> <i><a class="issue tracker-1 status-5 priority-4 priority-default closed" href="/issues/9108">Defect #9108</a>: Custom query not saving status filter</i> added</li></ul> Redmine - Defect #28264: Global and public custom queries are shown as editable to non administrators in projectshttps://www.redmine.org/issues/28264?journal_id=838352018-03-01T22:04:44ZMarius BĂLTEANU
<ul></ul><p>Bernhard Rohloff wrote:</p>
<blockquote>
<p>Applying your patch (or removing the line, as I did) breaks the fix for <a class="issue tracker-1 status-5 priority-4 priority-default closed" title="Defect: Custom query not saving status filter (Closed)" href="https://www.redmine.org/issues/9108">#9108</a> and causes failing tests in <code>issues_controller_test.rb</code>.</p>
</blockquote>
<p>You're able to reproduce the issue from <a class="issue tracker-1 status-5 priority-4 priority-default closed" title="Defect: Custom query not saving status filter (Closed)" href="https://www.redmine.org/issues/9108">#9108</a> with the patch applied? I'm asking because I tried to follow the steps from the description and it worked as expected in both cases (global issues page and project issues page). Maybe I miss something.</p>
<p>I chose to add the condition instead of removing the entire line in order to change the behaviour only for global queries.</p> Redmine - Defect #28264: Global and public custom queries are shown as editable to non administrators in projectshttps://www.redmine.org/issues/28264?journal_id=838402018-03-02T06:15:15ZBernhard Rohloff
<ul></ul><p>Marius BALTEANU wrote:</p>
<blockquote>
<p>Bernhard Rohloff wrote:</p>
<blockquote>
<p>Applying your patch (or removing the line, as I did) breaks the fix for <a class="issue tracker-1 status-5 priority-4 priority-default closed" title="Defect: Custom query not saving status filter (Closed)" href="https://www.redmine.org/issues/9108">#9108</a> and causes failing tests in <code>issues_controller_test.rb</code>.</p>
</blockquote>
<p>You're able to reproduce the issue from <a class="issue tracker-1 status-5 priority-4 priority-default closed" title="Defect: Custom query not saving status filter (Closed)" href="https://www.redmine.org/issues/9108">#9108</a> with the patch applied? I'm asking because I tried to follow the steps from the description and it worked as expected in both cases (global issues page and project issues page). Maybe I miss something.</p>
</blockquote>
<p>Oh I'm sorry! I mixed the issue numbers up... <a class="issue tracker-1 status-5 priority-4 priority-default closed" title="Defect: Setting of cross-project custom query is not remembered inside project (Closed)" href="https://www.redmine.org/issues/9738">#9738</a> is the right one. <a class="issue tracker-1 status-5 priority-4 priority-default closed" title="Defect: Custom query not saving status filter (Closed)" href="https://www.redmine.org/issues/9108">#9108</a> was the issue linked with the last changeset the line was affected by. They reattached the line because they had a problem with breaking the fix of <a class="issue tracker-1 status-5 priority-4 priority-default closed" title="Defect: Setting of cross-project custom query is not remembered inside project (Closed)" href="https://www.redmine.org/issues/9738">#9738</a>, too.</p>
<blockquote>
<p>I chose to add the condition instead of removing the entire line in order to change the behaviour only for global queries.</p>
</blockquote>
<p>As far as I could see, the <code>project_id</code> is stored within the <code>queries</code> table in the database and the queries are selected by the id of the current project. So there is no reason for overriding it with the same id except for the query is a global one.</p> Redmine - Defect #28264: Global and public custom queries are shown as editable to non administrators in projectshttps://www.redmine.org/issues/28264?journal_id=838582018-03-03T12:37:00ZMarius BĂLTEANU
<ul><li><strong>File</strong> deleted (<del><i>fix_edit_delete_query_links_for_users_without_permissions.patch</i></del>)</li></ul> Redmine - Defect #28264: Global and public custom queries are shown as editable to non administrators in projectshttps://www.redmine.org/issues/28264?journal_id=838592018-03-03T13:27:58ZMarius BĂLTEANU
<ul><li><strong>File</strong> <i>fix_edit_delete_query_links_for_users_without_permissions.patch</i> added</li></ul><p>I'm attaching an workaround which seems to work, but I'm not sure if is the proper fix. Maybe is better to add a new column (`is_for_all`) in the queries table with 0/1 values and update it using before_save hook (based on project_id value).</p>
<p>I've added Jean-Philippe Lang to this ticket, maybe he can take a look when he've time.</p>
<p>@Go Maeda, can I add this ticket to <a class="version" href="https://www.redmine.org/versions/138">3.3.7</a> or <a class="version" href="https://www.redmine.org/versions/99">4.0.0</a>? It'll be nice to fix it in the next version.</p> Redmine - Defect #28264: Global and public custom queries are shown as editable to non administrators in projectshttps://www.redmine.org/issues/28264?journal_id=839002018-03-07T16:00:32ZBernhard Rohloff
<ul><li><strong>File</strong> <a href="/attachments/20236">fix_not_initialized_variable_in_query_model.patch</a> <a class="icon-only icon-download" title="Download" href="/attachments/download/20236/fix_not_initialized_variable_in_query_model.patch">fix_not_initialized_variable_in_query_model.patch</a> added</li></ul><p>This bug caused me quite a headache but eventually I found the root of all evil in the query model.<br />The problem is that the overloaded <code>initialize</code> method doesn't get called.<br />Because of this the variable <code>@is_for_all</code> is not properly set and returns a false state in the condition statement.</p>
<p>I've fixed it now with the <code>after_initialize</code> callback method which seems to generally be the better way to do this, in reference to <a class="external" href="http://blog.dalethatcher.com/2008/03/rails-dont-override-initialize-on.html">http://blog.dalethatcher.com/2008/03/rails-dont-override-initialize-on.html</a>.</p>
<p>The attached patch is passing the tests in <code>issues_controller_test.rb</code> and <code>query_controller_test.rb</code> including the additional tests from Marius' patch.</p> Redmine - Defect #28264: Global and public custom queries are shown as editable to non administrators in projectshttps://www.redmine.org/issues/28264?journal_id=839012018-03-07T21:16:54ZMarius BĂLTEANU
<ul></ul><p>Bernhard Rohloff wrote:</p>
<blockquote>
<p>This bug caused me quite a headache but eventually I found the root of all evil in the query model.<br />The problem is that the overloaded <code>initialize</code> method doesn't get called.<br />Because of this the variable <code>@is_for_all</code> is not properly set and returns a false state in the condition statement.</p>
</blockquote>
<p>Indeed, that was my conclusion too, but I wasn't be able to find a better solution than my workaround from the previous post.</p>
<blockquote>
<p>I've fixed it now with the <code>after_initialize</code> callback method which seems to generally be the better way to do this, in reference to <a class="external" href="http://blog.dalethatcher.com/2008/03/rails-dont-override-initialize-on.html">http://blog.dalethatcher.com/2008/03/rails-dont-override-initialize-on.html</a>.</p>
</blockquote>
<p>Nice :) I didn't know about this callback. Today I learned something.</p>
<blockquote>
<p>The attached patch is passing the tests in <code>issues_controller_test.rb</code> and <code>query_controller_test.rb</code> including the additional tests from Marius' patch.</p>
</blockquote>
<p>What do you think if you add to your patch the method <code>is_for_all?</code>? Something like:<br /><pre>
def is_for_all?
@is_for_all ||= project.nil?
end
</pre><br />and use this new method in the check instead of the @is_for_all variable.</p>
<p>Also, it looks like that you are using tabs instead of two spaces for indentation.</p> Redmine - Defect #28264: Global and public custom queries are shown as editable to non administrators in projectshttps://www.redmine.org/issues/28264?journal_id=839042018-03-08T06:37:27ZBernhard Rohloff
<ul><li><strong>File</strong> <a href="/attachments/20240">fix_not_initialized_variable_in_query_model_V2.patch</a> <a class="icon-only icon-download" title="Download" href="/attachments/download/20240/fix_not_initialized_variable_in_query_model_V2.patch">fix_not_initialized_variable_in_query_model_V2.patch</a> added</li></ul><p>Marius BALTEANU wrote:</p>
<blockquote>
<p>What do you think if you add to your patch the method <code>is_for_all?</code>? Something like:<br />[...]<br />and use this new method in the check instead of the @is_for_all variable.</p>
</blockquote>
<p>Yep, that sounds good. I've implemented it in my new version of the patch and also gave it a more descriptive name.</p>
<blockquote>
<p>Also, it looks like that you are using tabs instead of two spaces for indentation.</p>
</blockquote>
<p>Indeed, some tabs have crept into the patch. I've fixed them.</p> Redmine - Defect #28264: Global and public custom queries are shown as editable to non administrators in projectshttps://www.redmine.org/issues/28264?journal_id=839092018-03-08T07:14:08ZMarius BĂLTEANU
<ul></ul><p>Bernhard Rohloff wrote:</p>
<blockquote>
<p>Yep, that sounds good. I've implemented it in my new version of the patch and also gave it a more descriptive name.</p>
<p>Indeed, some tabs have crept into the patch. I've fixed them.</p>
</blockquote>
<p>Looks good now the patch to me.</p> Redmine - Defect #28264: Global and public custom queries are shown as editable to non administrators in projectshttps://www.redmine.org/issues/28264?journal_id=839132018-03-08T07:31:07ZMarius BĂLTEANU
<ul><li><strong>File</strong> deleted (<del><i>fix_edit_delete_query_links_for_users_without_permissions.patch</i></del>)</li></ul> Redmine - Defect #28264: Global and public custom queries are shown as editable to non administrators in projectshttps://www.redmine.org/issues/28264?journal_id=840752018-03-21T09:05:55ZGo MAEDA
<ul><li><strong>Target version</strong> set to <i>4.0.0</i></li></ul><p>The following patches look good to me. Setting target version to 4.0.0.</p>
<ul>
<li><a class="attachment" href="https://www.redmine.org/attachments/20240">fix_not_initialized_variable_in_query_model_V2.patch</a></li>
<li><a class="attachment" href="https://www.redmine.org/attachments/20203">tests_for_28264.patch</a></li>
</ul>
<p>Marius BALTEANU wrote:</p>
<blockquote>
<p>@Go Maeda, can I add this ticket to <a class="version" href="https://www.redmine.org/versions/138">3.3.7</a> or <a class="version" href="https://www.redmine.org/versions/99">4.0.0</a>? It'll be nice to fix it in the next version.</p>
</blockquote>
<p>I think 4.0.0 is appropriate because the patch adds two new methods.</p> Redmine - Defect #28264: Global and public custom queries are shown as editable to non administrators in projectshttps://www.redmine.org/issues/28264?journal_id=844402018-04-08T13:24:49ZJean-Philippe Langjp_lang@yahoo.fr
<ul><li><strong>Subject</strong> changed from <i>Global and public custom queries are shown as editable to non administrators in projects.</i> to <i>Global and public custom queries are shown as editable to non administrators in projects</i></li><li><strong>Status</strong> changed from <i>Confirmed</i> to <i>Closed</i></li><li><strong>Assignee</strong> set to <i>Jean-Philippe Lang</i></li><li><strong>Resolution</strong> set to <i>Fixed</i></li></ul><p>Fixed by using a slightly different fix in <a class="changeset" title="Global and public custom queries are shown as editable to non administrators in projects (#28264)." href="https://www.redmine.org/projects/redmine/repository/svn/revisions/17292">r17292</a> with test included.<br />Thanks for pointing this out.</p> Redmine - Defect #28264: Global and public custom queries are shown as editable to non administrators in projectshttps://www.redmine.org/issues/28264?journal_id=847342018-04-29T05:29:15ZMarius BĂLTEANU
<ul><li><strong>Status</strong> changed from <i>Closed</i> to <i>Reopened</i></li></ul><p>Jean-Philippe Lang, looking at the fix committed, I'm observing that the tests "test_editable_by_for_global_query" and "test_editable_by_for_global_query_with_project_set" are identical. Is this ok?</p> Redmine - Defect #28264: Global and public custom queries are shown as editable to non administrators in projectshttps://www.redmine.org/issues/28264?journal_id=855752018-06-16T10:54:00ZJean-Philippe Langjp_lang@yahoo.fr
<ul><li><strong>Status</strong> changed from <i>Reopened</i> to <i>Closed</i></li></ul><p>Marius BALTEANU wrote:</p>
<blockquote>
<p>Jean-Philippe Lang, looking at the fix committed, I'm observing that the tests "test_editable_by_for_global_query" and "test_editable_by_for_global_query_with_project_set" are identical. Is this ok?</p>
</blockquote>
<p>Fixed, thanks!</p>