From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <v.shpilevoy@tarantool.org>
Received: from smtp32.i.mail.ru (smtp32.i.mail.ru [94.100.177.92])
 (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
 (No client certificate requested)
 by dev.tarantool.org (Postfix) with ESMTPS id C60A3440F3C
 for <tarantool-patches@dev.tarantool.org>;
 Wed,  6 Nov 2019 17:42:49 +0300 (MSK)
References: <cover.1572965692.git.v.shpilevoy@tarantool.org>
 <428837a3e85ef7a36ac842891794e137f17f20e8.1572965692.git.v.shpilevoy@tarantool.org>
 <20191105181552.GK29784@atlas>
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Message-ID: <b762ec62-dfbd-4860-a5b9-dcc7de9140b2@tarantool.org>
Date: Wed, 6 Nov 2019 17:48:41 +0300
MIME-Version: 1.0
In-Reply-To: <20191105181552.GK29784@atlas>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [Tarantool-patches] [PATCH 1/3] access: fix invalid error type
	for not found user
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Konstantin Osipov <kostja.osipov@gmail.com>, tarantool-patches@dev.tarantool.org

Hi! Thanks for the review!

On 05/11/2019 21:15, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/11/05 17:57]:
>> +++ b/src/box/user.cc
>> @@ -521,8 +521,9 @@ user_find_by_name(const char *name, uint32_t len)
>>  	if (schema_find_id(BOX_USER_ID, 2, name, len, &uid) != 0)
>>  		return NULL;
>>  	struct user *user = user_by_id(uid);
>> -	if (user == NULL || user->def->type != SC_USER) {
>> -		diag_set(ClientError, ER_NO_SUCH_USER, tt_cstr(name, len));
>> +	if (user == NULL || user->def->type != SC_USER || uid == BOX_ID_NIL) {
>> +		diag_set(ClientError, ER_NO_SUCH_USER,
>> +			 tt_cstr(name, MIN(BOX_INVALID_NAME_MAX, len)));
>>  		return NULL;
> 
> I would not call user_by_id with BOX_ID_NIL, this is not an error
> but it takes an effort to ponder on and realize it's OK to do so.
> 
> Otherwise LGTM.
> 
> 

Hm, you are right. I should not search by an invalid ID. Fixed
on the branch.

============================================================================

diff --git a/src/box/user.cc b/src/box/user.cc
index 03b4b2e3b..e2fd32740 100644
--- a/src/box/user.cc
+++ b/src/box/user.cc
@@ -520,12 +520,14 @@ user_find_by_name(const char *name, uint32_t len)
 	uint32_t uid;
 	if (schema_find_id(BOX_USER_ID, 2, name, len, &uid) != 0)
 		return NULL;
-	struct user *user = user_by_id(uid);
-	if (user == NULL || user->def->type != SC_USER) {
-		diag_set(ClientError, ER_NO_SUCH_USER, tt_cstr(name, len));
-		return NULL;
+	if (uid != BOX_ID_NIL) {
+		struct user *user = user_by_id(uid);
+		if (user != NULL && user->def->type == SC_USER)
+			return user;
 	}
-	return user;
+	diag_set(ClientError, ER_NO_SUCH_USER,
+		 tt_cstr(name, MIN(BOX_INVALID_NAME_MAX, len)));
+	return NULL;
 }

============================================================================