Re: alter user/role CURRENT_USER
| От | Adam Brightwell |
|---|---|
| Тема | Re: alter user/role CURRENT_USER |
| Дата | |
| Msg-id | CAKRt6CTCfEXHJtqVR79jaJ++e9Ei+bVGQDhq-mM8ObKg4fghFA@mail.gmail.com обсуждение исходный текст |
| Ответ на | Re: alter user/role CURRENT_USER (Rushabh Lathia <rushabh.lathia@gmail.com>) |
| Список | pgsql-hackers |
All,
I just ran through the patch giving it a good once over, some items to address/consider/discuss:
* Trailing whitespace.
* Why are you making changes in foreigncmds.c? These seem like unnecessary changes. I see that you are trying to consolidate but this refactor seems potentially out of scope.
* To the above point, is ResolveRoleId really necessary? Also, I'm not sure I agree with passing in the tuple, I don't see any reason to pull that look up into this function. Also, couldn't you simply just add a short circuit in "get_role_oid" to check for "current_user" and return GetUserId() and similar for "session_user"?
* In gram.y, is there really a need to have a separate RoleId_or_curruser? Why not:
-RoleId: NonReservedWord { $$ = $1; };
+RoleId: CURRENT_USER { $$ = "current_user";}
+ | USER { $$ = "current_user";}
+ | CURRENT_ROLE { $$ = "current_user";}
+ | SESSION_USER { $$ = "session_user";}
+ | NonReservedWord { $$ = $1; }
+ ;
This would certainly reduce the number of changes to the grammar but might also help with covering the cases mentioned by Rushabh? Also are there cases when we don't want CURRENT_USER to be considered a RoleId?
* The following seems like an unnecessary change:
- | RoleId { $$ = (strcmp($1, "public") == 0) ? NULL : $1; }
+ RoleId { $$ = (strcmp($1, "public") == 0) ?
+ NULL : $1; }
* Why is htup.h included in dbcommands.h?
* What's up with the following in user.c, did you replace tab with spaces?
- HeapTuple roletuple;
+ HeapTuple roletuple;
-- Not working
alter default privileges for role current_user grant SELECT ON TABLES TO current_user ;
-- Not working
grant user1 TO current_user;
Agreed. The latter of the two seems like an interesting case to me though. But they both kind of jump out at me as potential for security concerns (but perhaps not given the role id checks, etc). At any rate, I'd still expect these to behave accordingly with notification/error messages when appropriate.
Their might few more syntax like this.
I think this can be "covered" inherently by the grammar changes recommended above (if such changes are appropriate). Though, I'd also recommend investigating which commands are affected via the grammar (or RoleId) and then making sure to update the regression tests and the documentation accordingly.
I understand that patch is sightly getting bigger and complex then what it was
originally proposed.
IMHO, I think this patch has become more complex than is required.
Before going back to more review on latest patch I would
like to understand the requirement of this new feature. Also would like others
to comment on where/how we should restrict this feature ?
I think this is a fair request. I believe I can see the potential convenience of these changes, however, I'm uncertain of the necessity of them and whether or not it opens any security concerns (again, perhaps not, but I think it is worth asking). Also, how would this affect items like auditing?
-Adam
Adam Brightwell - adam.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
В списке pgsql-hackers по дате отправления: