diff mbox series

[v2,1/1] Replace SID with domain/username

Message ID 20231229120319.3797-2-soekkle@freenet.de (mailing list archive)
State Superseded
Headers show
Series Replace SID with domain/username on Windows | expand

Commit Message

Sören Krecker Dec. 29, 2023, 12:03 p.m. UTC
Replace SID with domain/username in erromessage, if owner of repository
and user are not equal on windows systems.

Signed-off-by: Sören Krecker <soekkle@freenet.de>
---
 compat/mingw.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

Comments

Junio C Hamano Jan. 2, 2024, 4:20 p.m. UTC | #1
Sören Krecker <soekkle@freenet.de> writes:

> Replace SID with domain/username in erromessage, if owner of repository
> and user are not equal on windows systems.

"erromessage" -> "error messages" or something?

This may not be a question raised by anybody who know Windows, but
because I do not do Windows, it makes me wonder if this is losing
information.  Can two SID for the same user be active at the same
time, which would cause user_sid_to_user_name() potentially yield
the same string for two different SID?

In any case, I am reasonably sure that Dscho will say yes or no to
this patch (the above "makes me wonder" does not need to be
resolved) and I can wait until then.

Thanks.

> Signed-off-by: Sören Krecker <soekkle@freenet.de>
> ---
>  compat/mingw.c | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 42053c1f65..05aeaaa9ad 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -2684,6 +2684,26 @@ static PSID get_current_user_sid(void)
>  	return result;
>  }
>  
> +static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
> +{
> +	SID_NAME_USE pe_use;
> +	DWORD len_user = 0, len_domain = 0;
> +	BOOL translate_sid_to_user;
> +
> +	/* returns only FALSE, because the string pointers are NULL*/
> +	LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
> +			  &pe_use); 
> +	/*Alloc needed space of the strings*/
> +	ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); 
> +	translate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,
> +				   *str, &len_domain, &pe_use);
> +	*(*str + len_domain) = '/';
> +	if (translate_sid_to_user == FALSE) {
> +		FREE_AND_NULL(*str);
> +	}
> +	return translate_sid_to_user;
> +}
> +
>  static int acls_supported(const char *path)
>  {
>  	size_t offset = offset_1st_component(path);
> @@ -2767,7 +2787,7 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
>  		} else if (report) {
>  			LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
>  
> -			if (ConvertSidToStringSidA(sid, &str1))
> +			if (user_sid_to_user_name(sid, &str1))
>  				to_free1 = str1;
>  			else
>  				str1 = "(inconvertible)";
> @@ -2776,7 +2796,7 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
>  				str2 = "(none)";
>  			else if (!IsValidSid(current_user_sid))
>  				str2 = "(invalid)";
> -			else if (ConvertSidToStringSidA(current_user_sid, &str2))
> +			else if (user_sid_to_user_name(current_user_sid, &str2))
>  				to_free2 = str2;
>  			else
>  				str2 = "(inconvertible)";
> @@ -2784,8 +2804,8 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
>  				    "'%s' is owned by:\n"
>  				    "\t'%s'\nbut the current user is:\n"
>  				    "\t'%s'\n", path, str1, str2);
> -			LocalFree(to_free1);
> -			LocalFree(to_free2);
> +			free(to_free1);
> +			free(to_free2);
>  		}
>  	}
Junio C Hamano Jan. 2, 2024, 5:33 p.m. UTC | #2
Junio C Hamano <gitster@pobox.com> writes:

> Sören Krecker <soekkle@freenet.de> writes:
>
>> Replace SID with domain/username in erromessage, if owner of repository
>> and user are not equal on windows systems.
>
> "erromessage" -> "error messages" or something?
>
> This may not be a question raised by anybody who know Windows, but
> because I do not do Windows, it makes me wonder if this is losing
> information.  Can two SID for the same user be active at the same
> time, which would cause user_sid_to_user_name() potentially yield
> the same string for two different SID?
>
> In any case, I am reasonably sure that Dscho will say yes or no to
> this patch (the above "makes me wonder" does not need to be
> resolved) and I can wait until then.
>
> Thanks.

Another thing I forgot to mention (but did wonder).  The new helper
function does allow LookupAccountSidA() to fail.  Should it fall
back to ConvertSidToStringSidA() that the original has been using?

In any case, I do not think a failure to convert will result in an
attempt to format ("%s", NULL) thanks to the existing code that uses
the stringified SID, which is good.
diff mbox series

Patch

diff --git a/compat/mingw.c b/compat/mingw.c
index 42053c1f65..05aeaaa9ad 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2684,6 +2684,26 @@  static PSID get_current_user_sid(void)
 	return result;
 }
 
+static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
+{
+	SID_NAME_USE pe_use;
+	DWORD len_user = 0, len_domain = 0;
+	BOOL translate_sid_to_user;
+
+	/* returns only FALSE, because the string pointers are NULL*/
+	LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
+			  &pe_use); 
+	/*Alloc needed space of the strings*/
+	ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); 
+	translate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,
+				   *str, &len_domain, &pe_use);
+	*(*str + len_domain) = '/';
+	if (translate_sid_to_user == FALSE) {
+		FREE_AND_NULL(*str);
+	}
+	return translate_sid_to_user;
+}
+
 static int acls_supported(const char *path)
 {
 	size_t offset = offset_1st_component(path);
@@ -2767,7 +2787,7 @@  int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
 		} else if (report) {
 			LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
 
-			if (ConvertSidToStringSidA(sid, &str1))
+			if (user_sid_to_user_name(sid, &str1))
 				to_free1 = str1;
 			else
 				str1 = "(inconvertible)";
@@ -2776,7 +2796,7 @@  int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
 				str2 = "(none)";
 			else if (!IsValidSid(current_user_sid))
 				str2 = "(invalid)";
-			else if (ConvertSidToStringSidA(current_user_sid, &str2))
+			else if (user_sid_to_user_name(current_user_sid, &str2))
 				to_free2 = str2;
 			else
 				str2 = "(inconvertible)";
@@ -2784,8 +2804,8 @@  int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
 				    "'%s' is owned by:\n"
 				    "\t'%s'\nbut the current user is:\n"
 				    "\t'%s'\n", path, str1, str2);
-			LocalFree(to_free1);
-			LocalFree(to_free2);
+			free(to_free1);
+			free(to_free2);
 		}
 	}