Allow CURRENT_ROLE in GRANTED BY

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

Allow CURRENT_ROLE in GRANTED BY

Peter Eisentraut-6
I was checking some loose ends in SQL conformance, when I noticed: We
support GRANT role ... GRANTED BY CURRENT_USER, but we don't support
CURRENT_ROLE in that place, even though in PostgreSQL they are
equivalent.  Here is a trivial patch to add that.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

0001-Allow-CURRENT_ROLE-in-GRANTED-BY.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Allow CURRENT_ROLE in GRANTED BY

Vik Fearing-6
On 6/24/20 8:35 AM, Peter Eisentraut wrote:
> I was checking some loose ends in SQL conformance, when I noticed: We
> support GRANT role ... GRANTED BY CURRENT_USER, but we don't support
> CURRENT_ROLE in that place, even though in PostgreSQL they are
> equivalent.  Here is a trivial patch to add that.


The only thing that isn't dead-obvious about this patch is the commit
message says "[PATCH 1/2]".  What is in the other part?

Assuming that's just a remnant of development, this LGTM.
--
Vik Fearing


Reply | Threaded
Open this post in threaded view
|

Re: Allow CURRENT_ROLE in GRANTED BY

Peter Eisentraut-6
On 2020-06-24 10:12, Vik Fearing wrote:
> On 6/24/20 8:35 AM, Peter Eisentraut wrote:
>> I was checking some loose ends in SQL conformance, when I noticed: We
>> support GRANT role ... GRANTED BY CURRENT_USER, but we don't support
>> CURRENT_ROLE in that place, even though in PostgreSQL they are
>> equivalent.  Here is a trivial patch to add that.
>
>
> The only thing that isn't dead-obvious about this patch is the commit
> message says "[PATCH 1/2]".  What is in the other part?

Hehe.  The second patch is some in-progress work to add the GRANTED BY
clause to the regular GRANT command.  More on that perhaps at a later date.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Allow CURRENT_ROLE in GRANTED BY

Alvaro Herrera-9
In reply to this post by Peter Eisentraut-6
On 2020-Jun-24, Peter Eisentraut wrote:

> I was checking some loose ends in SQL conformance, when I noticed: We
> support GRANT role ... GRANTED BY CURRENT_USER, but we don't support
> CURRENT_ROLE in that place, even though in PostgreSQL they are equivalent.
> Here is a trivial patch to add that.

Hmm, since this adds to RoleSpec, this change makes every place that
uses that production also take CURRENT_ROLE, so we'd need to document in
all those places.  For example, alter_role.sgml, create_schema.sgml,
etc.

This also affects role_list (but maybe the docs for those are already
vague enough -- eg. ALTER INDEX .. OWNED BY only says "role_name" with
no further explanation, even though it does take "current_user".)

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Allow CURRENT_ROLE in GRANTED BY

Peter Eisentraut-6
On 2020-06-24 23:08, Alvaro Herrera wrote:

> On 2020-Jun-24, Peter Eisentraut wrote:
>
>> I was checking some loose ends in SQL conformance, when I noticed: We
>> support GRANT role ... GRANTED BY CURRENT_USER, but we don't support
>> CURRENT_ROLE in that place, even though in PostgreSQL they are equivalent.
>> Here is a trivial patch to add that.
>
> Hmm, since this adds to RoleSpec, this change makes every place that
> uses that production also take CURRENT_ROLE, so we'd need to document in
> all those places.  For example, alter_role.sgml, create_schema.sgml,
> etc.
Good point.  Here is an updated patch that updates all the documentation
places where CURRENT_USER is mentioned.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

v2-0001-Allow-CURRENT_ROLE-where-CURRENT_USER-is-accepted.patch (56K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Allow CURRENT_ROLE in GRANTED BY

Peter Eisentraut-6
On 2020-06-29 14:47, Peter Eisentraut wrote:

> On 2020-06-24 23:08, Alvaro Herrera wrote:
>> On 2020-Jun-24, Peter Eisentraut wrote:
>>
>>> I was checking some loose ends in SQL conformance, when I noticed: We
>>> support GRANT role ... GRANTED BY CURRENT_USER, but we don't support
>>> CURRENT_ROLE in that place, even though in PostgreSQL they are equivalent.
>>> Here is a trivial patch to add that.
>>
>> Hmm, since this adds to RoleSpec, this change makes every place that
>> uses that production also take CURRENT_ROLE, so we'd need to document in
>> all those places.  For example, alter_role.sgml, create_schema.sgml,
>> etc.
>
> Good point.  Here is an updated patch that updates all the documentation
> places where CURRENT_USER is mentioned.
Here is another patch that also makes comprehensive updates to the
rolenames tests under src/test/modules/unsafe_tests/.

I think this should now cover all the required ancillary changes.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

v3-0001-Allow-CURRENT_ROLE-where-CURRENT_USER-is-accepted.patch (156K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Allow CURRENT_ROLE in GRANTED BY

Asif Rehman
The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            tested, passed

The patch applies cleanly and looks fine to me. However wouldn't it be better to just map the CURRENT_ROLE to CURRENT_USER in backend grammar?

The new status of this patch is: Waiting on Author
Reply | Threaded
Open this post in threaded view
|

Re: Allow CURRENT_ROLE in GRANTED BY

Peter Eisentraut-6
On 2020-09-07 12:02, Asif Rehman wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:       tested, passed
> Spec compliant:           tested, passed
> Documentation:            tested, passed
>
> The patch applies cleanly and looks fine to me. However wouldn't it be better to just map the CURRENT_ROLE to CURRENT_USER in backend grammar?

Existing code treats them differently.  I think, given that the code is
already written, it is good to preserve what the user wrote.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Allow CURRENT_ROLE in GRANTED BY

Alvaro Herrera-9
In reply to this post by Peter Eisentraut-6
On 2020-Aug-26, Peter Eisentraut wrote:

> Here is another patch that also makes comprehensive updates to the rolenames
> tests under src/test/modules/unsafe_tests/.

Looks good to me.  You need to DROP ROLE "current_role" at the bottom of
rolenames.sql, though (as well as DROP OWNED BY, I suppose.)

> I think this should now cover all the required ancillary changes.

\o/

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Allow CURRENT_ROLE in GRANTED BY

Peter Eisentraut-6
On 2020-09-11 22:05, Alvaro Herrera wrote:

> On 2020-Aug-26, Peter Eisentraut wrote:
>
>> Here is another patch that also makes comprehensive updates to the rolenames
>> tests under src/test/modules/unsafe_tests/.
>
> Looks good to me.  You need to DROP ROLE "current_role" at the bottom of
> rolenames.sql, though (as well as DROP OWNED BY, I suppose.)
>
>> I think this should now cover all the required ancillary changes.
>
> \o/
>

committed


--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Allow CURRENT_ROLE in GRANTED BY

Tom Lane-2
Reply | Threaded
Open this post in threaded view
|

Re: Allow CURRENT_ROLE in GRANTED BY

Peter Eisentraut-6
In reply to this post by Peter Eisentraut-6
On 2020-06-24 20:21, Peter Eisentraut wrote:

> On 2020-06-24 10:12, Vik Fearing wrote:
>> On 6/24/20 8:35 AM, Peter Eisentraut wrote:
>>> I was checking some loose ends in SQL conformance, when I noticed: We
>>> support GRANT role ... GRANTED BY CURRENT_USER, but we don't support
>>> CURRENT_ROLE in that place, even though in PostgreSQL they are
>>> equivalent.  Here is a trivial patch to add that.
>>
>>
>> The only thing that isn't dead-obvious about this patch is the commit
>> message says "[PATCH 1/2]".  What is in the other part?
>
> Hehe.  The second patch is some in-progress work to add the GRANTED BY
> clause to the regular GRANT command.  More on that perhaps at a later date.
Here is the highly anticipated and quite underwhelming second part of
this patch set.

--
Peter Eisentraut
2ndQuadrant, an EDB company
https://www.2ndquadrant.com/

0001-Allow-GRANTED-BY-clause-in-normal-GRANT-and-REVOKE-s.patch (22K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Allow CURRENT_ROLE in GRANTED BY

Simon Riggs
On Thu, 10 Dec 2020 at 18:40, Peter Eisentraut
<[hidden email]> wrote:

>
> On 2020-06-24 20:21, Peter Eisentraut wrote:
> > On 2020-06-24 10:12, Vik Fearing wrote:
> >> On 6/24/20 8:35 AM, Peter Eisentraut wrote:
> >>> I was checking some loose ends in SQL conformance, when I noticed: We
> >>> support GRANT role ... GRANTED BY CURRENT_USER, but we don't support
> >>> CURRENT_ROLE in that place, even though in PostgreSQL they are
> >>> equivalent.  Here is a trivial patch to add that.
> >>
> >>
> >> The only thing that isn't dead-obvious about this patch is the commit
> >> message says "[PATCH 1/2]".  What is in the other part?
> >
> > Hehe.  The second patch is some in-progress work to add the GRANTED BY
> > clause to the regular GRANT command.  More on that perhaps at a later date.
>
> Here is the highly anticipated and quite underwhelming second part of
> this patch set.

Looks great, but no test to confirm it works. I would suggest adding a
test and committing directly since I don't see any cause for further
discussion.

--
Simon Riggs                http://www.EnterpriseDB.com/


Reply | Threaded
Open this post in threaded view
|

Re: Allow CURRENT_ROLE in GRANTED BY

Peter Eisentraut-6
On 2020-12-30 13:43, Simon Riggs wrote:

> On Thu, 10 Dec 2020 at 18:40, Peter Eisentraut
> <[hidden email]> wrote:
>>
>> On 2020-06-24 20:21, Peter Eisentraut wrote:
>>> On 2020-06-24 10:12, Vik Fearing wrote:
>>>> On 6/24/20 8:35 AM, Peter Eisentraut wrote:
>>>>> I was checking some loose ends in SQL conformance, when I noticed: We
>>>>> support GRANT role ... GRANTED BY CURRENT_USER, but we don't support
>>>>> CURRENT_ROLE in that place, even though in PostgreSQL they are
>>>>> equivalent.  Here is a trivial patch to add that.
>>>>
>>>>
>>>> The only thing that isn't dead-obvious about this patch is the commit
>>>> message says "[PATCH 1/2]".  What is in the other part?
>>>
>>> Hehe.  The second patch is some in-progress work to add the GRANTED BY
>>> clause to the regular GRANT command.  More on that perhaps at a later date.
>>
>> Here is the highly anticipated and quite underwhelming second part of
>> this patch set.
>
> Looks great, but no test to confirm it works. I would suggest adding a
> test and committing directly since I don't see any cause for further
> discussion.

Committed with some tests.  Thanks.

--
Peter Eisentraut
2ndQuadrant, an EDB company
https://www.2ndquadrant.com/