Jakub,
On Thu, Aug 25, 2016 at 1:56 PM, Jakub Hrozek <jhrozek(a)redhat.com> wrote:
On Wed, Aug 17, 2016 at 03:13:57PM +0200, Fabiano Fidêncio wrote:
> The attached patch for #3125 [0]is based on Jakub's "secrets" branch
> on github [1], as there are (at least) a few issues with the current
> secrets code.
>
> [0]:
https://fedorahosted.org/sssd/ticket/312
> [1]:
https://github.com/jhrozek/sssd/tree/secrets
>
> Although I didn't fire a CI build, the local "make intgcheck" passes
> without issues.
>
> Best Regards,
> --
> Fabiano Fidêncio
Sorry about the late reply. The patch works fine as I would expect:
sudo curl -H "Content-Type: application/json" --unix-socket
/var/run/secrets.socket -XDELETE
http://localhost/secrets/xxx
<html>
<head>
<title>404 Not Found</title></head>
<body>
<h1>Not Found</h1>
<p>The requested resource was not found.</p>
</body>
Nonetheless, I think the patch can be a bit simpler:
> From bc39f7ba0ffa75a9ca972266bbc2741cbb0c8c73 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fidencio(a)redhat.com>
> Date: Wed, 17 Aug 2016 13:12:21 +0200
> Subject: [PATCH] SECRETS: Check whether a secret exists before trying to
> delete it
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> Checking whether a secret exists or not before trying to delete it
> allows SSSD to report the proper error code in case of trying to delete
> a non-existent secret.
>
> Resolves:
>
https://fedorahosted.org/sssd/ticket/3125
>
> Signed-off-by: Fabiano Fidêncio <fidencio(a)redhat.com>
> ---
> src/responder/secrets/local.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/src/responder/secrets/local.c b/src/responder/secrets/local.c
> index e91766f..3985b6d 100644
> --- a/src/responder/secrets/local.c
> +++ b/src/responder/secrets/local.c
> @@ -376,12 +376,26 @@ int local_db_delete(TALLOC_CTX *mem_ctx,
> struct local_context *lctx,
> const char *req_path)
> {
> + static const char *attrs[] = { "secret", NULL };
> struct ldb_dn *dn;
> + struct ldb_result *res;
> int ret;
>
> ret = local_db_dn(mem_ctx, lctx->ldb, req_path, &dn);
> if (ret != EOK) goto done;
>
> + ret = ldb_search(lctx->ldb, mem_ctx, &res, dn, LDB_SCOPE_BASE,
> + attrs, "%s", LOCAL_SIMPLE_FILTER);
> + if (ret != EOK) {
> + ret = ENOENT;
> + goto done;
> + }
> +
> + if (res->count == 0) {
> + ret = ENOENT;
> + goto done;
> + }
> +
> ret = ldb_delete(lctx->ldb, dn);
I think it's OK to just try to delete the secret and act on the return
value. This is what we already have in sysdb_delete_cache_entry():
105 ret = ldb_delete(ldb, dn);
106 switch (ret) {
107 case LDB_SUCCESS:
108 return EOK;
109 case LDB_ERR_NO_SUCH_OBJECT:
110 if (ignore_not_found) {
111 return EOK;
112 }
So I think we can use something else in secrets to return ENOENT on
receiving LDB_ERR_NO_SUCH_OBJECT from ldb_delete().
Suggestion taken. You can find the v2 attached to this email.
Best Regards,
--
Fabiano Fidêncio