This series divises the "write" access right into "add", "delete" and "update" sub-rights. After a preliminary work in patch 1, patch 2 switches all administration operations to HTTP POST requests, laying the ground to the implemention of the new access rights in patch 3. Julien Floret (3): job: forbid changing internal status if locked views: use HTTP POST methods for admin operations auth: define more fine-grained access rights dlrepo-cli | 18 ++++++-- dlrepo/fs/job.py | 2 + dlrepo/views/auth.py | 73 ++++++++++++++++++++++-------- dlrepo/views/branch.py | 2 +- dlrepo/views/job.py | 33 +++++++++++--- dlrepo/views/tag.py | 2 +- dlrepo/views/util.py | 2 +- tests/test_auth.py | 90 +++++++++++++++++++++++++++++++++++++ tests/test_auth/acls/group4 | 9 ++++ tests/test_auth/auth | 1 + tests/test_publish.py | 5 ++- 11 files changed, 206 insertions(+), 31 deletions(-) create mode 100644 tests/test_auth/acls/group4 -- 2.30.2
Julien Floret, Mar 27, 2023 at 15:57:
dlrepo/patches/.build.yml: SUCCESS in 1m34s [more fine-grained access rights][0] from [Julien Floret][1] [0]: https://lists.sr.ht/~rjarry/dlrepo/patches/40023 [1]: mailto:julien.floret@6wind.com ✓ #963507 SUCCESS dlrepo/patches/.build.yml https://builds.sr.ht/~rjarry/job/963507
Le ven. 31 mars 2023 à 21:20, Robin Jarry <robin@jarry.cc> a écrit :
Copy & paste the following snippet into your terminal to import this patchset into git:
curl -s https://lists.sr.ht/~rjarry/dlrepo/patches/40023/mbox | git am -3Learn more about email & git
Now, jobs must be unlocked prior to changing their release
status ("internal" or releasable). This will be useful in the next
commits.
Signed-off-by: Julien Floret <julien.floret@6wind.com>
Acked-by: Thomas Faivre <thomas.faivre@6wind.com>
---
dlrepo/fs/job.py | 2 ++
1 file changed, 2 insertions(+)
diff --git a/dlrepo/fs/job.py b/dlrepo/fs/job.py
index 5e92e6d9cffa..cc177ba15ffb 100644
--- a/dlrepo/fs/job.py
+++ b/dlrepo/fs/job.py
@@ -208,6 +208,8 @@ class Job(SubDir):
return self._internal_path().is_file()
def set_internal(self, internal: bool):
+ if self.is_locked():
+ raise FileExistsError("Job is locked")
path = self._internal_path()
if internal:
path.touch()
--
2.30.2
dlrepo uses the HTTP PUT method for creating new jobs (uploading artifacts and "finalizing" the newly created job). On the other hand, the HTTP PUT method is also used for so-called "administration" operations, which are: - releasing and unreleasing tags - marking tags stable or unstable - locking tags (to save them from automatic cleanup) - managing the branches' cleanup policy - modifying an existing job For these operations, let's use the HTTP POST method instead. Having a different method for administration tasks will help us define more fine-grained access rights in a next commit. There is small conceptual difference between the two methods; as explained in the link below, PUT is considered idempotent, whereas POST can cause a change of state or side effects in the server. Link: https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods Signed-off-by: Julien Floret <julien.floret@6wind.com> Acked-by: Thomas Faivre <thomas.faivre@6wind.com>
Hi Julien, This looks good. Could you also update the dlrepo-api man page?Julien Floret <julien.floret@6wind.com>Hi Robin, Oh right, I forgot to update the doc in this patch and the next. I'll send a v2 for this series. Thanks!Thanks!
--- dlrepo-cli | 18 ++++++++++++++---- dlrepo/views/branch.py | 2 +- dlrepo/views/job.py | 33 ++++++++++++++++++++++++++++----- dlrepo/views/tag.py | 2 +- tests/test_publish.py | 5 ++++- 5 files changed, 48 insertions(+), 12 deletions(-) diff --git a/dlrepo-cli b/dlrepo-cli index 13fa16f5157b..29561da8882b 100755 --- a/dlrepo-cli +++ b/dlrepo-cli @@ -274,10 +274,14 @@ def lock(args): if args.job: url = os.path.join("branches", args.branch, args.tag, args.job) + "/" data = {"job": data} + if args.unlock: + client.post(url, data) + else: + client.put(url, data) else: url = os.path.join("branches", args.branch, args.tag) + "/" data = {"tag": data} - client.put(url, data) + client.post(url, data) # -------------------------------------------------------------------------------------- @@ -393,7 +397,7 @@ def release(args): """ client = HttpClient(args.url) url = os.path.join("branches", args.branch, args.tag) + "/" - client.put(url, {"tag": {"released": not args.unset}}) + client.post(url, {"tag": {"released": not args.unset}}) # -------------------------------------------------------------------------------------- @@ -413,7 +417,7 @@ def stable(args): """ client = HttpClient(args.url) url = os.path.join("branches", args.branch, args.tag) + "/" - client.put(url, {"tag": {"stable": not args.unset}}) + client.post(url, {"tag": {"stable": not args.unset}}) # -------------------------------------------------------------------------------------- @@ -511,7 +515,7 @@ def cleanup_policy(args): if args.max_released is not None: policy["max_released_tags"] = args.max_released if policy: - client.put(os.path.join("branches", args.branch) + "/", {"branch": policy}) + client.post(os.path.join("branches", args.branch) + "/", {"branch": policy}) else: data = client.get(os.path.join("branches", args.branch) + "/") if args.raw_json: @@ -679,6 +683,12 @@ class HttpClient: headers["Content-Type"] = "text/plain" return body, headers + def post(self, url, body, headers=None): + url = self.make_url(url) + body, headers = self._encode_body(body, headers) + request = Request(url, body, method="POST") + return self._send(request, headers) + def put(self, url, body, headers=None): url = self.make_url(url) body, headers = self._encode_body(body, headers) diff --git a/dlrepo/views/branch.py b/dlrepo/views/branch.py index 2b306ebfceb4..868d5900535d 100644 --- a/dlrepo/views/branch.py +++ b/dlrepo/views/branch.py @@ -95,7 +95,7 @@ class BranchView(BaseView): return aiohttp_jinja2.render_template("branch.html", self.request, data) return web.json_response(data) - async def put(self): + async def post(self): """ Update the cleanup policy for a branch. """ diff --git a/dlrepo/views/job.py b/dlrepo/views/job.py index 2c6a13cd3d30..644cd3ae3576 100644 --- a/dlrepo/views/job.py +++ b/dlrepo/views/job.py @@ -93,16 +93,39 @@ class JobView(BaseView): if internal is not None and not isinstance(internal, bool): raise TypeError() locked = data.get("locked") - if locked is not None and not isinstance(locked, bool): - raise TypeError() - except (TypeError, KeyError) as e: + if locked is not None: + if not isinstance(locked, bool): + raise TypeError() + if not locked: + raise ValueError() + except (TypeError, KeyError, ValueError) as e: raise web.HTTPBadRequest(reason="Invalid parameters") from e try: if internal is not None: job.set_internal(internal) - if locked is not None: - await job.set_locked(locked) + if locked: + await job.set_locked(True) + except FileNotFoundError as e: + raise web.HTTPNotFound() from e + except OSError as e: + raise web.HTTPBadRequest(reason=str(e)) from e + + return web.Response() + + async def post(self): + job = _get_job(self.repo(), self.request) + try: + locked = (await self.json_body())["job"]["locked"] + if not isinstance(locked, bool): + raise TypeError() + if locked: + raise ValueError() + except (TypeError, KeyError, ValueError) as e: + raise web.HTTPBadRequest(reason="Invalid parameters") from e + + try: + await job.set_locked(False) except FileNotFoundError as e: raise web.HTTPNotFound() from e except OSError as e: diff --git a/dlrepo/views/tag.py b/dlrepo/views/tag.py index 5e75d28dc486..a2779093fb24 100644 --- a/dlrepo/views/tag.py +++ b/dlrepo/views/tag.py @@ -74,7 +74,7 @@ class TagView(BaseView): return aiohttp_jinja2.render_template("tag.html", self.request, data) return web.json_response(data) - async def put(self): + async def post(self): """ Change the released, stable and/or locked statuses of a tag. """ diff --git a/tests/test_publish.py b/tests/test_publish.py index 7c9f2f572749..c14fceeb3383 100644 --- a/tests/test_publish.py +++ b/tests/test_publish.py @@ -64,7 +64,10 @@ def dlrepo_servers(request, dlrepo_server): async def test_publish(dlrepo_servers): # pylint: disable=redefined-outer-name url, public_url = dlrepo_servers async with aiohttp.ClientSession(url) as sess: - resp = await sess.put("/branches/branch/tag/", json={"tag": {"released": True}}) + resp = await sess.post( + "/branches/branch/tag/", + json={"tag": {"released": True}}, + ) assert resp.status == 200 await asyncio.sleep(1) resp = await sess.get("/branches/branch/tag/") -- 2.30.2
Julien Floret, Mar 27, 2023 at 15:57:
Before this commit, only two different access rights were defined: "read" for HTTP GET and HEAD methods, and "write" for all other methods. Thus, the "write" access right allowed very different operations; adding and deleting artifacts, but also releasing tags or managing the tag cleanup policies... This commit splits the "write" access right into several sub-rights: "a" (for "add") covers the creation of new jobs: - uploading new artifacts, - removing the "dirty" flag on new formats, - setting a job's "internal" flag, - locking new jobs to prevent any further alterations. "add" operations are done via HTTP PUT and PATCH requests (and POST in case of docker registry: "/v2/*" URLs). "d" (for "delete") covers the deletion of branches, tags and jobs. "delete" operations are done via HTTP DELETE requests. "u" (for "update") covers administration operations: - unlocking existing jobs for modifications, - releasing and unreleasing tags, - marking tags stable or unstable, - locking tags (to save them from automatic cleanup), - managing the branches' cleanup policy. "update" operations are done via HTTP POST requests (except for docker commands, see above). While the "add" access right should typically be given to CI tools that automatically build and upload artifacts, the "delete" and "update" rights on the root repository should typically be reserved to a handful of administrators. Signed-off-by: Julien Floret <julien.floret@6wind.com> Acked-by: Thomas Faivre <thomas.faivre@6wind.com> --- dlrepo/views/auth.py | 73 ++++++++++++++++++++++-------- dlrepo/views/util.py | 2 +- tests/test_auth.py | 90 +++++++++++++++++++++++++++++++++++++ tests/test_auth/acls/group4 | 9 ++++ tests/test_auth/auth | 1 + 5 files changed, 156 insertions(+), 19 deletions(-) create mode 100644 tests/test_auth/acls/group4 diff --git a/dlrepo/views/auth.py b/dlrepo/views/auth.py index e070c52f28ee..8a67dece99b0 100644 --- a/dlrepo/views/auth.py +++ b/dlrepo/views/auth.py @@ -187,8 +187,12 @@ class AuthBackend: if self.AUTH_DISABLED: return - is_write = request.method not in ("GET", "HEAD") - if not is_write: + is_add = request.method in ("PUT", "PATCH") or ( + request.method == "POST" and request.path.startswith("/v2/") + ) + is_delete = request.method == "DELETE" + is_update = request.method == "POST" and not request.path.startswith("/v2/") + if not is_add and not is_delete and not is_update: if request.path.startswith("/static/") or request.path == "/favicon.ico": # no need for authentication for these return @@ -206,7 +210,9 @@ class AuthBackend: if login is None: groups = frozenset({"ANONYMOUS"}) - if login is None and (is_write or request.path == "/v2/"): + if login is None and ( + is_add or is_delete or is_update or request.path == "/v2/" + ): # anonymous write access is always denied # the docker registry API states that anonymous GET requests to /v2/ # must receive a 401 unauthorized error @@ -224,27 +230,29 @@ class AuthBackend: request["dlrepo_user_acls"] = acls if not self.access_granted( - acls, is_write, request.path, request["dlrepo_user"] + acls, is_add, is_delete, is_update, request.path, request["dlrepo_user"] ): raise self.auth_error(request) def access_granted( self, acls: FrozenSet["ACL"], - is_write: bool, + is_add: bool, + is_delete: bool, + is_update: bool, url: str, username: Optional[str] = None, ) -> bool: """ Check if the provided ACLs give write and/or read access to the specified URL. """ - access_key = (acls, is_write, url) + access_key = (acls, is_add, is_delete, is_update, url) granted = self.access_cache.get(access_key) if granted is not None: return granted granted = False for acl in acls: - if acl.access_granted(is_write, url): + if acl.access_granted(is_add, is_delete, is_update, url): if username is not None: LOG.debug("access granted to %s by ACL %s", username, acl) granted = True @@ -317,15 +325,22 @@ def parse_group_acls() -> Dict[str, FrozenSet["ACL"]]: if len(tokens) < 2: continue access, *args = tokens - if access not in ("ro", "rw"): + + # "w" is equivalent to "adu" + # "o" is ignored for compatibility with "ro" + if not set(access).issubset({"r", "o", "w", "a", "d", "u"}): continue + + add = "a" in access or "w" in access + delete = "d" in access or "w" in access + update = "u" in access or "w" in access globs = [] for a in args: if a.startswith("!"): globs.append((a[1:], True)) else: globs.append((a, False)) - acls.add(ACL(access == "rw", frozenset(globs))) + acls.add(ACL(add, delete, update, frozenset(globs))) group_acls[file.name] = frozenset(acls) except FileNotFoundError as e: @@ -337,8 +352,12 @@ def parse_group_acls() -> Dict[str, FrozenSet["ACL"]]: # -------------------------------------------------------------------------------------- class ACL: - def __init__(self, read_write: bool, globs: FrozenSet[Tuple[str, bool]]): - self.read_write = read_write + def __init__( + self, add: bool, delete: bool, update: bool, globs: FrozenSet[Tuple[str, bool]] + ): + self.add = add + self.delete = delete + self.update = update self.globs = globs self.can_expand_user = any("$user" in g for g, _ in self.globs) self.patterns = [ @@ -351,10 +370,16 @@ class ACL: globs = [] for glob, invert in self.globs: globs.append((glob.replace("$user", login), invert)) - return ACL(self.read_write, frozenset(globs)) + return ACL(self.add, self.delete, self.update, frozenset(globs)) - def access_granted(self, is_write: bool, path: str) -> bool: - if is_write and not self.read_write: + def access_granted( + self, is_add: bool, is_delete: bool, is_update: bool, path: str + ) -> bool: + if is_add and not self.add: + return False + if is_delete and not self.delete: + return False + if is_update and not self.update: return False granted = True for pattern, invert in self.patterns: @@ -391,7 +416,14 @@ class ACL: return re.compile(pattern) def __str__(self): - access = "rw" if self.read_write else "ro" + access = "r" + if self.add: + access += "a" + if self.delete: + access += "d" + if self.update: + access += "u" + globs = [] for glob, invert in self.globs: if invert: @@ -400,12 +432,17 @@ class ACL: return f"{access} {' '.join(globs)}" def __repr__(self): - return f"ACL(read_write={self.read_write}, globs={self.globs})" + return f"ACL(add={self.add}, delete={self.delete}, update={self.update}, globs={self.globs})" def __eq__(self, other): if not isinstance(other, ACL): return False - return other.read_write == self.read_write and other.globs == self.globs + return ( + other.add == self.add + and other.delete == self.delete + and other.update == self.update + and other.globs == self.globs + ) def __hash__(self): - return hash((self.read_write, self.globs)) + return hash((self.add, self.delete, self.update, self.globs)) diff --git a/dlrepo/views/util.py b/dlrepo/views/util.py index 09d7521214b4..c231b0272473 100644 --- a/dlrepo/views/util.py +++ b/dlrepo/views/util.py @@ -34,7 +34,7 @@ class BaseView(web.View): return True auth_backend = self.request.app[auth.AuthBackend.KEY] acls = self.request["dlrepo_user_acls"] - return auth_backend.access_granted(acls, False, url) + return auth_backend.access_granted(acls, False, False, False, url) X_SENDFILE_HEADER = os.getenv("DLREPO_X_SENDFILE_HEADER", None) diff --git a/tests/test_auth.py b/tests/test_auth.py index 306e042fb4f2..caaa3bba721c 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -114,3 +114,93 @@ async def test_acl_regexp(dlrepo_server): assert resp.status == 200 data = await resp.read() assert data == b"content" + + +async def _test_acl_combination(dlrepo_server, tag, add, delete, update): + tag = f"/branches/main/{tag}" + url, _ = dlrepo_server + data = b"content" + digest = hashlib.sha256(data).hexdigest() + async with aiohttp.ClientSession(url, auth=aiohttp.BasicAuth("bar", "bar")) as sess: + resp = await sess.put( + f"{tag}/job/format/file.txt", + data=data, + headers={"Digest": f"sha256:{digest}"}, + ) + assert resp.status == 201 + async with aiohttp.ClientSession( + url, auth=aiohttp.BasicAuth("plop", "plop") + ) as sess: + resp = await sess.put( + f"{tag}/job/format/file.txt", + data=data, + headers={"Digest": f"sha256:{digest}"}, + ) + if add: + assert resp.status == 201 + else: + assert resp.status == 401 + resp = await sess.post(tag, json={"tag": {"stable": True}}) + if update: + assert resp.status == 200 + else: + assert resp.status == 401 + resp = await sess.delete(f"{tag}/job") + if delete: + assert resp.status == 200 + else: + assert resp.status == 401 + + +async def test_r(dlrepo_server): + await _test_acl_combination( + dlrepo_server, "test_r_tag", add=False, delete=False, update=False + ) + + +async def test_ra(dlrepo_server): + await _test_acl_combination( + dlrepo_server, "test_ra_tag", add=True, delete=False, update=False + ) + + +async def test_rd(dlrepo_server): + await _test_acl_combination( + dlrepo_server, "test_rd_tag", add=False, delete=True, update=False + ) + + +async def test_ru(dlrepo_server): + await _test_acl_combination( + dlrepo_server, "test_ru_tag", add=False, delete=False, update=True + ) + + +async def test_rad(dlrepo_server): + await _test_acl_combination( + dlrepo_server, "test_rad_tag", add=True, delete=True, update=False + ) + + +async def test_rau(dlrepo_server): + await _test_acl_combination( + dlrepo_server, "test_rau_tag", add=True, delete=False, update=True + ) + + +async def test_rdu(dlrepo_server): + await _test_acl_combination( + dlrepo_server, "test_rdu_tag", add=False, delete=True, update=True + ) + + +async def test_radu(dlrepo_server): + await _test_acl_combination( + dlrepo_server, "test_radu_tag", add=True, delete=True, update=True + ) + + +async def test_rw(dlrepo_server): + await _test_acl_combination( + dlrepo_server, "test_rw_tag", add=True, delete=True, update=True + ) diff --git a/tests/test_auth/acls/group4 b/tests/test_auth/acls/group4 new file mode 100644 index 000000000000..c61875bd815b --- /dev/null +++ b/tests/test_auth/acls/group4 @@ -0,0 +1,9 @@ +r /branches/main/test_r_tag** +ra /branches/main/test_ra_tag** +rd /branches/main/test_rd_tag** +ru /branches/main/test_ru_tag** +rad /branches/main/test_rad_tag** +rau /branches/main/test_rau_tag** +rdu /branches/main/test_rdu_tag** +radu /branches/main/test_radu_tag** +rw /branches/main/test_rw_tag** diff --git a/tests/test_auth/auth b/tests/test_auth/auth index 7ddc01e77bca..ae3212d1be67 100644 --- a/tests/test_auth/auth +++ b/tests/test_auth/auth @@ -1,3 +1,4 @@ foo:baz:group1 bar:bar:group2 coin:coin:group3 +plop:plop:group4 -- 2.30.2
builds.sr.ht <builds@sr.ht>dlrepo/patches/.build.yml: SUCCESS in 1m34s [more fine-grained access rights][0] from [Julien Floret][1] [0]: https://lists.sr.ht/~rjarry/dlrepo/patches/40023 [1]: mailto:julien.floret@6wind.com ✓ #963507 SUCCESS dlrepo/patches/.build.yml https://builds.sr.ht/~rjarry/job/963507