Mercurial > hg > orthanc-authorization

changeset 313:be8457e5aabc

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
fix /tool/bulk-delete: a user was able to delete resources he does not have access to
author Alain Mazy <am@orthanc.team>
date Fri, 20 Mar 2026 17:06:41 +0100
parents 773185dc4db4
children 331809444cec
files NEWS Plugin/AuthorizationParserBase.cpp Plugin/AuthorizationParserBase.h Plugin/Plugin.cpp
diffstat 4 files changed, 109 insertions(+), 9 deletions(-) [+]
line wrap: on
line diff
--- a/NEWS	Tue Feb 10 14:47:00 2026 +0100
+++ b/NEWS	Fri Mar 20 17:06:41 2026 +0100
@@ -3,6 +3,8 @@
 
 * Now recording audit-logs when uploading a zip.
 * New default permissions for sending emails when sharing studies.
+* Fix: in /tools/bulk-delete, a user was able to delete resources
+  he does not have access to.
 
 
 2025-11-20 - v 0.10.3
--- a/Plugin/AuthorizationParserBase.cpp	Tue Feb 10 14:47:00 2026 +0100
+++ b/Plugin/AuthorizationParserBase.cpp	Fri Mar 20 17:06:41 2026 +0100
@@ -69,6 +69,29 @@
     throw Orthanc::OrthancException(Orthanc::ErrorCode_UnknownResource);
   }
 
+  void AuthorizationParserBase::AddOrthancResource(AccessedResources& target,
+                                                   Orthanc::ResourceType type,
+                                                   const std::string& orthancId)
+  {
+    switch (type)
+    {
+      case Orthanc::ResourceType_Instance:
+        AddOrthancInstance(target, orthancId);
+        break;
+      case Orthanc::ResourceType_Series:
+        AddOrthancSeries(target, orthancId);
+        break;
+      case Orthanc::ResourceType_Study:
+        AddOrthancStudy(target, orthancId);
+        break;
+      case Orthanc::ResourceType_Patient:
+        AddOrthancPatient(target, orthancId);
+        break;
+      default:
+        throw Orthanc::OrthancException(Orthanc::ErrorCode_InternalError);
+    }
+  }
+
 
   void AuthorizationParserBase::AddOrthancInstance(AccessedResources& target,
                                                    const std::string& orthancId)
--- a/Plugin/AuthorizationParserBase.h	Tue Feb 10 14:47:00 2026 +0100
+++ b/Plugin/AuthorizationParserBase.h	Fri Mar 20 17:06:41 2026 +0100
@@ -55,9 +55,6 @@
     void AddOrthancPatient(AccessedResources& target,
                            const std::string& orthancId);
 
-    Orthanc::ResourceType AddOrthancUnknownResource(AccessedResources& target,
-                                                    const std::string& orthancId);
-
     void AddDicomPatient(AccessedResources& target,
                          const std::string& patientId);
 
@@ -71,6 +68,13 @@
                           const std::string& instanceDicomUid);
 
   public:
+    Orthanc::ResourceType AddOrthancUnknownResource(AccessedResources& target,
+                                                    const std::string& orthancId);
+
+    void AddOrthancResource(AccessedResources& target,
+                            Orthanc::ResourceType type,
+                            const std::string& orthancId);
+
     virtual void AddDicomStudy(AccessedResources& target,
                                const std::string& studyDicomUid) ORTHANC_OVERRIDE;
 
--- a/Plugin/Plugin.cpp	Tue Feb 10 14:47:00 2026 +0100
+++ b/Plugin/Plugin.cpp	Fri Mar 20 17:06:41 2026 +0100
@@ -40,7 +40,7 @@
 static bool resourceTokensEnabled_ = false;
 static bool userTokensEnabled_ = false;
 static bool enableAuditLogs_ = false;
-static std::unique_ptr<OrthancPlugins::IAuthorizationParser> authorizationParser_;
+static std::unique_ptr<OrthancPlugins::AuthorizationParserBase> authorizationParser_;
 static std::unique_ptr<OrthancPlugins::IAuthorizationService> authorizationService_;
 static std::unique_ptr<OrthancPlugins::PermissionParser> permissionParser_;
 static std::set<std::string> uncheckedResources_;
@@ -1021,6 +1021,67 @@
 }
 
 
+void CheckHasAccessToAllResourcesInPayload(OrthancPluginRestOutput* output, const OrthancPluginHttpRequest* request)
+{
+  // make sur the user has access to all resources listed in "Resources" (with potentialy a "Level" field to help identify the resources)
+
+  OrthancPlugins::IAuthorizationService::UserProfile profile;
+  if (!GetUserProfileInternal(profile, request))
+  {
+    throw Orthanc::OrthancException(Orthanc::ErrorCode_ForbiddenAccess, "Auth plugin: unable to check if the user has access to all Resources in the payload - no user profile found");
+  }
+
+  if (HasAccessToAllLabels(profile))  // these guys can do whatever they want
+  {
+    return;
+  }
+
+  Json::Value payload;
+  if (!OrthancPlugins::ReadJson(payload, request->body, request->bodySize) || !payload.isMember("Resources") || !payload["Resources"].isArray())
+  {
+    throw Orthanc::OrthancException(Orthanc::ErrorCode_BadFileFormat, "A JSON payload with a 'Resources' field was expected");
+  }
+
+  Orthanc::ResourceType levelInPayload = Orthanc::ResourceType_Instance; // random value
+  bool hasLevelInPayload = payload.isMember("Level") && payload["Level"].isString();
+  
+  if (hasLevelInPayload)
+  {
+    levelInPayload = Orthanc::StringToResourceType(payload["Level"].asString().c_str());
+  }
+  
+  std::map<std::string, Orthanc::ResourceType> resources;
+
+  for (Json::ArrayIndex i = 0; i < payload["Resources"].size(); ++i)
+  {
+    std::string resourceId = payload["Resources"][i].asString();
+
+    OrthancPlugins::IAuthorizationParser::AccessedResources accessedResources;
+
+    if (hasLevelInPayload)
+    {
+      authorizationParser_->AddOrthancResource(accessedResources, levelInPayload, resourceId);
+    }
+    else
+    {
+      authorizationParser_->AddOrthancUnknownResource(accessedResources, resourceId);
+    }
+
+    bool granted = false;
+
+    if (!HasAuthorizedLabelsForResource(granted, accessedResources, profile))
+    {
+      throw Orthanc::OrthancException(Orthanc::ErrorCode_ForbiddenAccess, "Auth plugin: Unable to check resource access based on the authorized_labels.");
+    }
+
+    if (!granted)
+    {
+      throw Orthanc::OrthancException(Orthanc::ErrorCode_ForbiddenAccess, "Auth plugin: the user does not have access to resource " + resourceId);
+    }
+  }
+}
+
+
 void ToolsFindOrCountResources(OrthancPluginRestOutput* output,
                                const char* /*url*/,
                                const OrthancPluginHttpRequest* request,
@@ -1653,6 +1714,7 @@
 }
 
 
+template <bool enableAudiLogs>
 void BulkDeleteWithAuditLogs(OrthancPluginRestOutput* output,
                              const char* url,
                              const OrthancPluginHttpRequest* request)
@@ -1670,13 +1732,18 @@
     throw Orthanc::OrthancException(Orthanc::ErrorCode_BadFileFormat, "A JSON payload was expected");
   }
 
+  CheckHasAccessToAllResourcesInPayload(output, request);
+
   std::list<AuditLog> auditLogs;
 
-  for (Json::ArrayIndex i = 0; i < payload["Resources"].size(); ++i)
+  if (enableAudiLogs)
   {
-    std::string resourceId = payload["Resources"][i].asString();
-    OrthancPluginResourceType resourceType = IdentifyResourceType(resourceId);
-    GetResourceDeletionAuditLogs(auditLogs, resourceType, resourceId, request);
+    for (Json::ArrayIndex i = 0; i < payload["Resources"].size(); ++i)
+    {
+      std::string resourceId = payload["Resources"][i].asString();
+      OrthancPluginResourceType resourceType = IdentifyResourceType(resourceId);
+      GetResourceDeletionAuditLogs(auditLogs, resourceType, resourceId, request);
+    }
   }
 
   OrthancPlugins::RestApiClient coreApi(url, request);
@@ -2496,7 +2563,7 @@
             OrthancPlugins::RegisterRestCallback<AnonymizeWithAuditLogs>("/(patients|studies|series)/([^/]*)/anonymize", true);
             OrthancPlugins::RegisterRestCallback<ModifyWithAuditLogs>("/(patients|studies|series)/([^/]*)/modify", true);
             OrthancPlugins::RegisterRestCallback<LabelWithAuditLogs>("/(patients|studies|series)/([^/]*)/labels/([^/]*)", true);
-            OrthancPlugins::RegisterRestCallback<BulkDeleteWithAuditLogs>("/tools/bulk-delete", true);
+            OrthancPlugins::RegisterRestCallback<BulkDeleteWithAuditLogs<true> >("/tools/bulk-delete", true);
             OrthancPlugins::RegisterRestCallback<BulkModifyAnonymizeWithAuditLogs>("/tools/bulk-modify", true);
             OrthancPlugins::RegisterRestCallback<BulkModifyAnonymizeWithAuditLogs>("/tools/bulk-anonymize", true);
             OrthancPlugins::RegisterRestCallback<GetAuditLogs>("/auth/audit-logs", true);
@@ -2510,6 +2577,10 @@
             // /media + create-media + create-media-extended
 
           }
+          else
+          {
+            OrthancPlugins::RegisterRestCallback<BulkDeleteWithAuditLogs<false> >("/tools/bulk-delete", true);
+          }
         }
 
         if (!urlTokenCreationBase.empty())