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

Conversation

regisoc
Copy link
Contributor

@regisoc regisoc commented Sep 24, 2024

Brief summary of changes

This PR checks the user attached projects on top of the dicom_archive_view_allsites permission when trying to access the view details page.

Link(s) to related issue(s)

Resolves #6658

$tarchiveID = intval($_REQUEST['tarchiveID']);
$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

*/
private function _getProjectFromTarchiveID(): ?\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();

@cmadjar cmadjar self-assigned this Oct 1, 2024
@cmadjar cmadjar added the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DicomArchive] Add project permissions to Subpage
3 participants