Merge pull request #73 from informatics-isi-edu/acl_mgmt_urldecode_fix

ACL mgmt url-decode fix
informatics-isi-edu · Sep 10, 2024 · e3d58db · e3d58db
2 parents cdada66 + b8e94c2
commit e3d58db
Show file tree

Hide file tree

Showing 3 changed files with 19 additions and 2 deletions.
diff --git a/hatrac/model/directory/pgsql.py b/hatrac/model/directory/pgsql.py
@@ -95,7 +95,7 @@ def get_content(self, client_context, get_data=True):
     def __getitem__(self, role):
         if role not in self:
             raise core.NotFound(
-                'ACL member %s;acl/%s/%s not found.' % (self.resource, self.access, role)
+                'ACL member %s;acl/%s/%s not found.' % (self.resource, self.access, urllib.parse.quote(role, safe=''))
             )
         entry = ACLEntry(role + '\n')
         entry.resource = self.resource

diff --git a/hatrac/rest/acl.py b/hatrac/rest/acl.py
@@ -6,6 +6,7 @@
 
 import json
 from flask import request, g as hatrac_ctx
+from urllib.parse import unquote
 
 from . import app
 from .core import RestHandler, \
@@ -20,6 +21,7 @@ def __init__(self):
     def put(self, access, role, path="/", name="", version=""):
         """Add entry to ACL."""
         self.enforce_firewall('manage_acl')
+        role = unquote(role)
         resource = self.resolve_name_or_version(
             path, name, version
         )
@@ -35,6 +37,7 @@ def put(self, access, role, path="/", name="", version=""):
     def delete(self, access, role, path="/", name="", version=""):
         """Remove entry from ACL."""
         self.enforce_firewall('manage_acl')
+        role = unquote(role)
         resource = self.resolve_name_or_version(
             path, name, version
         )
@@ -50,6 +53,7 @@ def delete(self, access, role, path="/", name="", version=""):
     def get(self, access, role, path="/", name="", version=""):
         """Get entry from ACL."""
         self.get_body = False if request.method == 'HEAD' else True
+        role = unquote(role)
         resource = self.resolve_name_or_version(path, name, version).acls[access]
         self.set_http_etag(hash_list(resource))
         resource = resource[role]

diff --git a/test/rest-smoketest.sh b/test/rest-smoketest.sh
@@ -49,11 +49,19 @@ RESPONSE_HEADERS=/tmp/${RUNKEY}-response-headers
 RESPONSE_CONTENT=/tmp/${RUNKEY}-response-content
 TEST_DATA=/tmp/${RUNKEY}-test-data
 TEST_ACL_ANON=/tmp/${RUNKEY}-test-acl-anon.json
+TEST_ACL_URI=/tmp/${RUNKEY}-test-acl-uri.json
 
 cat > ${TEST_ACL_ANON} <<EOF
 ["*"]
 EOF
 
+test_acl_uri_member='scheme://authority/path'
+test_acl_uri_member_quoted='scheme%3A%2F%2Fauthority%2Fpath'
+
+cat > ${TEST_ACL_URI} <<EOF
+["unlikely value", "${test_acl_uri_member}"]
+EOF
+
 cleanup()
 {
     rm -f ${RESPONSE_HEADERS} ${RESPONSE_CONTENT} ${TEST_DATA} ${TEST_ACL_ANON}
@@ -788,7 +796,12 @@ dotest "200::application/json::*" "/ns-${RUNKEY}/foo;acl/"
 dotest "200::application/json::*" "/ns-${RUNKEY}/foo;acl/owner"
 dotest "200::application/json::*" "/ns-${RUNKEY}/foo;acl/owner" --head
 dotest "200::application/json::*" "/ns-${RUNKEY}/foo;acl/create"
-dotest "404::*::*" "/ns-${RUNKEY}/foo/bar;acl/create/DUMMY"
+dotest "204::*::*" "/ns-${RUNKEY}/foo/bar;acl/create" -T ${TEST_ACL_URI} -H "Content-Type: application/json"
+dotest "200::*::*" "/ns-${RUNKEY}/foo/bar;acl/create/${test_acl_uri_member_quoted}"
+dotest "204::*::*" "/ns-${RUNKEY}/foo/bar;acl/create/${test_acl_uri_member_quoted}" -X DELETE
+dotest "404::*::*" "/ns-${RUNKEY}/foo/bar;acl/create/${test_acl_uri_member_quoted}"
+dotest "204::*::*" "/ns-${RUNKEY}/foo/bar;acl/create/${test_acl_uri_member_quoted}" -X PUT
+dotest "200::*::*" "/ns-${RUNKEY}/foo/bar;acl/create/${test_acl_uri_member_quoted}"
 dotest "204::*::*" "/ns-${RUNKEY}/foo/bar;acl/create/DUMMY" -X PUT
 dotest "200::text/plain*::*" "/ns-${RUNKEY}/foo/bar;acl/create/DUMMY" --head
 dotest "204::*::*" "/ns-${RUNKEY}/foo/bar;acl/create/DUMMY" -X DELETE -H "If-Match: *"