[API] Fix PUT request for Candidates \visit endpoint and integration test #7088

spell00 · 2020-10-08T07:09:50Z

Brief summary of changes

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://<hostname>.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.

Testing instructions

The testing is made automatic.

To test manually:

Before the test, make sure the User that will be used don't have affiliation with Montreal.
First get the API token using: curl https://<hostname>/api/v0.0.3/login -d '{"username": "<username>", "password": "<password>"}'
In the terminal, enter curl -X PUT -H "Authorization: Bearer $token" https://<hostname>/api/v0.0.4-dev/candidates/300001/V3 -d '{"CandID":"300001","Visit":"V3","Site":"Montreal","Battery":"Low Yeast","Project":"Pumpernickel"}'

Link(s) to related issue(s)

Resolves The PUT request allows to edit a candidate from a Site where the User has no affiliation with (but should not) #7106

raisinbread/test/api/LorisApiVisitsTest.php

christinerogers · 2020-11-02T20:08:19Z

@driusan requesting final review - this is our last stretch in which @spell00 can readily make changes. thank you!

spell00 · 2020-11-02T20:31:40Z

blocked by #7101

spell00 · 2020-11-05T07:55:35Z

@xlecours @driusan It is passing Travis checks now and is ready for final review

driusan · 2020-11-05T13:39:39Z

modules/api/php/endpoints/candidate/visit/visit.class.inc

@@ -215,11 +215,19 @@ class Visit extends Endpoint implements \LORIS\Middleware\ETagCalculator
            $this->_candidate->getListOfVisitLabels()
        );

+        if (!in_array($this->_candidate->getCenterID(), $user->getCenterIDs())) {


The Candidate class implements the AccessibleResource interface. I think this should just be using$this->_candidate->isAccessibleBy($user) instead of duplicating the logic in slightly different ways in the API. (That would also handle Project permissions, which this is currently not doing.)

It is now using $this->_candidate->isAccessibleBy($user)

spell00 · 2020-11-06T16:23:15Z

@driusan I did the correction, this is ready for re-review

driusan · 2020-11-06T18:35:01Z

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'


Suggested change

'You can`t create or modify that candidate'

"You can't create or modify that candidate"

"can't" doesn't have a backtick in it.

I changed it

driusan · 2020-11-06T18:35:23Z

modules/api/php/endpoints/candidate/visit/visit.class.inc

        $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 ' .


Suggested change

'You can`t create or modify candidates visit for the site ' .

"You can't create or modify candidates visit for the site " .

spell00 · 2020-11-06T18:57:54Z

@driusan I did all the corrections

driusan

pending Travis

spell00 · 2020-11-06T21:57:50Z

@driusan It passed Travis

…test (aces#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://<hostname>.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 aces#7106

spell00 marked this pull request as ready for review October 8, 2020 18:56

spell00 added Area: API PR or issue related to the API Category: Bug PR or issue that aims to report or fix a bug Testing PR contains test plan or automated test code (or config files for Travis) labels Oct 9, 2020

spell00 added "Help! I don't understand Travis!" PR is having a beef with TRAVIS. Someone needs to help and removed "Help! I don't understand Travis!" PR is having a beef with TRAVIS. Someone needs to help labels Oct 19, 2020

spell00 force-pushed the 2020-10-07-FixPutVisitEndpoint branch from 77970fe to 9c9d96f Compare October 29, 2020 17:22

spell00 requested a review from xlecours October 29, 2020 20:02

xlecours requested changes Oct 30, 2020

View reviewed changes

raisinbread/test/api/LorisApiVisitsTest.php Show resolved Hide resolved

raisinbread/test/api/LorisApiVisitsTest.php Outdated Show resolved Hide resolved

spell00 requested a review from xlecours October 30, 2020 14:26

xlecours approved these changes Oct 30, 2020

View reviewed changes

spell00 requested a review from driusan October 30, 2020 14:55

spell00 added the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Nov 2, 2020

spell00 added 15 commits November 4, 2020 13:36

the put request should not create new visits. it should be post

56ec7e8

Add modification to Date amd Status of visit

1249e04

[API] New test that creates conflict

6df2e72

remove unnecessary modification

935ac8d

remove unnecessary modification

f11da3a

remove unnecessary modification; PUT request was good before

53018e8

add put that add new visit

08c2a40

add put that add new visit

c13b579

change battery name for test assert 201

8e9a0bb

Use test candidate

97f4ac1

remove space

7b214b5

New tests

700dcd1

New tests sites

0b024a2

Fix error expected and add tests

b246d9f

2020-10-07-FixPutVisitEndpoint

eef4920

spell00 added 7 commits November 4, 2020 13:36

wrong version in test request

2802c7a

Removed mock cnadidate

96252ec

Rebase

76790ee

Removing experimental stuff

91f12ff

restore an empty line

cb58bfc

forbid changing a visit of a candidate from a site user has no access to

f14e0e1

Adding comments to new tests for PUT in Candidates visits endpoint

ed0baf7

spell00 force-pushed the 2020-10-07-FixPutVisitEndpoint branch from 7ba7074 to ed0baf7 Compare November 4, 2020 18:36

add test candidate and visits

e79d6e5

spell00 removed the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Nov 5, 2020

spell00 added 2 commits November 5, 2020 02:00

Test repair

160148a

Remove one test

a744cf0

driusan reviewed Nov 5, 2020

View reviewed changes

driusan mentioned this pull request Nov 5, 2020

[API] PUT and PATCH methods added to Candidates visit/instruments/flags #6780

Merged

spell00 added 2 commits November 5, 2020 10:37

applying changes in candidate permission check

7cf441d

if not have access throw error

ed0cc96

spell00 requested a review from driusan November 6, 2020 07:12

driusan reviewed Nov 6, 2020

View reviewed changes

replace backticks

063ad31

spell00 requested a review from driusan November 6, 2020 20:25

driusan approved these changes Nov 6, 2020

View reviewed changes

spell00 requested a review from driusan November 6, 2020 22:22

driusan merged commit df29ed8 into aces:main Nov 9, 2020

ridz1208 added this to the 24.0.0 milestone Nov 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[API] Fix PUT request for Candidates \visit endpoint and integration test #7088

[API] Fix PUT request for Candidates \visit endpoint and integration test #7088

spell00 commented Oct 8, 2020 •

edited

Loading

christinerogers commented Nov 2, 2020

spell00 commented Nov 2, 2020

spell00 commented Nov 5, 2020

driusan Nov 5, 2020 •

edited

Loading

spell00 Nov 6, 2020

spell00 commented Nov 6, 2020

driusan Nov 6, 2020

spell00 Nov 6, 2020

driusan Nov 6, 2020

spell00 commented Nov 6, 2020 •

edited

Loading

driusan left a comment

spell00 commented Nov 6, 2020

	'You can`t create or modify that candidate'
	"You can't create or modify that candidate"

	'You can`t create or modify candidates visit for the site ' .
	"You can't create or modify candidates visit for the site " .

[API] Fix PUT request for Candidates \visit endpoint and integration test #7088

[API] Fix PUT request for Candidates \visit endpoint and integration test #7088

Conversation

spell00 commented Oct 8, 2020 • edited Loading

Brief summary of changes

Testing instructions

Link(s) to related issue(s)

christinerogers commented Nov 2, 2020

spell00 commented Nov 2, 2020

spell00 commented Nov 5, 2020

driusan Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

spell00 Nov 6, 2020

Choose a reason for hiding this comment

spell00 commented Nov 6, 2020

driusan Nov 6, 2020

Choose a reason for hiding this comment

spell00 Nov 6, 2020

Choose a reason for hiding this comment

driusan Nov 6, 2020

Choose a reason for hiding this comment

spell00 commented Nov 6, 2020 • edited Loading

driusan left a comment

Choose a reason for hiding this comment

spell00 commented Nov 6, 2020

spell00 commented Oct 8, 2020 •

edited

Loading

driusan Nov 5, 2020 •

edited

Loading

spell00 commented Nov 6, 2020 •

edited

Loading