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 activity.getcount function to filter out unpermitted activities. #13377

Merged
merged 1 commit into from
Jan 11, 2019

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

As part of an ongoing performance refactor of the activity.get api handling this extends the application of activity
type filtering to the getcount function.

Before

Activity.getcount returns all activities matches by the input critieria when check_permissions passed even if the contact has no permission to them

After

Activity.getcount returns does not return un-permitted activity types when check_permissions passed if the contact has no permission to them. Other ACLs / permissioning still not applied to getcount calls. Test coverage added for this

Technical Details

The current methodology of the activity.get api is to load all activities that meet the passed in criteria and
then go through them one by one running a series of queries on each one to determine if the contact has
permission to see them and if not then filtering them from the array. At some point this did work with
getcount but it had a crippling performance impact. More recently getcount was simply giving a fatal error
when acls were in play and right before this patch the status was that no permissions
were applied when the action is getcount.

This alters that to partially apply permissions - notably a filter is added to the query that reflects
which activity types the contact may access - this is basically those with no component or a component
the contact has high level permissions to. It is a check that is applied in the 'allow' function
and it still will be after this but it's a fairly cheap check and it will always return TRUE
from here on as no activities will be fetched if their activity type is not available.

The mechanism is that the CRM_Utils_SQL_Select class calls the relevant addSelectWhereClause but ONLY when
check_permissions is TRUE. In this case the activity type filter is added if they don't have
permission to check all activities.

Tangental changes

  1. test enabled for getcount with activity type checks
  2. I added a type casting for activity_type_id in the function that retrieves
    and caches them - it should be pretty impossible for them not to be all integers
    but as I later concat them this seems a good security insurance
  3. I changed a very recently added function used in this from public to protected. It was added in Dec 2018
    and is only called from the Activity BAO and I want to discourage other classes from calling it
    now that I believe this is the correct approach.

Comments

@colemanw in theory if there are other places calling this methodology then activity type limits would be added where they are currently not - I couldn't see any downside but you have experience adding this addSelectWhere methodology. From a quick look I suspect that activity report doesn't apply this permission filter at all.

@civibot
Copy link

civibot bot commented Jan 1, 2019

(Standard links)

@civibot civibot bot added the master label Jan 1, 2019
@seamuslee001
Copy link
Contributor

As part of an ongoing performance refactor of the activity.get api handling this extends the application of activity
type filtering to the getcount function.

The current methodology of the activity.get api is to load all activities that meet the passed in criteria and
then go through them one by one running a series of queries on each one to determine if the contact has
permission to see them and if not then filtering them from the array. At some point this did work with
getcount but it had a crippling performance impact. More recently getcount was simply giving a fatal error
when acls were in play and right before this patch the status was that no permissions
were applied when the action is getcount.

This alters that to partially apply permissions - notably a filter is added to the query that reflects
which activity types the contact may access - this is basically those with no component or a component
the contact has high level permissions to. It is a check that is applied in the 'allow' function
and it still will be after this but it's a fairly cheap check and it will always return TRUE
from here on as no activities will be fetched if their activity type is not available.

The mechanism is that the CRM_Utils_SQL_Select class calls the relevant addSelectWhereClause but ONLY when
check_permissions is TRUE. In this case the activity type filter is added if they don't have
permission to check all activities.

Tangental changes
1) test enabled for getcount with activity type checks
2) I added a type casting for activity_type_id in the function that retrieves
and caches them - it should be pretty impossible for them not to be all integers
but as I later concat them this seems a good security insurance
3) I changed a very recently added function used in this from public to protected. It was added in Dec 2018
and is only called from the Activity BAO and I want to discourage other classes from calling it
now that I believe this is the correct approach.
@eileenmcnaughton
Copy link
Contributor Author

thanks @seamuslee001 - trying again!

@colemanw
Copy link
Member

@colemanw colemanw merged commit 333f9b4 into civicrm:master Jan 11, 2019
@colemanw colemanw deleted the activity_extract2 branch January 11, 2019 00:54
@eileenmcnaughton
Copy link
Contributor Author

@colemanw thanks for merging - do you endorse me expanding this approach to cover the contact filtering acls to also work with getcount - it feels like the approach you were working towards I just want confirmation

@colemanw
Copy link
Member

Yes that sounds good @eileenmcnaughton

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants