This started out as a support ticket investigation. One of our clients was receiving message board email notifications from a category that she was not subscribed to and more importantly this message was from a category that was restricted at the team level and she was not included within that team membership.
At the time of the incident we had not enabled the functionality to include the body of the message posted to the forum, so all she received was a link. On clicking that link and logging in she was informed that she did not have the permissions required to view the message in question.
When impersonating this user, it was clear that she could not see anything related to the category in question - not in the Recent Posts section, not in the forum category / posts lists.
We then discovered that she had subscribed at the root category and it appeared that this was allowing her to receive these email notifications.
Since we wanted to offer the ability to include the message body within the email and then allow recipients to reply back to the message board via email – this security hole was a clear blocker.
Tracing through from the point at which an com.liferay.portlet.messageboards.model.MBMessage is persisted to where a forum subscriber is sent a notification of this new message led us to the heart of the problem – the com.liferay.portal.util.SubscriptionSender class.
Within its hasPermission() method - the permission check that is initiated via the com.liferay.portal.service.permission.SubscriptionPermissionImpl.contains() method is a check against the current user's permissions for a SUBSCRIBE action on either the MBCategory or MBThread model resource.
The problem lies within the MBCategory model, not the MBThread model. The MBThread model SUBSCRIBE permission is validated by using the com.liferay.portlet.messageboards.service.permission.MBMessagePermission.contains() method which - if permissions.view.dynamic.inheritance is set to TRUE in the Liferay server - will result in a walk up the message's category ancestry checking each of them for VIEW action permission restrictions.
This is NOT the case for the MBCategory model. Instead - the only check that is made is whether the user is subscribed to the current category or any of its parent categories. This causes issues should ANY sub-category below where the user is subscribed be restricted, because the current implementation does not check for those restrictions – it only cares about restrictions at the current category or higher (if applicable).
- User "Boris" is not an admin
- User "Boris" is not a member of the team Special Folks
Example Forum Configuration
In the above example - if Boris subscribes to either root-category or category-2 - without our fix in place, he will receive all messages posted to cat2_sub-cat1-thread1 and any subsequent threads that are posted under this restricted category.
This is because, as explained above, if Boris is subscribing to a higher level category - there will never be a permissions check made to see if he is even allowed to see a message posted to cat2_sub-cat1-thread1.
- Modify SubscriptionSender to change the access modifier on the _persistestedSubscribersOVPs member variable from private to protected
- Modify MBSubscriptionSender
- Override the initialize() method to retrieve the messageThreadId from the _persistestedSubscribersOVPs collection and set it as a private variable after calling the parent's implementation
- Override the hasPermission() method to incorporate logic to see if the current user is allowed to see the message associated with the MessageThread. Since we are checking at the MBThread level - this is guaranteed to work correctly as described above. Only once we have determined that they are allowed to view the message do we then call up to the parent's implementation to see if they have an actual subscription to the current or ancestor categories.
Please see attached files for change diffs of our internally implemented solution.