Re: [PATCH V4 1/1] Replace SID with domain/username

[Junio C Hamano <gitster@pobox.com>]

Sören Krecker <soekkle@freenet.de> writes:

> Replace SID with domain/username in error message, if owner of repository
> and user are not equal on windows systems. Each user should have a unique
> SID (https://learn.microsoft.com/en-us/windows-server/identity/ad-ds/manage/understand-security-identifiers#what-are-security-identifiers).

That paragraph your URL refers to does say that a SID that is used
for an account will never be reused to identify a different account.
But I am not sure if it means a user will never be assigned more
than one SID (in other words, the reverse is not necessarily true).

The paragraph also mentions that a SID can identify a non-user
entity like a computer account (as opposed to "a user account")---I
do not know what its implications are in the context of this patch,
though.

> This means that domain/username is not a loss of information.

This statement does not (grammatically) make sense, but more
importantly, loss of information may not be a bad thing in this
case.  If more than one SIDs are given to a user account and
processes working for that account, these different SIDs may be
translated, by using LookupAccountSidA(), to the same string for a
single user@domain, and it would be an operation that loses
information in that sense.

But if what we *care* about is user@domain between the current
process and the owner of the directory in question being the same
(or not), then such a loss of information is a *good* thing.  

So I dunno.  Arguing what we care about (is that exact SID equality
between the "owner of the directory" and the "user, which the
current process is working on behalf of", or do we care about the
equality of the "accounts"?) may be a better way to justify this
change, if you ask me.

> +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);
> +	if (translate_sid_to_user == FALSE) {
> +		FREE_AND_NULL(*str);
> +	}

Style: do not enclose a single-statement block inside {}.

> +	else
> +		(*str)[len_domain] = '/';
> +	return translate_sid_to_user;
> +}

> @@ -2767,7 +2788,9 @@ 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 if (ConvertSidToStringSidA(sid, &str1))
>  				to_free1 = str1;

Do these two helper functions return pointers pointing into the same
kind of memory that you can free with the same function?  That is ...

> ...
>  				    "'%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);

... the original code seems to say that the piece of memory we
obtain from ConvertSidToStringSidA() must not be freed by calling
free() but use something special called LocalFree().  I am assuing
that your user_sid_to_user_name() returns a regular piece of memory
that can be freed by calling regular free()?  Do we need to keep
track of where we got the memory from and use different function to
free each variable, or something (again I do not do Windows so I'll
defer all of these to Dscho, who is CC'ed this time).

Thanks and a happy new year.

>  		}
>  	}