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
From 3e71f901ab12f5e5c570dcf0796ef47e379e7a53 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 v2] SECRETS: Return ENOENT when_deleting a non-existent secret
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The approach taken is quite similar to what is done by
sysdb_delete_cache_entry().
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, 8 insertions(+), 6 deletions(-)
diff --git a/src/responder/secrets/local.c b/src/responder/secrets/local.c
index e91766f..c32af8c 100644
--- a/src/responder/secrets/local.c
+++ b/src/responder/secrets/local.c
@@ -380,15 +380,17 @@ int local_db_delete(TALLOC_CTX *mem_ctx,
int ret;
ret = local_db_dn(mem_ctx, lctx->ldb, req_path, &dn);
- if (ret != EOK) goto done;
+ if (ret != EOK) return ret;
ret = ldb_delete(lctx->ldb, dn);
- if (ret != EOK) {
- ret = EIO;
+ switch (ret) {
+ case LDB_SUCCESS:
+ return EOK;
+ case LDB_ERR_NO_SUCH_OBJECT:
+ return ENOENT;
+ default:
+ return EIO;
}
Is this mapping ldb_error to ernno different as in sysdb_error_to_errno?
If no then it would be good to reuse existing fucntion.
LS