Hi!
On Aug 02, Sergey Kudriavtsev wrote:
> 2008/7/31 Sergei Golubchik <serg@stripped>:
>
> >> === modified file 'scripts/mysql_system_tables_fix.sql'
> >> --- scripts/mysql_system_tables_fix.sql 2008-04-29 04:37:22 +0000
> >> +++ scripts/mysql_system_tables_fix.sql 2008-07-06 19:53:28 +0000
> >> @@ -520,6 +520,30 @@ ALTER TABLE tables_priv MODIFY Table_pri
> >> +
> >> +SET @hadCreateRolePriv := 0;
> >> +SELECT @hadCreateRolePriv :=1 FROM user WHERE Create_role_priv LIKE '%';
> >> +ALTER TABLE user ADD Create_role_priv enum('N','Y') COLLATE utf8_general_ci
> DEFAULT 'N' NOT NULL AFTER Trigger_priv;
> >> +ALTER TABLE user MODIFY Create_role_priv enum('N','Y') COLLATE
> utf8_general_ci DEFAULT 'N' NOT NULL AFTER Trigger_priv;
> >> +UPDATE user SET Create_role_priv=Create_user_priv WHERE @hadCreateRolePriv
> = 0;
> >> +SET @hadDropRolePriv := 0;
> >> +SELECT @hadDropRolePriv :=1 FROM user WHERE Drop_role_priv LIKE '%';
> >> +ALTER TABLE user ADD Drop_role_priv enum('N','Y') COLLATE utf8_general_ci
> DEFAULT 'N' NOT NULL AFTER Create_role_priv;
> >> +ALTER TABLE user MODIFY Drop_role_priv enum('N','Y') COLLATE
> utf8_general_ci DEFAULT 'N' NOT NULL AFTER Create_role_priv;
> >> +UPDATE user SET Drop_role_priv=Create_user_priv WHERE @hadDropRolePriv =
> 0;
> >
> > Why do you handle Drop_role_priv separetely instead of adding it in the same
> > ALTER TABLE that adds Create_role_priv ?
>
> Thought it may help reading and understanding script.
I don't think it does. And MODIFY certainly doesn't - it's completely
pointless and thus only confusing.
Write as
SET @hadCreateRolePriv := 0;
SELECT @hadCreateRolePriv :=1 FROM user WHERE Create_role_priv LIKE '%';
ALTER TABLE user ADD Create_role_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N'
NOT NULL AFTER Trigger_priv,
ADD Drop_role_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT
NULL AFTER Create_role_priv;
UPDATE user SET Create_role_priv=Create_user_priv,
Drop_role_priv=Create_user_priv WHERE @hadCreateRolePriv = 0;
> >> === modified file 'sql/sql_class.h'
> >> --- sql/sql_class.h 2008-06-28 11:00:59 +0000
> >> +++ sql/sql_class.h 2008-07-21 21:13:34 +0000
> >> @@ -828,14 +828,17 @@ public:
> >> the connection
> >> priv_user - The user privilege we are using. May be "" for anonymous
> user.
> >> ip - client IP
> >> + role - current user role
> >> */
> >> - char *host, *user, *priv_user, *ip;
> >> + char *host, *user, *priv_user, *ip, *role, *priv_role;
> >> /* The host privilege we are using */
> >> char priv_host[MAX_HOSTNAME];
> >> /* points to host if host is available, otherwise points to ip */
> >> const char *host_or_ip;
> >> - ulong master_access; /* Global privileges from mysql.user
> */
> >> - ulong db_access; /* Privileges for current db */
> >> + ulonglong master_access; /* Global mixed privileges for
> current user an role from mysql.user */
> >
> > not "mixed" but "combined" or "merged"
>
> Accepted.
>
> >> + ulonglong db_access; /* Privileges for current db */
> >
> > also combined ?
>
> Yes, of course.
I mean, the comment should mention it
> >> === modified file 'sql/sql_parse.cc'
> >> --- sql/sql_parse.cc 2008-06-28 11:00:59 +0000
> >> +++ sql/sql_parse.cc 2008-07-21 21:13:34 +0000
> >> @@ -5057,11 +5093,11 @@ check_access(THD *thd, ulong want_access
> >> The effective privileges are the union of the global privileges
> >> and the the intersection of db- and host-privileges.
> >> */
> >> - *save_priv=sctx->master_access | db_access;
> >> + *save_priv= sctx->master_access | db_access;
> >> DBUG_RETURN(FALSE);
> >> }
> >> - if (((want_access & ~sctx->master_access) & ~DB_ACLS) ||
> >> - ! db && dont_check_global_grants)
> >> + if (((want_access & ~sctx->master_access) & ~(DB_ACLS |
> EXTRA_ACL)) ||
> >> + db && dont_check_global_grants)
> >
> > Why ?
>
> Actually, I don't remember this at all O_O. What original version did
> you use for review?
It's in my copy of your branch.
revid:dotsent@dot-sent-20080725202734-y3im7gyxcdowqab6
> >> { // We can never grant this
> >> DBUG_PRINT("error",("No possible access"));
> >> if (!no_errors)
> >> === modified file 'sql/sql_acl.h'
> >> --- sql/sql_acl.h 2008-03-28 13:33:31 +0000
> >> +++ sql/sql_acl.h 2008-07-21 21:13:34 +0000
> >> @@ -188,13 +191,25 @@ public:
> >> };
> >>
> >>
> >> +class ACL_ROLE:public ACL_ACCESS
> >> +{
> >> + public:
> >> + char* role;
> >> + /* Shall be used in implementation of role-to-role assignment */
> >
> > you mean it's not used yet ? or just forgot to remove the comment ? :)
>
> Not used _yet_. As strict timeline prevents me from implementing
> role-to-role assignment in the scope of GSoC project, I think I'll
> continue working on it and implement this.
Ah, right. Sorry. I saw that you use assigned_roles[], that's why I
wrote "forgot to remove the comment?". But now I realize that what I saw
was ACL_USER.assigned_roles[], not ACL_ROLE.assigned_roles[].
> >> + DYNAMIC_ARRAY assigned_roles;
> >> +};
> >> +
> >> +
> >> class ACL_USER :public ACL_ACCESS
> >> {
> >> public:
> >> acl_host_and_ip host;
> >> uint hostname_length;
> >> USER_RESOURCES user_resource;
> >> - char *user;
> >> + char *user, *default_role_name;
> >> + ACL_ROLE *default_role, *current_role;
> >
> > why do you need default_role_name ? Isn't it the same as
> > default_role->role ?
>
> Data from mysql.users table is loaded before data from mysql.roles table and
> assigning of assigned_roles array, default_role and current_role members is done
> after entire mysql.users table has already been loaded in acl_users array. So we
> need a string variable to store default role name for each user in acl_users.
Yeah, I saw that. I'd prefer a cleaner solution, but I'm ok with yours.
Don't forget to set default_role_name=0 when you assign default_role.
default_role_name is allocated in THD's memroot (which is freed at the
end of the statement) so you'll have a dangling pointer.
> >> + DYNAMIC_ARRAY assigned_roles;
> >> +// bool is_role;
> >> uint8 salt[SCRAMBLE_LENGTH+1]; // scrambled password in binary
> form
> >> uint8 salt_len; // 0 - no password, 4 - 3.20, 8 - 3.23, 20 -
> 4.1.1
> >> enum SSL_type ssl_type;
> >> === modified file 'sql/sql_acl.cc'
> >> --- sql/sql_acl.cc 2008-06-28 11:00:59 +0000
> >> +++ sql/sql_acl.cc 2008-07-25 20:27:34 +0000
> >> @@ -619,6 +716,77 @@ static my_bool acl_load(THD *thd, TABLE_
> >> + if (!strcmp(user, acl_user_tmp->user))
> >> + {
> >> + if (host && &acl_user_tmp->host)
> >
> > what does that mean ?
>
> null-check. Had server crash there sometimes.
Yes, host could be NULL. But &acl_user_tmp->host cannot.
> >> @@ -1363,10 +1545,18 @@ ulong acl_get(const char *host, const ch
> >> + else if (!acl_db->user)
> >> + {
> >> + for (uint j=0 ; j < acl_users.elements ; j++)
> >> + {
> >> + ACL_USER *acl_user= dynamic_element(&acl_users,j,ACL_USER*);
> >> + if (strcmp(acl_user->current_role->role, (char*) "NONE")
> && !strcmp(acl_user->current_role->role, acl_db->user))
> >> + {
> >> + db_access|= acl_db->access;
> >> }
> >
> > I don't understand this loop at all.
>
> sctx->db_access must contain composition of user's and current_role's
> privileges on current table, so we have to get both privileges. First
> is assigned by exisitng code, this loop does the second part (at
> least, it was supposed to).
I don't think it should work that way. Instead, you have current role
(in the active Security_context), get its privileges for the db and
merge. E.g. restore acl_get to its original state, and
acl_get_with_role(const char *host, const char *ip,
const char *user,const char *role,
const char *db, my_bool db_is_pattern)
{
return acl_get(host,ip,user,db,db_is_pattern) |
acl_get(0,0,role,db,db_is_pattern);
}
although you typically will need to set both Security_context::db_access
and true_user_db_access, so you do it as
sctx->true_user_db_access= acl_get(host,ip,user,db,db_is_pattern);
sctx->db_access= sctx->true_user_db_access |
acl_get(0,0,rule,db,db_is_pattern);
and here you don't need acl_get_with_role() at all.
> >> }
> >> }
> >> @@ -3477,6 +3669,146 @@ bool mysql_grant(THD *thd, const char *d
> >> }
> >>
> >>
> >> +bool mysql_grant_role(THD *thd, List <LEX_USER> &list,
> >> + bool revoke_grant)
> >> +{
> >> + List_iterator <LEX_USER> str_list (list);
> >> + /* Skipping and saving first record in list as it contains desired
> role*/
> >> + LEX_USER *Role= str_list++;
> >> + LEX_USER *Str, *tmp_Str;
> >
> > as you have noticed, we use lowercase in variable names
> >
>
> OK, will change that. Anyway, mysql_grant looks the same. Should I
> correct it, too?
feel free to
> >> + TABLE_LIST tables[2];
> >> + TABLE *table;
> >> + DBUG_ENTER("mysql_grant_role");
> >> + if (!initialized)
> >> + {
> >> + my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0),
> >> + "--skip-grant-tables");
> >> + DBUG_RETURN(TRUE);
> >> + }
............
> >> + for (uint j=0 ; j < acl_user_tmp->assigned_roles.elements ;
> j++)
> >> + {
> >> + ACL_ROLE *acl_role_tmp=
> dynamic_element(&acl_user_tmp->assigned_roles, j, ACL_ROLE*);
> >> + if (!strcmp(acl_role_tmp->role,Role->user.str))
> >> + result=-1;
> >
> > where's "break;" ?
> > alternatively, you may skip the check for duplicate grants above,
> > and let ha_write_row() below fail with duplicate key.
> >
> > on the other hand, you *must* verify that the role exists before
> > writing its name to the table.
>
> OK, will add a break. About verification of existence: what about a
> feature like auto-creating
> user in case of GRANTing privileges to him?
Right, a user can be auto-created. But not a role when you grant it. I
mean in
GRANT roleA TO userB;
userB can be created if it does not exists. But if roleA doesn't exist
it should not be created, it's an error.
Regards / Mit vielen Grüssen,
Sergei
--
__ ___ ___ ____ __
/ |/ /_ __/ __/ __ \/ / Sergei Golubchik <serg@stripped>
/ /|_/ / // /\ \/ /_/ / /__ Principal Software Engineer/Server Architect
/_/ /_/\_, /___/\___\_\___/ Sun Microsystems GmbH, HRB München 161028
<___/ Sonnenallee 1, 85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schroeder, Wolfgang Engels, Dr. Roland Boemer
Vorsitzender des Aufsichtsrates: Martin Häring
