#0006939: Number of Private notes visible for reporters #43

Closed
wants to merge 9 commits into
from

Projects

None yet

4 participants

@Dentxinho

Private notes aren't counted anymore if the user is not allowed to see them.

@Dentxinho Dentxinho Do not allow the number of private notes be known by users who don't …
…have private_bugnote_threshold

Private notes aren't counted anymore if the user is not allowed to see them.
bc99228
@vboctor
Member

What's the number of queries for a full page with 50 issues before and after your changes. Is this number affected by whether issues are private or public?

@vboctor, i haven't added any queries. I've just changed the IN portion of the where condition.
Instead of always one IN (id, id...) there is now one or two INs:

(bug_id IN (id, id...) AND view_state = VS_PUBLIC) -- for issues where user can only see public bugnotes
OR
(bug_id IN (id, id...)) -- #for issues where user can see all bugnotes

To reproduce it in a full page I used my production server, which has only one project, so i can't show the result if a user has private_bugnote_threshold on only one of N projects. But as you can see in the results, this PR doesn't affect anything on the other queries.

On this test there are 3 private issues. The majority of issues have private bugnotes (the commit message is always put there as private).
Columns viewed are the same. No custom fields viewed.

An user with access to private bugnotes viewing the page:
Before: 36 queries (29 unique)
filter_cache_result() produces:

SELECT DISTINCT bug_id,MAX(last_modified) as last_modified, COUNT(last_modified) as count FROM mantis_bugnote_table WHERE mantis_bugnote_table.bug_id in (6, 62, 61, 60, 59, 57, 53, 58, 50, 42, 55, 54, 51, 43, 34, 52, 56, 30, 48, 47, 41, 37, 33, 32, 45, 49, 35, 46, 44, 38, 27, 29, 36, 31, 28, 26, 40, 39, 25, 10, 23, 22, 16, 24, 20, 21, 12, 19, 7, 14) GROUP BY bug_id

After: 36 queries (29 unique)
filter_cache_result() produces:

SELECT DISTINCT bug_id,MAX(last_modified) as last_modified, COUNT(last_modified) as count FROM mantis_bugnote_table WHERE (mantis_bugnote_table.bug_id in (6, 62, 61, 60, 59, 57, 53, 58, 50, 42, 55, 54, 51, 43, 34, 52, 56, 30, 48, 47, 41, 37, 33, 32, 45, 49, 35, 46, 44, 38, 27, 29, 36, 31, 28, 26, 40, 39, 25, 10, 23, 22, 16, 24, 20, 21, 12, 19, 7, 14)) GROUP BY bug_id

An user without access to private bugnotes viewing the page:
Before: 39 queries (33 unique)
filter_cache_result() produces:

SELECT DISTINCT bug_id,MAX(last_modified) as last_modified, COUNT(last_modified) as count FROM mantis_bugnote_table WHERE mantis_bugnote_table.bug_id in (6, 62, 61, 60, 59, 57, 53, 58, 56, 55, 54, 52, 51, 50, 43, 34, 41, 30, 37, 33, 32, 45, 49, 35, 46, 44, 38, 36, 31, 29, 28, 27, 26, 40, 39, 25, 24, 23, 22, 16, 10, 20, 21, 19, 12, 7, 18, 14, 11) GROUP BY bug_id

After: 39 queries (33 unique)
filter_cache_result() produces:

SELECT DISTINCT bug_id,MAX(last_modified) as last_modified, COUNT(last_modified) as count FROM mantis_bugnote_table WHERE (mantis_bugnote_table.bug_id in (6, 62, 61, 60, 59, 57, 53, 58, 56, 55, 54, 52, 51, 50, 43, 34, 41, 30, 37, 33, 32, 45, 49, 35, 46, 44, 38, 36, 31, 29, 28, 27, 26, 40, 39, 25, 24, 23, 22, 16, 10, 20, 21, 19, 12, 7, 18, 14, 11) AND mantis_bugnote_table.view_state = 10) GROUP BY bug_id
Dentxinho added some commits Mar 1, 2012
@Dentxinho Dentxinho Merge remote-tracking branch 'upstream/master-1.2.x' into master-1.2.x
* upstream/master-1.2.x:
  Localisation updates from http://translatewiki.net.
  Revert incorrect change to function definition
  Fixed documentation typo for column names
  Fix #11916: List of user profiles not sorted alphabetically
3e61f7c
@Dentxinho Dentxinho Do not allow the number of private notes be known by users who don't …
…have private_bugnote_threshold

Private notes aren't counted anymore if the user is not allowed to see them.
723fbb8
@Dentxinho Dentxinho Merge remote-tracking branch 'origin/master-1.2.x' into master-1.2.x
* origin/master-1.2.x:
  Do not allow the number of private notes be known by users who don't have private_bugnote_threshold
  Update version to 1.2.9
  Fixes #13994: Use rounded edges for avatars
  Correct access checks in mc_issue_get_id_from_summary
  Add regression tests for  #13901
  Fixes #13993: Support search by email in manage users page
  Fix #13992: Use Gravatar generated patterns based on email hash for users who don't have avatar
  Add "New Issue" entry to history when copying bugs
  Fix #13816: Update history when copying issues
c5a4cba
@Dentxinho Dentxinho Merge remote-tracking branch 'upstream/master-1.2.x' into master-1.2.x
* upstream/master-1.2.x:
  Updated doc/RELEASE for 1.2.10.
  Updated release notes for 1.2.9
  Update version to 1.2.10
  Issue #13998: Logo is not present on new installation.
  Issue #13998: Logo is not present on new installation.
a3b7e06
@Dentxinho Dentxinho Merge remote-tracking branch 'upstream/master-1.2.x' into master-1.2.x 7372300
@Dentxinho Dentxinho Merge remote-tracking branch 'upstream/master-1.2.x' into master-1.2.x
* upstream/master-1.2.x:
  Code cleanup in excel_xml_export.php
fd65a82
@Dentxinho Dentxinho Merge branch 'master-1.2.x' of https://github.com/mantisbt/mantisbt i…
…nto master-1.2.x

* 'master-1.2.x' of https://github.com/mantisbt/mantisbt:
  Localisation updates from http://translatewiki.net.
19da276
@Dentxinho Dentxinho Merge branch 'master-1.2.x' of https://github.com/mantisbt/mantisbt i…
…nto master-1.2.x

* 'master-1.2.x' of https://github.com/mantisbt/mantisbt: (23 commits)
  Version bump
  Update doc/RELEASE with release notes for 1.2.10.
  Added PHPdoc header for function config_is_private()
  PHPdoc and whitespace fixes in config_defaults_inc.php
  Prevent setting permanent cookie using hand-crafted login.php
  Add 'allow_permanent_cookie' to g_global_settings
  Documentation for new config $g_allow_permanent_cookie
  Fix #4465: Add config to disable 'save login' feature
  Revert "Prevent selection of released version as target"
  Fix #13785: Add filename and line number to system notices and warnings
  Avoid unnecessary execution of query logging code
  Make index variable 0-based to optimize and make code more readable
  Fix utf8 offset when logging SQL in db_query_bound
  Updated MantisTouch website url in the manual.
  Revert "Update version to 1.2.10"
  Removing previous checkins in preparation for 1.2.10.  Will re-do just before release.
  Document g_file_upload_max_num in the administration guide
  Remove duplicate error code and message
  I18l support for multi-file upload
  Add multi-file upload to bug reporting page
  ...
8ad66f3
@dregad

I think the code would be more efficient if $p_rows were grouped by project_id, and access_has_project_level() only called when the project_id changes

@dregad
Member
dregad commented Apr 16, 2012

This is not a direct consequence of @Dentxinho's patch, but I think this SQL could potentially break and cause issues due to RDBMS restrictions on the number of items in an IN statement

  • MySQL is limited by the packet size
  • Oracle can't have more than 1'000 individual items in the list (or you get ORA-01795)

Didn't check for other DB's but I'm sure they are limited as well.

Also somewhat out of scope of this PR: it is quite inefficient to use hardcoded list of values, as it causes DB to parse the query each time it's executed.

Maybe this could be addressed by using temporary table(s), but I guess that may be difficult to implement due to RDBMS-specific handling of such things

@Dentxinho

@dregad i'll take a look and see if I can find a better solution. Thanks for looking into it.

@atrol
Member
atrol commented Aug 19, 2016

Closed in favor of #856

@atrol atrol closed this Aug 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment