From e2c79bb87dc39b9090b89d3693c90d4c76fcc726 Mon Sep 17 00:00:00 2001
From: Marko Grujic <markoog@gmail.com>
Date: Tue, 12 Oct 2021 12:12:38 +0000
Subject: [PATCH 1/5] Implement client side splitgraph_api no-op enforcement

---
 .../resources/static/splitgraph_api.sql       | 95 ++++++++++++-------
 1 file changed, 60 insertions(+), 35 deletions(-)

diff --git a/splitgraph/resources/static/splitgraph_api.sql b/splitgraph/resources/static/splitgraph_api.sql
index 4a890612..972f9d62 100644
--- a/splitgraph/resources/static/splitgraph_api.sql
+++ b/splitgraph/resources/static/splitgraph_api.sql
@@ -51,19 +51,35 @@ $$
 LANGUAGE plpgsql
 SECURITY DEFINER;
 
+CREATE OR REPLACE FUNCTION splitgraph_api.check_objects_privilege (
+    _object_ids varchar[],
+    _action varchar DEFAULT 'repository.read'
+)
+    RETURNS void
+    AS $$
+BEGIN
+    -- A no-op privilege check that can be overridden
+    RETURN;
+END;
+$$
+LANGUAGE plpgsql
+SECURITY DEFINER;
+
 CREATE OR REPLACE FUNCTION splitgraph_api.check_privilege (
-    namespace varchar
+    _namespace varchar,
+    _repository varchar DEFAULT NULL,
+    _action varchar DEFAULT 'repository.read'
 )
     RETURNS void
     AS $$
 BEGIN
-    IF splitgraph_api.bypass_privilege () THEN
+    IF splitgraph_api.bypass_privilege () OR _action = 'repository.read' THEN
         RETURN;
     END IF;
     -- Here "current_user" is the definer, "session_user" is the caller and we can access another session variable
     -- to establish identity.
     -- Use IS DISTINCT FROM rather than != to catch namespace=NULL
-    IF splitgraph_api.get_current_username () IS DISTINCT FROM namespace THEN
+    IF splitgraph_api.get_current_username () IS DISTINCT FROM _namespace THEN
         RAISE insufficient_privilege
         USING MESSAGE = 'You do not have access to this namespace!';
     END IF;
@@ -89,6 +105,7 @@ CREATE OR REPLACE FUNCTION splitgraph_api.get_images (
         )
         AS $$
 BEGIN
+    PERFORM splitgraph_api.check_privilege (_namespace, _repository);
     RETURN QUERY
     SELECT i.image_hash,
         i.parent_id,
@@ -102,7 +119,7 @@ BEGIN
 END
 $$
 LANGUAGE plpgsql
-SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp;
+SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp, public;
 
 
 -- get_repository_size(namespace, repository): get repository size in bytes (counting tables
@@ -114,6 +131,7 @@ CREATE OR REPLACE FUNCTION splitgraph_api.get_repository_size (
     RETURNS BIGINT
     AS $$
 BEGIN
+    PERFORM splitgraph_api.check_privilege (_namespace, _repository);
     RETURN (WITH iob AS (
             SELECT DISTINCT unnest(object_ids) AS object_id
             FROM splitgraph_meta.tables t
@@ -126,7 +144,7 @@ BEGIN
 END
 $$
 LANGUAGE plpgsql
-SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp;
+SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp, public;
 
 -- Search for an image by its hash prefix. Currently the prefix feature
 -- is only used for better API/cmdline UX -- pushes/pulls to the registry
@@ -145,6 +163,7 @@ CREATE OR REPLACE FUNCTION splitgraph_api.get_image (
         )
         AS $$
 BEGIN
+    PERFORM splitgraph_api.check_privilege (_namespace, _repository);
     RETURN QUERY
     SELECT i.image_hash,
         i.parent_id,
@@ -159,7 +178,7 @@ BEGIN
 END
 $$
 LANGUAGE plpgsql
-SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp;
+SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp, public;
 
 -- get_tagged_images(namespace, repository): get hashes of all images with a tag.
 CREATE OR REPLACE FUNCTION splitgraph_api.get_tagged_images (
@@ -172,6 +191,7 @@ CREATE OR REPLACE FUNCTION splitgraph_api.get_tagged_images (
         )
         AS $$
 BEGIN
+    PERFORM splitgraph_api.check_privilege (_namespace, _repository);
     RETURN QUERY
     SELECT t.image_hash,
         t.tag
@@ -181,7 +201,7 @@ BEGIN
 END
 $$
 LANGUAGE plpgsql
-SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp;
+SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp, public;
 
 -- get_image_size(namespace, repository, image_hash): get image size in bytes (counting tables
 -- that share objects only once)
@@ -193,6 +213,7 @@ CREATE OR REPLACE FUNCTION splitgraph_api.get_image_size (
     RETURNS BIGINT
     AS $$
 BEGIN
+    PERFORM splitgraph_api.check_privilege (_namespace, _repository);
     RETURN (WITH iob AS (
             SELECT DISTINCT image_hash,
                 unnest(object_ids) AS object_id
@@ -207,7 +228,7 @@ BEGIN
 END
 $$
 LANGUAGE plpgsql
-SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp;
+SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp, public;
 
 -- get_image_dependencies(namespace, repository, image_hash): get all images
 -- that this Splitfile-built image depends on.
@@ -223,6 +244,7 @@ CREATE OR REPLACE FUNCTION splitgraph_api.get_image_dependencies (
         )
         AS $$
 BEGIN
+    PERFORM splitgraph_api.check_privilege (_namespace, _repository);
     RETURN QUERY WITH provenance_data AS (
         SELECT jsonb_array_elements(i.provenance_data) AS d
         FROM splitgraph_meta.images i
@@ -248,7 +270,7 @@ WHERE d ->> 'source_namespace' IS NOT NULL
 END
 $$
 LANGUAGE plpgsql
-SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp;
+SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp, public;
 
 -- get_image_dependents(namespace, repository, image_hash): get all images in this
 -- repository that were built by a Splitfile and used this image through a FROM command.
@@ -266,6 +288,7 @@ CREATE OR REPLACE FUNCTION splitgraph_api.get_image_dependents (
 DECLARE
     FILTER JSONB;
 BEGIN
+    PERFORM splitgraph_api.check_privilege (_namespace, _repository);
     FILTER = jsonb_build_array (jsonb_build_object('source_namespace',
 	_namespace, 'source', _repository, 'source_hash', _image_hash));
     RETURN QUERY
@@ -278,7 +301,7 @@ BEGIN
 END
 $$
 LANGUAGE plpgsql
-SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp;
+SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp, public;
 
 -- Consider merging writes to all tables into one big routine (e.g. also include a list of tables here, which
 -- will get added to the tables table)
@@ -295,7 +318,7 @@ CREATE OR REPLACE FUNCTION splitgraph_api.add_image (
     RETURNS void
     AS $$
 BEGIN
-    PERFORM splitgraph_api.check_privilege (namespace);
+    PERFORM splitgraph_api.check_privilege (namespace, repository, 'repository.push_image');
     INSERT INTO splitgraph_meta.images (namespace, repository, image_hash,
 	parent_id, created, comment, provenance_data)
 	VALUES (namespace, repository, image_hash, parent_id, created, comment,
@@ -318,7 +341,7 @@ CREATE OR REPLACE FUNCTION splitgraph_api.delete_image (
     RETURNS void
     AS $$
 BEGIN
-    PERFORM splitgraph_api.check_privilege (_namespace);
+    PERFORM splitgraph_api.check_privilege (_namespace, _repository, 'repository.delete');
     DELETE FROM splitgraph_meta.tags
     WHERE namespace = _namespace
         AND repository = _repository
@@ -346,7 +369,7 @@ CREATE OR REPLACE FUNCTION splitgraph_api.tag_image (
     RETURNS void
     AS $$
 BEGIN
-    PERFORM splitgraph_api.check_privilege (_namespace);
+    PERFORM splitgraph_api.check_privilege (_namespace, _repository, 'repository.update_metadata');
     INSERT INTO splitgraph_meta.tags (namespace, repository, image_hash, tag)
         VALUES (_namespace, _repository, _image_hash, _tag)
     ON CONFLICT (namespace, repository, tag)
@@ -355,7 +378,7 @@ BEGIN
 END
 $$
 LANGUAGE plpgsql
-SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp;
+SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp, public;
 
 CREATE OR REPLACE FUNCTION splitgraph_api.delete_tag (
     _namespace varchar,
@@ -365,7 +388,7 @@ CREATE OR REPLACE FUNCTION splitgraph_api.delete_tag (
     RETURNS void
     AS $$
 BEGIN
-    PERFORM splitgraph_api.check_privilege (_namespace);
+    PERFORM splitgraph_api.check_privilege (_namespace, _repository, 'repository.update_metadata');
     DELETE FROM splitgraph_meta.tags
         WHERE namespace = _namespace
         AND repository = _repository
@@ -373,7 +396,7 @@ BEGIN
 END
 $$
 LANGUAGE plpgsql
-SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp;
+SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp, public;
 
 -- delete_repository(namespace, repository)
 CREATE OR REPLACE FUNCTION splitgraph_api.delete_repository (
@@ -383,7 +406,7 @@ CREATE OR REPLACE FUNCTION splitgraph_api.delete_repository (
     RETURNS void
     AS $$
 BEGIN
-    PERFORM splitgraph_api.check_privilege (_namespace);
+    PERFORM splitgraph_api.check_privilege (_namespace, _repository, 'repository.delete');
     DELETE FROM splitgraph_meta.tables
     WHERE namespace = _namespace
         AND repository = _repository;
@@ -408,6 +431,7 @@ CREATE OR REPLACE FUNCTION splitgraph_api.get_new_objects (
     RETURNS varchar[]
     AS $$
 BEGIN
+    PERFORM splitgraph_api.check_objects_privilege (object_ids);
     RETURN ARRAY (
         SELECT o
         FROM unnest(object_ids) o
@@ -417,7 +441,7 @@ BEGIN
 END
 $$
 LANGUAGE plpgsql
-SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp;
+SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp, public;
 
 -- get_object_meta(object_ids): get metadata for objects
 CREATE OR REPLACE FUNCTION splitgraph_api.get_object_meta (
@@ -437,6 +461,7 @@ CREATE OR REPLACE FUNCTION splitgraph_api.get_object_meta (
         )
         AS $$
 BEGIN
+    PERFORM splitgraph_api.check_objects_privilege (object_ids);
     RETURN QUERY
     SELECT o.object_id,
         o.format,
@@ -453,7 +478,7 @@ BEGIN
 END
 $$
 LANGUAGE plpgsql
-SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp;
+SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp, public;
 
 -- get_object_locations(object_ids): get external locations for objects
 CREATE OR REPLACE FUNCTION splitgraph_api.get_object_locations (
@@ -466,6 +491,7 @@ CREATE OR REPLACE FUNCTION splitgraph_api.get_object_locations (
         )
         AS $$
 BEGIN
+    PERFORM splitgraph_api.check_objects_privilege (object_ids);
     RETURN QUERY
     SELECT o.object_id,
         o.location,
@@ -475,7 +501,7 @@ BEGIN
 END
 $$
 LANGUAGE plpgsql
-SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp;
+SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp, public;
 
 -- add_object(object_id, format, namespace, size, insertion_hash, deletion_hash, index)
 -- If the object already exists, it gets overwritten (making sure the caller has permissions
@@ -513,6 +539,7 @@ BEGIN
 		    _rows_deleted);
     ELSE
         PERFORM splitgraph_api.check_privilege (existing.namespace);
+        PERFORM splitgraph_api.check_objects_privilege (ARRAY[_object_id], 'repository.push_image');
         UPDATE
             splitgraph_meta.objects
         SET format = _format,
@@ -527,7 +554,7 @@ BEGIN
 END
 $$
 LANGUAGE plpgsql
-SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp;
+SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp, public;
 
 -- add_object_location(object_id, location, protocol)
 CREATE OR REPLACE FUNCTION splitgraph_api.add_object_location (
@@ -537,14 +564,8 @@ CREATE OR REPLACE FUNCTION splitgraph_api.add_object_location (
 )
     RETURNS void
     AS $$
-DECLARE
-    namespace varchar;
 BEGIN
-    namespace = (
-        SELECT o.namespace
-        FROM splitgraph_meta.objects o
-        WHERE o.object_id = _object_id);
-    PERFORM splitgraph_api.check_privilege (namespace);
+    PERFORM splitgraph_api.check_objects_privilege (ARRAY[_object_id], 'repository.push_image');
     INSERT INTO splitgraph_meta.object_locations (object_id, LOCATION, protocol)
         VALUES (_object_id, LOCATION, protocol)
     ON CONFLICT (object_id)
@@ -552,7 +573,7 @@ BEGIN
 END
 $$
 LANGUAGE plpgsql
-SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp;
+SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp, public;
 
 --
 -- TABLE API
@@ -571,6 +592,7 @@ CREATE OR REPLACE FUNCTION splitgraph_api.get_tables (
         )
         AS $$
 BEGIN
+    PERFORM splitgraph_api.check_privilege (_namespace, _repository);
     RETURN QUERY
     SELECT t.table_name,
         t.table_schema,
@@ -582,7 +604,7 @@ BEGIN
 END
 $$
 LANGUAGE plpgsql
-SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp;
+SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp, public;
 
 -- get_tables(namespace, repository): list all tables in a given repository together with
 -- images they belong to.
@@ -598,6 +620,7 @@ CREATE OR REPLACE FUNCTION splitgraph_api.get_all_tables (
         )
         AS $$
 BEGIN
+    PERFORM splitgraph_api.check_privilege (_namespace, _repository);
     RETURN QUERY
     SELECT t.image_hash,
         t.table_name,
@@ -609,7 +632,7 @@ BEGIN
 END
 $$
 LANGUAGE plpgsql
-SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp;
+SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp, public;
 
 -- add_table(namespace, repository, table_name, table_schema, object_ids) -- add a table to an existing image.
 -- Technically, we shouldn't allow this to be done once the image has been created (so maybe that idea with only having
@@ -628,7 +651,7 @@ CREATE OR REPLACE FUNCTION splitgraph_api.add_table (
     RETURNS void
     AS $$
 BEGIN
-    PERFORM splitgraph_api.check_privilege (_namespace);
+    PERFORM splitgraph_api.check_privilege (_namespace, _repository, 'repository.push_image');
     INSERT INTO splitgraph_meta.tables (namespace, repository, image_hash,
 	table_name, table_schema, object_ids)
         VALUES (_namespace, _repository, _image_hash, _table_name, _table_schema, _object_ids)
@@ -639,7 +662,7 @@ BEGIN
 END
 $$
 LANGUAGE plpgsql
-SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp;
+SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp, public;
 
 -- get_table_size(namespace, repository, image_hash, table_name): get table size in bytes
 CREATE OR REPLACE FUNCTION splitgraph_api.get_table_size (
@@ -651,6 +674,7 @@ CREATE OR REPLACE FUNCTION splitgraph_api.get_table_size (
     RETURNS BIGINT
     AS $$
 BEGIN
+    PERFORM splitgraph_api.check_privilege (_namespace, _repository);
     RETURN (WITH iob AS (
             SELECT unnest(object_ids) AS object_id
             FROM splitgraph_meta.tables t
@@ -665,7 +689,7 @@ BEGIN
 END
 $$
 LANGUAGE plpgsql
-SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp;
+SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp, public;
 
 -- get_table_length: use object metadata to get the number of rows in a table. Will return
 -- NULL if some objects have an unknown number of rows.
@@ -678,6 +702,7 @@ CREATE OR REPLACE FUNCTION splitgraph_api.get_table_length (
     RETURNS INTEGER
     AS $$
 BEGIN
+    PERFORM splitgraph_api.check_privilege (_namespace, _repository);
     RETURN (WITH iob AS (
             SELECT unnest(object_ids) AS object_id
             FROM splitgraph_meta.tables t
@@ -692,7 +717,7 @@ BEGIN
 END
 $$
 LANGUAGE plpgsql
-SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp;
+SECURITY DEFINER SET search_path = splitgraph_meta, pg_temp, public;
 
 --
 -- S3 UPLOAD/DOWNLOAD API

From a5b3dca761367fcac97eb169e938ae5754f9a8a7 Mon Sep 17 00:00:00 2001
From: Marko Grujic <markoog@gmail.com>
Date: Tue, 12 Oct 2021 13:18:59 +0000
Subject: [PATCH 2/5] Remove access check when querying for non-existing object
 ids

---
 splitgraph/resources/static/splitgraph_api.sql | 1 -
 1 file changed, 1 deletion(-)

diff --git a/splitgraph/resources/static/splitgraph_api.sql b/splitgraph/resources/static/splitgraph_api.sql
index 972f9d62..19ca9903 100644
--- a/splitgraph/resources/static/splitgraph_api.sql
+++ b/splitgraph/resources/static/splitgraph_api.sql
@@ -431,7 +431,6 @@ CREATE OR REPLACE FUNCTION splitgraph_api.get_new_objects (
     RETURNS varchar[]
     AS $$
 BEGIN
-    PERFORM splitgraph_api.check_objects_privilege (object_ids);
     RETURN ARRAY (
         SELECT o
         FROM unnest(object_ids) o

From d48c22ecab677f634e4e6d0431dba22777aaf7e0 Mon Sep 17 00:00:00 2001
From: Marko Grujic <markoog@gmail.com>
Date: Wed, 13 Oct 2021 08:16:01 +0000
Subject: [PATCH 3/5] Use the filtered object ids in get functions

---
 .../resources/static/splitgraph_api.sql       | 23 +++++++++++--------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/splitgraph/resources/static/splitgraph_api.sql b/splitgraph/resources/static/splitgraph_api.sql
index 19ca9903..4e3469bd 100644
--- a/splitgraph/resources/static/splitgraph_api.sql
+++ b/splitgraph/resources/static/splitgraph_api.sql
@@ -53,13 +53,14 @@ SECURITY DEFINER;
 
 CREATE OR REPLACE FUNCTION splitgraph_api.check_objects_privilege (
     _object_ids varchar[],
-    _action varchar DEFAULT 'repository.read'
+    _action varchar DEFAULT 'repository.read',
+    _raise_error boolean DEFAULT false
 )
-    RETURNS void
+    RETURNS varchar[]
     AS $$
 BEGIN
     -- A no-op privilege check that can be overridden
-    RETURN;
+    RETURN _object_ids;
 END;
 $$
 LANGUAGE plpgsql
@@ -459,8 +460,10 @@ CREATE OR REPLACE FUNCTION splitgraph_api.get_object_meta (
             rows_deleted integer
         )
         AS $$
+DECLARE
+    _allowed_object_ids varchar[];
 BEGIN
-    PERFORM splitgraph_api.check_objects_privilege (object_ids);
+    SELECT splitgraph_api.check_objects_privilege (object_ids) INTO _allowed_object_ids;
     RETURN QUERY
     SELECT o.object_id,
         o.format,
@@ -473,7 +476,7 @@ BEGIN
         o.rows_inserted,
         o.rows_deleted
     FROM splitgraph_meta.objects o
-    WHERE o.object_id = ANY (object_ids);
+    WHERE o.object_id = ANY (_allowed_object_ids);
 END
 $$
 LANGUAGE plpgsql
@@ -489,14 +492,16 @@ CREATE OR REPLACE FUNCTION splitgraph_api.get_object_locations (
             protocol varchar
         )
         AS $$
+DECLARE
+    _allowed_object_ids varchar[];
 BEGIN
-    PERFORM splitgraph_api.check_objects_privilege (object_ids);
+    SELECT splitgraph_api.check_objects_privilege (object_ids) INTO _allowed_object_ids;
     RETURN QUERY
     SELECT o.object_id,
         o.location,
         o.protocol
     FROM splitgraph_meta.object_locations o
-    WHERE o.object_id = ANY (object_ids);
+    WHERE o.object_id = ANY (_allowed_object_ids);
 END
 $$
 LANGUAGE plpgsql
@@ -538,7 +543,7 @@ BEGIN
 		    _rows_deleted);
     ELSE
         PERFORM splitgraph_api.check_privilege (existing.namespace);
-        PERFORM splitgraph_api.check_objects_privilege (ARRAY[_object_id], 'repository.push_image');
+        PERFORM splitgraph_api.check_objects_privilege (ARRAY[_object_id], 'repository.push_image', true);
         UPDATE
             splitgraph_meta.objects
         SET format = _format,
@@ -564,7 +569,7 @@ CREATE OR REPLACE FUNCTION splitgraph_api.add_object_location (
     RETURNS void
     AS $$
 BEGIN
-    PERFORM splitgraph_api.check_objects_privilege (ARRAY[_object_id], 'repository.push_image');
+    PERFORM splitgraph_api.check_objects_privilege (ARRAY[_object_id], 'repository.push_image', true);
     INSERT INTO splitgraph_meta.object_locations (object_id, LOCATION, protocol)
         VALUES (_object_id, LOCATION, protocol)
     ON CONFLICT (object_id)

From 14588e561f5190491296c185771d2dacf43ae6fb Mon Sep 17 00:00:00 2001
From: Marko Grujic <markoog@gmail.com>
Date: Wed, 13 Oct 2021 12:21:55 +0000
Subject: [PATCH 4/5] Modify error message for more clarity

---
 splitgraph/resources/static/splitgraph_api.sql |  2 +-
 test/splitgraph/test_security.py               | 10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/splitgraph/resources/static/splitgraph_api.sql b/splitgraph/resources/static/splitgraph_api.sql
index 4e3469bd..a26b3ede 100644
--- a/splitgraph/resources/static/splitgraph_api.sql
+++ b/splitgraph/resources/static/splitgraph_api.sql
@@ -82,7 +82,7 @@ BEGIN
     -- Use IS DISTINCT FROM rather than != to catch namespace=NULL
     IF splitgraph_api.get_current_username () IS DISTINCT FROM _namespace THEN
         RAISE insufficient_privilege
-        USING MESSAGE = 'You do not have access to this namespace!';
+        USING MESSAGE = 'You do not have sufficient permissions on this namespace!';
     END IF;
 END;
 $$
diff --git a/test/splitgraph/test_security.py b/test/splitgraph/test_security.py
index 8190fdd6..8a67a2e9 100644
--- a/test/splitgraph/test_security.py
+++ b/test/splitgraph/test_security.py
@@ -91,14 +91,14 @@ def test_push_others(local_engine_empty, readonly_pg_repo):
 
     with pytest.raises(ProgrammingError) as e:
         destination.push(remote_repository=readonly_pg_repo, handler="S3")
-    assert "You do not have access to this namespace!" in str(e.value)
+    assert "You do not have sufficient permissions on this namespace!" in str(e.value)
 
 
 @pytest.mark.registry
 def test_delete_others(readonly_pg_repo):
     with pytest.raises(ProgrammingError) as e:
         readonly_pg_repo.delete(uncheckout=False)
-    assert "You do not have access to this namespace!" in str(e.value)
+    assert "You do not have sufficient permissions on this namespace!" in str(e.value)
 
     with pytest.raises(ProgrammingError) as e:
         readonly_pg_repo.images.delete([readonly_pg_repo.images["latest"].image_hash])
@@ -128,13 +128,13 @@ def test_overwrite_other_object_meta(readonly_pg_repo):
 
     with pytest.raises(ProgrammingError) as e:
         readonly_pg_repo.objects.register_objects([object_meta])
-    assert "You do not have access to this namespace!" in str(e.value)
+    assert "You do not have sufficient permissions on this namespace!" in str(e.value)
     object_meta = readonly_pg_repo.objects.get_object_meta(fruits.objects)[fruits.objects[0]]
     assert object_meta.size != 12345
 
     with pytest.raises(ProgrammingError) as e:
         readonly_pg_repo.objects.register_objects([object_meta], namespace=REMOTE_NAMESPACE)
-    assert "You do not have access to this namespace!" in str(e.value)
+    assert "You do not have sufficient permissions on this namespace!" in str(e.value)
 
 
 @pytest.mark.registry
@@ -146,7 +146,7 @@ def test_impersonate_external_object(readonly_pg_repo):
     # an object that they own.
     with pytest.raises(ProgrammingError) as e:
         readonly_pg_repo.objects.register_object_locations([(sample_object, "fake_location", "S3")])
-    assert "You do not have access to this namespace!" in str(e.value)
+    assert "You do not have sufficient permissions on this namespace!" in str(e.value)
 
 
 @pytest.mark.registry

From 482479e345cecb8e3b5254f0aa18026f74304c44 Mon Sep 17 00:00:00 2001
From: Marko Grujic <markoog@gmail.com>
Date: Wed, 13 Oct 2021 13:51:47 +0000
Subject: [PATCH 5/5] Fix permission levels in access checks

---
 splitgraph/resources/static/splitgraph_api.sql | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/splitgraph/resources/static/splitgraph_api.sql b/splitgraph/resources/static/splitgraph_api.sql
index a26b3ede..f1739975 100644
--- a/splitgraph/resources/static/splitgraph_api.sql
+++ b/splitgraph/resources/static/splitgraph_api.sql
@@ -370,7 +370,7 @@ CREATE OR REPLACE FUNCTION splitgraph_api.tag_image (
     RETURNS void
     AS $$
 BEGIN
-    PERFORM splitgraph_api.check_privilege (_namespace, _repository, 'repository.update_metadata');
+    PERFORM splitgraph_api.check_privilege (_namespace, _repository, 'repository.push_image');
     INSERT INTO splitgraph_meta.tags (namespace, repository, image_hash, tag)
         VALUES (_namespace, _repository, _image_hash, _tag)
     ON CONFLICT (namespace, repository, tag)
@@ -389,7 +389,7 @@ CREATE OR REPLACE FUNCTION splitgraph_api.delete_tag (
     RETURNS void
     AS $$
 BEGIN
-    PERFORM splitgraph_api.check_privilege (_namespace, _repository, 'repository.update_metadata');
+    PERFORM splitgraph_api.check_privilege (_namespace, _repository, 'repository.push_image');
     DELETE FROM splitgraph_meta.tags
         WHERE namespace = _namespace
         AND repository = _repository
@@ -527,7 +527,7 @@ CREATE OR REPLACE FUNCTION splitgraph_api.add_object (
 DECLARE
     existing record;
 BEGIN
-    PERFORM splitgraph_api.check_privilege (_namespace);
+    PERFORM splitgraph_api.check_privilege (_namespace, _action => 'repository.push_image');
     -- Do SELECT FOR UPDATE to lock the row if it actually does exist to avoid two people
     -- calling this at the same time
     SELECT * INTO existing
@@ -542,7 +542,7 @@ BEGIN
 		    _insertion_hash, _deletion_hash, _index, _rows_inserted,
 		    _rows_deleted);
     ELSE
-        PERFORM splitgraph_api.check_privilege (existing.namespace);
+        PERFORM splitgraph_api.check_privilege (existing.namespace, _action => 'repository.push_image');
         PERFORM splitgraph_api.check_objects_privilege (ARRAY[_object_id], 'repository.push_image', true);
         UPDATE
             splitgraph_meta.objects
@@ -568,7 +568,14 @@ CREATE OR REPLACE FUNCTION splitgraph_api.add_object_location (
 )
     RETURNS void
     AS $$
+DECLARE
+    namespace varchar;
 BEGIN
+    namespace = (
+        SELECT o.namespace
+        FROM splitgraph_meta.objects o
+        WHERE o.object_id = _object_id);
+    PERFORM splitgraph_api.check_privilege (namespace, _action => 'repository.push_image');
     PERFORM splitgraph_api.check_objects_privilege (ARRAY[_object_id], 'repository.push_image', true);
     INSERT INTO splitgraph_meta.object_locations (object_id, LOCATION, protocol)
         VALUES (_object_id, LOCATION, protocol)