From df29ed8075cfd1826c6760b2abf1460b441653dd Mon Sep 17 00:00:00 2001 From: Simon Pelletier Date: Mon, 9 Nov 2020 14:00:29 -0500 Subject: [PATCH] [API] Fix PUT request for Candidates \visit endpoint and integration test (#7088) The PUT request should never allow to edit a candidate from a Site where the User has no affiliation with. In this case, the test User has only affiliation with Data Coordinating Center (DCC). It is however possible to do so if the new Site is DCC. For example, in raisinbread, candidate 300001 (https://.loris.ca/api/v0.0.4-dev/candidates/300001) is from site Montreal. The test user should not be able to modify this candidate, because the candidate is from Montreal, but is able to. Also adds automatic the integration tests to test the changes suggested. Resolves #7106 --- .../endpoints/candidate/visit/visit.class.inc | 9 +- .../test/api/LorisApiAuthenticatedTest.php | 58 ++++++++ raisinbread/test/api/LorisApiVisitsTest.php | 137 +++++++++++++++++- 3 files changed, 202 insertions(+), 2 deletions(-) diff --git a/modules/api/php/endpoints/candidate/visit/visit.class.inc b/modules/api/php/endpoints/candidate/visit/visit.class.inc index c1f6b10566f..fc3772a56e1 100644 --- a/modules/api/php/endpoints/candidate/visit/visit.class.inc +++ b/modules/api/php/endpoints/candidate/visit/visit.class.inc @@ -215,11 +215,18 @@ class Visit extends Endpoint implements \LORIS\Middleware\ETagCalculator $this->_candidate->getListOfVisitLabels() ); + if (!$this->_candidate->isAccessibleBy($user)) { + return new \LORIS\Http\Response\JSON\Forbidden( + "You can't create or modify that candidate" + ); + } + $centerid = array_search($visitinfo['Site'], \Utility::getSiteList()); if (!in_array($centerid, $user->getCenterIDs())) { return new \LORIS\Http\Response\JSON\Forbidden( - 'You can`t create candidates visit for that site' + "You can't create or modify candidates visit for the site " . + $centerid ); } diff --git a/raisinbread/test/api/LorisApiAuthenticatedTest.php b/raisinbread/test/api/LorisApiAuthenticatedTest.php index 783bb750e3c..3ac2a517c60 100644 --- a/raisinbread/test/api/LorisApiAuthenticatedTest.php +++ b/raisinbread/test/api/LorisApiAuthenticatedTest.php @@ -75,6 +75,59 @@ public function setUp() $this->DB->update('Config', $set, $where); $this->apiLogin('UnitTester', $this->validPassword); + + + $this->DB->insert( + "candidate", + [ + 'CandID' => '900000', + 'PSCID' => 'TST0001', + 'RegistrationCenterID' => 1, + 'RegistrationProjectID' => 1, + 'Active' => 'Y', + 'UserID' => 1, + 'Entity_type' => 'Human', + 'Sex' => 'Female' + ] + ); + $this->DB->insert( + 'session', + [ + 'ID' => '999999', + 'CandID' => '900000', + 'Visit_label' => 'V1', + 'CenterID' => 1, + 'ProjectID' => 1, + 'Current_stage' => 'Not Started', + ] + ); + $this->DB->insert( + 'test_names', + [ + 'ID' => '999999', + 'Test_name' => 'testtest', + 'Full_name' => 'Test Test', + 'Sub_group' => 1, + ] + ); + $this->DB->insert( + 'flag', + [ + 'ID' => '999999', + 'SessionID' => '999999', + 'Test_name' => 'testtest', + 'CommentID' => '11111111111111111', + ] + ); + $this->DB->insert( + 'flag', + [ + 'ID' => '999999', + 'SessionID' => '999999', + 'Test_name' => 'testtest', + 'CommentID' => 'DDE_11111111111111111', + ] + ); } /** @@ -131,6 +184,11 @@ function testLoginSuccess() */ public function tearDown() { + $this->DB->delete("session", ['CandID' => '900000']); + $this->DB->delete("candidate", ['CandID' => '900000']); + $this->DB->delete("flag", ['ID' => '999999']); + $this->DB->delete("test_names", ['ID' => '999999']); + $set = [ 'Value' => $this->originalJwtKey ]; diff --git a/raisinbread/test/api/LorisApiVisitsTest.php b/raisinbread/test/api/LorisApiVisitsTest.php index 2aec6340417..9e3150c61e2 100644 --- a/raisinbread/test/api/LorisApiVisitsTest.php +++ b/raisinbread/test/api/LorisApiVisitsTest.php @@ -114,7 +114,142 @@ public function testGetCandidatesCandidVisit(): void */ public function testPutCandidatesCandidVisit(): void { - $this->markTestSkipped('No access to create visits for any site'); + // Test changing the Project & Battery + $json = ['CandID' => '900000', + 'Visit' => "V1", + 'Site' => "Data Coordinating Center", + 'Battery' => "High Yeast", + 'Project' => "Rye", + ]; + $response = $this->client->request( + 'PUT', + "candidates/900000/V1", + [ + 'headers' => $this->headers, + 'json' => $json + ] + ); + // Verify the status code + $this->assertEquals(204, $response->getStatusCode()); + // Verify the endpoint has a body + $body = $response->getBody(); + $this->assertNotEmpty($body); + + /** + * Test changing from a site with no affiliation to a site with affiliation + * Candidate 400266 is from site Rome. The test user only has access to + * Data Coordinating Center. He should not be able to modify a visit + * for a candidate from a site he has no access to. + */ + $json = ['CandID' => '400266', + 'Visit' => "V3", + 'Site' => "Data Coordinating Center", + 'Battery' => "Stale", + 'Project' => "Pumpernickel", + ]; + $response = $this->client->request( + 'PUT', + "candidates/400266/V3", + [ + 'headers' => $this->headers, + 'json' => $json, + 'http_errors' => false + ] + ); + // Verify the status code + $this->assertEquals(403, $response->getStatusCode()); + // Verify the endpoint has a body + $body = $response->getBody(); + $this->assertNotEmpty($body); + + // Test changing the Battery from a visit that is already initiated. + $json = ['CandID' => "115788", + 'Visit' => "V3", + 'Site' => "Data Coordinating Center", + 'Battery' => "Stale", + 'Project' => "Pumpernickel", + ]; + $response = $this->client->request( + 'PUT', + "candidates/115788/V3", + [ + 'headers' => $this->headers, + 'json' => $json, + 'http_errors' => false + ] + ); + // verify the status code + $this->assertequals(409, $response->getstatuscode()); + // verify the endpoint has a body + $body = $response->getbody(); + $this->assertnotempty($body); + + // Test assigning site with no affiliation. It changes the site from + // Data Coordinating Center, which the test user has its only affiliation, + // to Montreal, where he has no affiliation. + $json = ['CandID' => '900000', + 'Visit' => "V1", + 'Site' => "Montreal", + 'Battery' => "Stale", + 'Project' => "Pumpernickel", + ]; + $response = $this->client->request( + 'PUT', + "candidates/900000/V1", + [ + 'headers' => $this->headers, + 'json' => $json, + 'http_errors' => false + ] + ); + // Verify the status code + $this->assertEquals(403, $response->getStatusCode()); + // Verify the endpoint has a body + $body = $response->getBody(); + $this->assertNotEmpty($body); + + // Test what happen when a field is missing (here, Battery) + $json = ['CandID' => $this->candidTest, + 'Visit' => $this->visitTest, + 'Site' => "Data Coordinating Center", + 'Project' => "Pumpernickel", + ]; + $response = $this->client->request( + 'PUT', + "candidates/$this->candidTest/$this->visitTest", + [ + 'headers' => $this->headers, + 'json' => $json, + 'http_errors' => false + ] + ); + // verify the status code + $this->assertequals(400, $response->getstatuscode()); + // verify the endpoint has a body + $body = $response->getbody(); + $this->assertnotempty($body); + + // Test CandID in URL should match CandID in the request fields + $json = ['CandID' => $this->candidTest, + 'Visit' => $this->visitTest, + 'Site' => "Montreal", + 'Battery' => "Low Yeast", + 'Project' => "Pumpernickel", + ]; + $response = $this->client->request( + 'PUT', + "candidates/300001/$this->visitTest", + [ + 'headers' => $this->headers, + 'json' => $json, + 'http_errors' => false + ] + ); + // verify the status code + $this->assertequals(400, $response->getstatuscode()); + // verify the endpoint has a body + $body = $response->getbody(); + $this->assertnotempty($body); } /**