maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Vicentiu!

Two comments to the patch - see below.

One general comment - you've done quite a bit of refactoring and this
refactoring is *not* in 10.1 style. How should it be merged into 10.1?
Should your changes be ignored (and not merged)? Should they be merged
as is? Aren't they mostly unnecessary in 10.1?

On May 05, vicentiu@xxxxxxxxxxx wrote:
> revision-id: 54e9df42d1f5837ae0df51e511ba48d3682be20d
> parent(s): ae18a28500974351cf42fa3cac67c83e0647d510
> committer: Vicențiu Ciorbaru
> branch nick: server
> timestamp: 2015-05-05 21:19:53 +0300
> message:
> 
> MDEV-7985: MySQL Users Break when Migrating to MariaDB, part 2
> 
> Gave priority to password field when using a native authentication
> plugin.
> 
> Also, prevented a user from setting an invalid auth_string, when using
> native authentication.
> 
> diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc
> --- a/sql/sql_acl.cc
> +++ b/sql/sql_acl.cc
> @@ -862,6 +862,17 @@ static char *fix_plugin_ptr(char *name)
>      return name;
>  }
>  
> +static bool plugin_is_native(const char* plugin)
> +{
> +  if (!plugin)
> +    return false;
> +  if ((my_strcasecmp(system_charset_info, plugin,
> +                    native_password_plugin_name.str) == 0) ||
> +      (my_strcasecmp(system_charset_info, plugin,
> +                    old_password_plugin_name.str) == 0))
> +    return true;
> +  return false;
> +}

I tried to compare plugin names almost always by pointers, not by
comparing strings. That is, the string (as coming from the table or from
the user) is strcmp'ed *once* and then replaced with a pointer to a
static plugin name string.

So I'd prefer it if you'd remove plugin_is_native() and instead used
fix_user_plugin_ptr(). And then you remove fix_user_plugin_ptr() from
acl_update_user() and acl_insert_user().

>  /**
>    Fix ACL::plugin pointer to point to a hard-coded string, if appropriate
>  
> @@ -1973,6 +2020,10 @@ static void acl_update_user(const char *user, const char *host,
>          acl_user->auth_string.length= auth->length;
>          if (fix_user_plugin_ptr(acl_user))
>            acl_user->plugin.str= strmake_root(&acl_memroot, plugin->str, plugin->length);
> +        else
> +          set_user_salt(acl_user, acl_user->auth_string.str,
> +                                  acl_user->auth_string.length);

what if auth_string is empty, how does the password take precedence in
this case?

> +
>        }
>        else
>          if (password[0])
Regards,
Sergei