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.

Sign up for GitHub

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

PCHR-3820: Add Leave Request ACL Rules to LeaveRequest.getAttachments API #2682

Merged
merged 6 commits into from
Jun 14, 2018
Merged

PCHR-3820: Add Leave Request ACL Rules to LeaveRequest.getAttachments API #2682

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c4c15dd
PCHR-3820: Add the LeaveAttachment Service Get method. Add tests.
tunbola Jun 12, 2018
c87b59a
PCHR-3820: Fix broken test.
tunbola Jun 12, 2018
00a4fe0
PCHR-3820: Refactor constructor for the LeaveAttachment Service. Use …
tunbola Jun 13, 2018
6c3b86b
PCHR-3820: Add tests for LeaveRequestAttachments Service GET method.
tunbola Jun 13, 2018
6229df3
PCHR-3820: Rename Test.
tunbola Jun 13, 2018
20d8441
PCHR-3820: Refactor some tests. Fix typo.
tunbola Jun 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 46 additions & 2 deletions ...corp.civicrm.hrleaveandabsences/CRM/HRLeaveAndAbsences/Service/LeaveRequestAttachment.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,31 @@

use CRM_HRLeaveAndAbsences_BAO_LeaveRequest as LeaveRequest;
use CRM_HRLeaveAndAbsences_Service_LeaveManager as LeaveManagerService;
use CRM_HRLeaveAndAbsences_Service_LeaveRequestRights as LeaveRequestRightsService;

class CRM_HRLeaveAndAbsences_Service_LeaveRequestAttachment {

/**
* @var LeaveManagerService
*/
private $leaveManagerService;

/**
* @var LeaveRequestRightsService
*/
private $leaveRequestRightsService;

/**
* CRM_HRLeaveAndAbsences_Service_LeaveRequestAttachment constructor.
*
* @param LeaveRequestRightsService $leaveRequestRights
* @param LeaveManagerService $leaveManager
*/
public function __construct(LeaveRequestRightsService $leaveRequestRights, LeaveManagerService $leaveManager) {
$this->leaveManagerService = $leaveManager;
$this->leaveRequestRightsService = $leaveRequestRights;
}

/**
* Uses the Attachment API to delete an attachment associated with a LeaveRequest.
* This method also implement some checks to ensure that only the LeaveRequest Approver
Expand All @@ -23,9 +45,8 @@ public function delete($params) {

if ($attachment['count'] > 0) {
$leaveRequest = LeaveRequest::findById($attachment['values'][0]['entity_id']);
$leaveManagerService = new LeaveManagerService();

if ($leaveManagerService->currentUserIsAdmin() || $leaveManagerService->currentUserIsLeaveManagerOf($leaveRequest->contact_id)) {
if ($this->leaveManagerService->currentUserIsAdmin() || $this->leaveManagerService->currentUserIsLeaveManagerOf($leaveRequest->contact_id)) {
return $this->callAttachmentAPI('delete', $params);
}

Expand Down Expand Up @@ -72,4 +93,27 @@ private function callAttachmentAPI($action, $params) {

return civicrm_api3('Attachment', $action, $params);
}

/**
* Uses the Attachment API to retrieve attachments associated with a LeaveRequest.
* It ensures that the current user can only retrieve Leave attachments for the
* Leave requests linked to the contacts the user has access to. The admin can
* retrieve all Leave attachments for all contacts.
*
* @param array $params
* @param LeaveRequestRights $leaveRequestRights
*
* @return array
*/
public function get($params) {
$leaveRequestID = isset($params['entity_id']) ? $params['entity_id'] : '';
$leaveRequest = LeaveRequest::findById($leaveRequestID);
$accessibleContacts = $this->leaveRequestRightsService->getLeaveContactsCurrentUserHasAccessTo();

if ($this->leaveManagerService->currentUserIsAdmin() || in_array($leaveRequest->contact_id, $accessibleContacts)) {
return $this->callAttachmentAPI('get', $params);
}

return [];
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also write tests for this method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking it will be a duplication of the tests for the LeaveRequest.getAttachments API, since this API uses this method directly.

I could modify this method to accept params and also the LeaveRequestRights service so that I can mock that and pass it in the function but I would still have to create leave requests and all. What do you suggest?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, because of how the API works, there's no way to avoid this duplication. The tests wouldn't be completely duplicated though, since, as you said, you can mock a few things. In this case it's better to have some duplication than not having tests for this method

}
15 changes: 11 additions & 4 deletions uk.co.compucorp.civicrm.hrleaveandabsences/api/v3/LeaveRequest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -709,12 +709,17 @@ function _civicrm_api3_leave_request_getattachments_spec(&$spec) {
function civicrm_api3_leave_request_getattachments($params) {
$params['entity_id'] = $params['leave_request_id'];
$params['entity_table'] = CRM_HRLeaveAndAbsences_BAO_LeaveRequest::getTableName();
$leaveManagerService = new CRM_HRLeaveAndAbsences_Service_LeaveManager();
$leaveRequestRights = new CRM_HRLeaveAndAbsences_Service_LeaveRequestRights($leaveManagerService);
$leaveRequestAttachmentService = new CRM_HRLeaveAndAbsences_Service_LeaveRequestAttachment($leaveRequestRights, $leaveManagerService);
$result = $leaveRequestAttachmentService->get($params);

$result = civicrm_api3('Attachment', 'get', $params);

if ($result['count'] > 0) {
if (!empty($result)) {
array_walk($result['values'], '_civicrm_api3_leave_request_filter_attachment_fields');
}
else{
$result = civicrm_api3_create_success([]);
}

return $result;
}
Expand Down Expand Up @@ -766,7 +771,9 @@ function _civicrm_api3_leave_request_deleteattachment_spec(&$spec) {
* @return array
*/
function civicrm_api3_leave_request_deleteattachment($params) {
$leaveRequestAttachmentService = new CRM_HRLeaveAndAbsences_Service_LeaveRequestAttachment();
$leaveManagerService = new CRM_HRLeaveAndAbsences_Service_LeaveManager();
$leaveRequestRights = new CRM_HRLeaveAndAbsences_Service_LeaveRequestRights($leaveManagerService);
$leaveRequestAttachmentService = new CRM_HRLeaveAndAbsences_Service_LeaveRequestAttachment($leaveRequestRights, $leaveManagerService);
return $leaveRequestAttachmentService->delete($params);
}

Expand Down
144 changes: 116 additions & 28 deletions ...veandabsences/tests/phpunit/CRM/HRLeaveAndAbsences/Service/LeaveRequestAttachmentTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
use CRM_HRCore_Test_Fabricator_Contact as ContactFabricator;
use CRM_HRLeaveAndAbsences_Test_Fabricator_LeaveRequest as LeaveRequestFabricator;
use CRM_HRLeaveAndAbsences_Service_LeaveRequestAttachment as LeaveRequestAttachmentService;
use CRM_HRLeaveAndAbsences_Service_LeaveManager as LeaveManagerService;
use CRM_HRLeaveAndAbsences_Service_LeaveRequestRights as LeaveRightsService;

/**
* Class CRM_HRLeaveAndAbsences_Service_LeaveRequestAttachmentTest
Expand All @@ -15,14 +17,12 @@ class CRM_HRLeaveAndAbsences_Service_LeaveRequestAttachmentTest extends BaseHead
use CRM_HRLeaveAndAbsences_SessionHelpersTrait;
use CRM_HRLeaveAndAbsences_LeaveRequestHelpersTrait;

private $leaveRequestAttachmentService;

private $leaveContact;

public function setUp() {
CRM_Core_DAO::executeQuery('SET foreign_key_checks = 0;');

$this->leaveRequestAttachmentService = new LeaveRequestAttachmentService();
$this->leaveContact = 1;
}

Expand All @@ -42,18 +42,18 @@ public function testDeleteShouldThrowAnExceptionWhenLoggedInUserIsNotAnAdminOrLe
$this->registerCurrentLoggedInContactInSession($contactID);
CRM_Core_Config::singleton()->userPermissionClass->permissions = [];

$leaveManagerService = new LeaveManagerService();
$leaveRequestRights = new LeaveRightsService($leaveManagerService);
$leaveRequestAttachmentService = new LeaveRequestAttachmentService($leaveRequestRights, $leaveManagerService);

$attachment = $this->createAttachmentForLeaveRequest(['entity_id' => $leaveRequest->id]);
$this->leaveRequestAttachmentService->delete(['leave_request_id' => $leaveRequest->id, 'attachment_id' => $attachment['id']]);
$leaveRequestAttachmentService->delete(['leave_request_id' => $leaveRequest->id, 'attachment_id' => $attachment['id']]);
}

public function testDeleteShouldDeleteAttachmentWhenLoggedInUserIsAnAdmin() {
$adminID = 2;
$params = $this->getDefaultLeaveRequestParams();
$leaveRequest = LeaveRequestFabricator::fabricateWithoutValidation($params);

// Register contact in session and set permission to admin
$this->registerCurrentLoggedInContactInSession($adminID);
CRM_Core_Config::singleton()->userPermissionClass->permissions = ['administer leave and absences'];
$leaveRequestAttachmentService = $this->createLeaveRequestAttachmentsServiceWhenUserIsAdmin();

$attachment = $this->createAttachmentForLeaveRequest(['entity_id' => $leaveRequest->id]);
$attachment2 = $this->createAttachmentForLeaveRequest(['entity_id' => $leaveRequest->id]);
Expand All @@ -62,7 +62,7 @@ public function testDeleteShouldDeleteAttachmentWhenLoggedInUserIsAnAdmin() {
$attachmentList = $this->getAttachmentForLeaveRequest(['entity_id' => $leaveRequest->id]);
$this->assertEquals($attachmentList['count'], 2);

$result = $this->leaveRequestAttachmentService->delete(['leave_request_id' => $leaveRequest->id, 'attachment_id' => $attachment['id']]);
$result = $leaveRequestAttachmentService->delete(['leave_request_id' => $leaveRequest->id, 'attachment_id' => $attachment['id']]);

$expected = [
'is_error' => 0,
Expand All @@ -78,22 +78,21 @@ public function testDeleteShouldDeleteAttachmentWhenLoggedInUserIsAnAdmin() {
}

public function testDeleteShouldDeleteAttachmentWhenLoggedInUserIsTheLeaveApprover() {
$manager = ContactFabricator::fabricate();
$leaveContact = ContactFabricator::fabricate();
$params = $this->getDefaultLeaveRequestParams(['contact_id' => $leaveContact['id']]);
$leaveRequest = LeaveRequestFabricator::fabricateWithoutValidation($params);

// Set logged in user as manager of Contact who requested leave
$this->registerCurrentLoggedInContactInSession($manager['id']);
$this->setContactAsLeaveApproverOf($manager, $leaveContact);
CRM_Core_Config::singleton()->userPermissionClass->permissions = [];

$leaveManagerService = $this->getLeaveManagerServiceWhenUserIsLeaveApprover($leaveContact['id']);
$leaveRequestRights = new LeaveRightsService($leaveManagerService);
$leaveRequestAttachmentService = new LeaveRequestAttachmentService($leaveRequestRights, $leaveManagerService);

$attachment = $this->createAttachmentForLeaveRequest(['entity_id' => $leaveRequest->id]);

//confirm that only one attachment exist for the leave request before deletion
$attachmentList = $this->getAttachmentForLeaveRequest(['entity_id' => $leaveRequest->id]);
$this->assertEquals($attachmentList['count'], 1);
$result = $this->leaveRequestAttachmentService->delete(['leave_request_id' => $leaveRequest->id, 'attachment_id' => $attachment['id']]);
$result = $leaveRequestAttachmentService->delete(['leave_request_id' => $leaveRequest->id, 'attachment_id' => $attachment['id']]);

$expected = [
'is_error' => 0,
Expand All @@ -108,36 +107,28 @@ public function testDeleteShouldDeleteAttachmentWhenLoggedInUserIsTheLeaveApprov
}

public function testDeleteShouldThrowAnExceptionWhenAttachmentHasBeenDeletedBefore() {
$adminID = 2;
$params = $this->getDefaultLeaveRequestParams();
$leaveRequest = LeaveRequestFabricator::fabricateWithoutValidation($params);

// Register contact in session and set permission to admin
$this->registerCurrentLoggedInContactInSession($adminID);
CRM_Core_Config::singleton()->userPermissionClass->permissions = ['administer leave and absences'];
$leaveRequestAttachmentService = $this->createLeaveRequestAttachmentsServiceWhenUserIsAdmin();

$attachment = $this->createAttachmentForLeaveRequest(['entity_id' => $leaveRequest->id]);

$this->leaveRequestAttachmentService->delete(['leave_request_id' => $leaveRequest->id, 'attachment_id' => $attachment['id']]);
$leaveRequestAttachmentService->delete(['leave_request_id' => $leaveRequest->id, 'attachment_id' => $attachment['id']]);

//try delete attachment again
$this->setExpectedException('InvalidArgumentException', 'Attachment does not exist or has been deleted already!');
$this->leaveRequestAttachmentService->delete(['leave_request_id' => $leaveRequest->id, 'attachment_id' => $attachment['id']]);
$leaveRequestAttachmentService->delete(['leave_request_id' => $leaveRequest->id, 'attachment_id' => $attachment['id']]);
}

/**
* @expectedException InvalidArgumentException
* @expectedExceptionMessage Attachment does not exist or has been deleted already!
*/
public function testDeleteShouldThrowAnExceptionWhenAttachmentDoesNotExist() {
$adminID = 2;
$leaveRequestID = 1;
$leaveRequestAttachmentService = $this->createLeaveRequestAttachmentsServiceWhenUserIsAdmin();

// Register contact in session and set permission to admin
$this->registerCurrentLoggedInContactInSession($adminID);
CRM_Core_Config::singleton()->userPermissionClass->permissions = ['administer leave and absences'];

$this->leaveRequestAttachmentService->delete(['leave_request_id' => $leaveRequestID, 'attachment_id' => 1]);
$leaveRequestAttachmentService->delete(['leave_request_id' => $leaveRequestID, 'attachment_id' => 1]);
}

private function getDefaultLeaveRequestParams($params = []) {
Expand All @@ -151,4 +142,101 @@ private function getDefaultLeaveRequestParams($params = []) {
];
return array_merge($defaultParams, $params);
}


public function testGetReturnsLeaveAttachmentDataOnlyForContactAUserHasAccessTo() {
$staff1 = 1;
$staff2 = 2;
$params1 = $this->getDefaultLeaveRequestParams(['contact_id' => $staff1]);
$params2 = $this->getDefaultLeaveRequestParams(['contact_id' => $staff2]);

//Create Leave requests
$leaveRequest1 = LeaveRequestFabricator::fabricateWithoutValidation($params1);
$leaveRequest2 = LeaveRequestFabricator::fabricateWithoutValidation($params2);

//create attachments for Leave requests
$attachment1 = $this->createAttachmentForLeaveRequest(['entity_id' => $leaveRequest1->id]);
$attachment2 = $this->createAttachmentForLeaveRequest(['entity_id' => $leaveRequest2->id]);


//Register the staff1 in session
$this->registerCurrentLoggedInContactInSession($staff1);
CRM_Core_Config::singleton()->userPermissionClass->permissions = [];

$leaveManagerService = new LeaveManagerService();
$leaveRightsService = $this->prophesize(LeaveRightsService::class);

// staff1 has access to own self alone
$leaveRightsService->getLeaveContactsCurrentUserHasAccessTo()->willReturn([$staff1]);
$leaveRequestAttachmentService = new LeaveRequestAttachmentService($leaveRightsService->reveal(), $leaveManagerService);

$staff1Attachment = $leaveRequestAttachmentService->get(['entity_id' => $leaveRequest1->id, 'sequential' => 1]);
$staff2Attachment = $leaveRequestAttachmentService->get(['entity_id' => $leaveRequest2->id, 'sequential' => 1]);

//result would be empty for contact user does not have access to
$this->assertEmpty($staff2Attachment);

$this->assertCount(1, $staff1Attachment['values']);
$this->assertEquals($staff1Attachment['values'][0]['id'], $attachment1['id']);
$this->assertEquals($staff1Attachment['values'][0]['name'], $attachment1['name']);
}

public function testGetReturnsLeaveAttachmentDataOnlyForAllContactsForAdmin() {
$staff1 = 1;
$staff2 = 2;
$params1 = $this->getDefaultLeaveRequestParams(['contact_id' => $staff1]);
$params2 = $this->getDefaultLeaveRequestParams(['contact_id' => $staff2]);

//Create Leave requests
$leaveRequest1 = LeaveRequestFabricator::fabricateWithoutValidation($params1);
$leaveRequest2 = LeaveRequestFabricator::fabricateWithoutValidation($params2);

//create attachments for Leave requests
$attachment1 = $this->createAttachmentForLeaveRequest(['entity_id' => $leaveRequest1->id]);
$attachment2 = $this->createAttachmentForLeaveRequest(['entity_id' => $leaveRequest2->id]);

$leaveManagerService = $this->getLeaveManagerServiceWhenUserIsAdmin();
$leaveRightsService = $this->prophesize(LeaveRightsService::class);
$leaveRightsService->getLeaveContactsCurrentUserHasAccessTo()->willReturn([]);
$leaveRequestAttachmentService = new LeaveRequestAttachmentService($leaveRightsService->reveal(), $leaveManagerService);

$staff1Attachment = $leaveRequestAttachmentService->get(['entity_id' => $leaveRequest1->id, 'sequential' => 1]);
$staff2Attachment = $leaveRequestAttachmentService->get(['entity_id' => $leaveRequest2->id, 'sequential' => 1]);

//Admin is able to access attachments for all contacts
$this->assertCount(1, $staff1Attachment['values']);
$this->assertEquals($staff1Attachment['values'][0]['id'], $attachment1['id']);
$this->assertEquals($staff1Attachment['values'][0]['name'], $attachment1['name']);

$this->assertCount(1, $staff2Attachment['values']);
$this->assertEquals($staff2Attachment['values'][0]['id'], $attachment2['id']);
$this->assertEquals($staff2Attachment['values'][0]['name'], $attachment2['name']);
}

private function getLeaveManagerService($isAdmin, $leaveContact = NULL) {
$leaveManagerService = $this->prophesize(LeaveManagerService::class);
$leaveManagerService->currentUserIsAdmin()->willReturn($isAdmin);

if ($leaveContact) {
$leaveManagerService->currentUserIsLeaveManagerOf($leaveContact)->willReturn(TRUE);
}

return $leaveManagerService->reveal();
}

private function getLeaveManagerServiceWhenUserIsAdmin() {
return $this->getLeaveManagerService(TRUE);
}

private function getLeaveManagerServiceWhenUserIsLeaveApprover($leaveContact) {
return $this->getLeaveManagerService(FALSE, $leaveContact);
}

private function createLeaveRequestAttachmentsServiceWhenUserIsAdmin() {
$leaveManagerService = $this->getLeaveManagerServiceWhenUserIsAdmin();
$leaveRequestRights = new LeaveRightsService($leaveManagerService);
$leaveRequestAttachmentService = new LeaveRequestAttachmentService($leaveRequestRights, $leaveManagerService);

return $leaveRequestAttachmentService;
}
}
Loading