From df7d011b01bae883caa887dcf564c891aa0a7b84 Mon Sep 17 00:00:00 2001 From: Dave MacFarlane Date: Tue, 20 Dec 2022 09:36:03 -0500 Subject: [PATCH 1/3] [Instrument] Remove exception from NDB_BVL_Instrument::factory This removes the exception from NDB_BVL_Instrument::factory(), so that it's possible to call hasAccess on an instrument. Currently the hasAccess or Factory call mix concerns, making it difficult for a module to check if a user has access to an instrument. --- htdocs/survey.php | 8 +++++++- .../candidate/visit/instruments.class.inc | 5 +++++ .../php/endpoints/project/instruments.class.inc | 5 +++++ .../php/endpoints/unresolved.class.inc | 5 +++++ modules/datadict/php/datadict.class.inc | 8 ++++++++ modules/instruments/php/module.class.inc | 8 +++++++- modules/instruments/php/visitsummary.class.inc | 9 +++++++-- php/libraries/NDB_BVL_Instrument.class.inc | 15 +-------------- 8 files changed, 45 insertions(+), 18 deletions(-) diff --git a/htdocs/survey.php b/htdocs/survey.php index 029d97ec1e7..8ef2ad57918 100644 --- a/htdocs/survey.php +++ b/htdocs/survey.php @@ -103,10 +103,16 @@ function initialize() throw new Exception("Data has already been submitted.", 403); } - $instrumentObj = \NDB_BVL_Instrument::factory( + $instrumentObj = \NDB_BVL_Instrument::factory( $this->loris, $this->TestName, ); + + $user = \User::singleton(); + if ($instrumentObj->_hasAccess($user) !== true) { + throw new \Exception("Permission denied", 403); + } + $subtests = $instrumentObj->getSubtestList(); $this->NumPages = count($subtests) + 1; diff --git a/modules/api/php/endpoints/candidate/visit/instruments.class.inc b/modules/api/php/endpoints/candidate/visit/instruments.class.inc index c16e8043fb5..ca537b379cd 100644 --- a/modules/api/php/endpoints/candidate/visit/instruments.class.inc +++ b/modules/api/php/endpoints/candidate/visit/instruments.class.inc @@ -131,6 +131,11 @@ class Instruments extends Endpoint implements \LORIS\Middleware\ETagCalculator '', true ); + + $user = $request->getAttribute('user'); + if ($instrument->_hasAccess($user) == false) { + return new \LORIS\Http\Response\JSON\Forbidden(); + }; } catch (\Exception $e) { return new \LORIS\Http\Response\JSON\NotFound(); } diff --git a/modules/api/php/endpoints/project/instruments.class.inc b/modules/api/php/endpoints/project/instruments.class.inc index dae6f154407..eaeeab5d72e 100644 --- a/modules/api/php/endpoints/project/instruments.class.inc +++ b/modules/api/php/endpoints/project/instruments.class.inc @@ -108,6 +108,11 @@ class Instruments extends Endpoint implements \LORIS\Middleware\ETagCalculator '', true ); + + $user = $request->getAttribute("user"); + if ($instrument->_hasAccess($user) == false) { + return new \LORIS\Http\Response\JSON\Forbidden(); + } } catch (\Exception $e) { return new \LORIS\Http\Response\JSON\NotFound(); } diff --git a/modules/conflict_resolver/php/endpoints/unresolved.class.inc b/modules/conflict_resolver/php/endpoints/unresolved.class.inc index edabd250d32..17713e4db58 100644 --- a/modules/conflict_resolver/php/endpoints/unresolved.class.inc +++ b/modules/conflict_resolver/php/endpoints/unresolved.class.inc @@ -252,6 +252,11 @@ class Unresolved extends Endpoint implements ETagCalculator '', true ); + if ($instrument->_hasAccess($user) == false) { + return new \LORIS\Http\Response\JSON\NotFound( + 'Permission denied for ' . $conflict['TableName'] + ); + } $instrument->_saveValues( [$conflict['FieldName'] => $conflict['CorrectAnswer']] ); diff --git a/modules/datadict/php/datadict.class.inc b/modules/datadict/php/datadict.class.inc index 23248982031..9c7161ccc7c 100644 --- a/modules/datadict/php/datadict.class.inc +++ b/modules/datadict/php/datadict.class.inc @@ -60,6 +60,7 @@ class Datadict extends \DataFrameworkMenu { $instruments = \Utility::getAllInstruments(); $dictInstruments = []; + foreach ($instruments as $instrument => $name) { // Since the testname does not always match the table name in the // database we need to instantiate the object to get the table name @@ -70,6 +71,13 @@ class Datadict extends \DataFrameworkMenu '', '' ); + if ($iObj->_hasAccess($user)) { + $this->logger->debug( + "Skipping $instrument in field options" + . " because user does not have permission" + ); + continue; + } } catch (\Exception $e) { error_log( "There was a problem instantiating the instrument ". diff --git a/modules/instruments/php/module.class.inc b/modules/instruments/php/module.class.inc index 3894c463b80..b09a42f2e38 100644 --- a/modules/instruments/php/module.class.inc +++ b/modules/instruments/php/module.class.inc @@ -84,6 +84,13 @@ class Module extends \Module $page ); + $user = $request->getAttribute("user"); + if ($instrument->_hasAccess($user) == false) { + return (new \Laminas\Diactoros\Response()) + ->withStatus(403) + ->withBody(new \LORIS\Http\StringStream("Permission denied")); + } + $request = $request->withAttribute('pageclass', $instrument); return $instrument->process($request, $instrument); @@ -151,4 +158,3 @@ class Module extends \Module return []; } } - diff --git a/modules/instruments/php/visitsummary.class.inc b/modules/instruments/php/visitsummary.class.inc index 986acfe89a6..20970d426db 100644 --- a/modules/instruments/php/visitsummary.class.inc +++ b/modules/instruments/php/visitsummary.class.inc @@ -52,6 +52,7 @@ class VisitSummary extends \NDB_Page return $this->_handleGET( new CandID($params['CandID']), $params['VisitLabel'], + $user, ); default: return new \LORIS\Http\Response\JSON\MethodNotAllowed( @@ -61,15 +62,16 @@ class VisitSummary extends \NDB_Page } /** - * Helper to specifically handle PATCH HTTP methods to the endpoint. + * Helper to specifically handle GET HTTP methods to the endpoint. * * @param CandID $candid The CandID for the visit. * @param string $visitLabel The visit label string. + * @param \User $user The user accessing the endpoint * * @return ResponseInterface */ private function _handleGET( - CandID $candid, string $visitLabel + CandID $candid, string $visitLabel, \User $user ) : ResponseInterface { $DB = \NDB_Factory::singleton()->database(); @@ -111,6 +113,9 @@ class VisitSummary extends \NDB_Page '', false ); + if ($instrument->_hasAccess($user) == false) { + continue; + } if ($instrument === null) { $bvl_result[$key]['Completion'] = 0; } else { diff --git a/php/libraries/NDB_BVL_Instrument.class.inc b/php/libraries/NDB_BVL_Instrument.class.inc index 3035f735664..e1b12405277 100644 --- a/php/libraries/NDB_BVL_Instrument.class.inc +++ b/php/libraries/NDB_BVL_Instrument.class.inc @@ -308,17 +308,6 @@ abstract class NDB_BVL_Instrument extends NDB_Page $base . "project/instruments/$instrument.rules" ); - $user = \User::singleton(); - $access = $obj->_hasAccess($user); - - // check that user has access - if ($access == false) { - throw new Exception( - "You do not have access to this page.", - 403 - ); - } - return $obj; } @@ -380,9 +369,7 @@ abstract class NDB_BVL_Instrument extends NDB_Page return true; } } - - //no user permissions match required instrument permissions - throw new Exception("You do not have access to this page.", 403); + return false; } } From a4784beced09bd312a23a6a5affe7250f19ce297 Mon Sep 17 00:00:00 2001 From: Dave MacFarlane Date: Tue, 21 Feb 2023 14:12:17 -0500 Subject: [PATCH 2/3] Forbidden not NotFound --- modules/conflict_resolver/php/endpoints/unresolved.class.inc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/conflict_resolver/php/endpoints/unresolved.class.inc b/modules/conflict_resolver/php/endpoints/unresolved.class.inc index 17713e4db58..818cf147c98 100644 --- a/modules/conflict_resolver/php/endpoints/unresolved.class.inc +++ b/modules/conflict_resolver/php/endpoints/unresolved.class.inc @@ -253,7 +253,7 @@ class Unresolved extends Endpoint implements ETagCalculator true ); if ($instrument->_hasAccess($user) == false) { - return new \LORIS\Http\Response\JSON\NotFound( + return new \LORIS\Http\Response\JSON\Forbidden( 'Permission denied for ' . $conflict['TableName'] ); } From 96a0f0fcf45f3f248be543375cfb20560e3c7fc6 Mon Sep 17 00:00:00 2001 From: Dave MacFarlane Date: Tue, 21 Feb 2023 14:21:17 -0500 Subject: [PATCH 3/3] Add debug log --- modules/instruments/php/visitsummary.class.inc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/modules/instruments/php/visitsummary.class.inc b/modules/instruments/php/visitsummary.class.inc index 20970d426db..94fb47452ca 100644 --- a/modules/instruments/php/visitsummary.class.inc +++ b/modules/instruments/php/visitsummary.class.inc @@ -114,6 +114,10 @@ class VisitSummary extends \NDB_Page false ); if ($instrument->_hasAccess($user) == false) { + $this->logger->debug( + "Skipping $testName" + . " because user does not have permission" + ); continue; } if ($instrument === null) {