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.

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

[dicom archive] add project permission check based on tarchiveID #9359

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Changes from 3 commits
Commits
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
42 changes: 41 additions & 1 deletion modules/dicom_archive/php/viewdetails.class.inc
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,47 @@ class ViewDetails extends \NDB_Form
*/
function _hasAccess(\User $user) : bool
{
return $user->hasPermission('dicom_archive_view_allsites');
// remove the possibility to have no tarchive ID in this page
if (empty($_REQUEST['tarchiveID'])) {
// defaults to permission denied
return false;
}

// get project ID from Tarchive ID.
$tarchiveID = intval($_REQUEST['tarchiveID']);
regisoc marked this conversation as resolved.
Show resolved Hide resolved
$projectID = self::getProjectFromTarchiveID($tarchiveID);
if (is_null($projectID)) {
return false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be return true? Otherwise no one will ever be able to see it?

(Maybe a discussion for an imaging meeting?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That means the TarchiveID does not exist in db or is not linked to a project.. ?
I was not sure about this. It is even possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we can involve @cmadjar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Point added to next imaging meeting.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm assuming it would mean the TarchiveID is not linked to a project

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Imaging meeting: no one should see it by default.
There should be a specific permission to see the list of "dangling TarchiveIDs" (Tarchive not assigned to any Project). Also might be good to have a front-end page for that.
It will be linked to a new issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created here: #9389

}

// check permissions
return $user->hasPermission('dicom_archive_view_allsites')
&& $user->hasProject($projectID);
}

/**
* Get the ProjectID attached to a given tarchive ID.
*
* @param int $tarchiveID a tarchiveID
*
* @return ProjectID|null a ProjectID if found, else null
*/
private static function _getProjectFromTarchiveID(int $tarchiveID): ?\ProjectID
{
$db = \NDB_Factory::singleton()->database();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
$db = \NDB_Factory::singleton()->database();
$db = $this->loris->getDatabaseConnection();

$pid = $db->pselectOne(
"SELECT p.ProjectID
FROM tarchive t
JOIN session s ON (t.SessionID = s.ID)
JOIN Project p ON (p.ProjectID = s.ProjectID)
WHERE t.TarchiveID = :tar",
['tar' => $tarchiveID]
);
//
if (is_null($pid)) {
return null;
}
return new \ProjectID($pid);
regisoc marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down
Loading