table partitioning and access privileges

Fujii Masao <masao.fujii@gmail.com> writes:

My customer reported me that the queries through a partitioned table
ignore each partition's SELECT, INSERT, UPDATE, and DELETE privileges,
on the other hand, only TRUNCATE privilege specified for each partition
is applied. I'm not sure if this behavior is expected or not. But anyway
is it better to document that? For example,

Access privileges may be defined and removed separately for each partition.
But note that queries through a partitioned table ignore each partition's
SELECT, INSERT, UPDATE and DELETE privileges, and apply only TRUNCATE one.

I believe it's intentional that we only check access privileges on
the table explicitly named in the query. So I'd say SELECT etc
are doing the right thing, and if TRUNCATE isn't in step with them
that's a bug to fix, not something to document.

regards, tom lane

On Fri, Dec 27, 2019 at 4:26 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Fujii Masao <masao.fujii@gmail.com> writes:

My customer reported me that the queries through a partitioned table
ignore each partition's SELECT, INSERT, UPDATE, and DELETE privileges,
on the other hand, only TRUNCATE privilege specified for each partition
is applied. I'm not sure if this behavior is expected or not. But anyway
is it better to document that? For example,

Access privileges may be defined and removed separately for each partition.
But note that queries through a partitioned table ignore each partition's
SELECT, INSERT, UPDATE and DELETE privileges, and apply only TRUNCATE one.

I believe it's intentional that we only check access privileges on
the table explicitly named in the query. So I'd say SELECT etc
are doing the right thing, and if TRUNCATE isn't in step with them
that's a bug to fix, not something to document.

I tend to agree that TRUNCATE's permission model for inheritance
should be consistent with that for the other commands. How about the
attached patch toward that end?

Thanks,
Amit

On Tue, Jan 7, 2020 at 5:15 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Fri, Dec 27, 2019 at 4:26 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Fujii Masao <masao.fujii@gmail.com> writes:

My customer reported me that the queries through a partitioned table
ignore each partition's SELECT, INSERT, UPDATE, and DELETE privileges,
on the other hand, only TRUNCATE privilege specified for each partition
is applied. I'm not sure if this behavior is expected or not. But anyway
is it better to document that? For example,

Access privileges may be defined and removed separately for each partition.
But note that queries through a partitioned table ignore each partition's
SELECT, INSERT, UPDATE and DELETE privileges, and apply only TRUNCATE one.

I believe it's intentional that we only check access privileges on
the table explicitly named in the query. So I'd say SELECT etc
are doing the right thing, and if TRUNCATE isn't in step with them
that's a bug to fix, not something to document.

I tend to agree that TRUNCATE's permission model for inheritance
should be consistent with that for the other commands. How about the
attached patch toward that end?

Thanks for the patch!

The patch basically looks good to me.

+GRANT SELECT (f1, fz), UPDATE (fz) ON atestc TO regress_priv_user2;
+REVOKE TRUNCATE ON atestc FROM regress_priv_user2;

These seem not to be necessary for the test.

BTW, I found that LOCK TABLE on the parent table checks the permission
of its child tables. This also needs to be fixed (as a separate patch)?

Regards,

--
Fujii Masao

Fujii-san,

Thanks for taking a look.

On Fri, Jan 10, 2020 at 10:29 AM Fujii Masao <masao.fujii@gmail.com> wrote:

On Tue, Jan 7, 2020 at 5:15 PM Amit Langote <amitlangote09@gmail.com> wrote:

I tend to agree that TRUNCATE's permission model for inheritance
should be consistent with that for the other commands. How about the
attached patch toward that end?

Thanks for the patch!

The patch basically looks good to me.

+GRANT SELECT (f1, fz), UPDATE (fz) ON atestc TO regress_priv_user2;
+REVOKE TRUNCATE ON atestc FROM regress_priv_user2;

These seem not to be necessary for the test.

You're right. Removed in the attached updated patch.

BTW, I found that LOCK TABLE on the parent table checks the permission
of its child tables. This also needs to be fixed (as a separate patch)?

Commit ac33c7e2c13 and a past discussion ([1]https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=ac33c7e2c13, [2]/messages/by-id/34d269d40905121340h535ef652kbf8f054811e42e39@mail.gmail.com, resp.) appear to
disagree with that position, but I would like to agree with you
because the behavior you suggest would be consistent with other
commands. So, I'm attaching a patch for that too, although it would
be better to hear more opinions before accepting it.

Thanks,
Amit

[1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=ac33c7e2c13
[2]: /messages/by-id/34d269d40905121340h535ef652kbf8f054811e42e39@mail.gmail.com

On 2020/01/22 16:54, Amit Langote wrote:

Fujii-san,

Thanks for taking a look.

On Fri, Jan 10, 2020 at 10:29 AM Fujii Masao <masao.fujii@gmail.com> wrote:

On Tue, Jan 7, 2020 at 5:15 PM Amit Langote <amitlangote09@gmail.com> wrote:

I tend to agree that TRUNCATE's permission model for inheritance
should be consistent with that for the other commands. How about the
attached patch toward that end?

Thanks for the patch!

The patch basically looks good to me.

+GRANT SELECT (f1, fz), UPDATE (fz) ON atestc TO regress_priv_user2;
+REVOKE TRUNCATE ON atestc FROM regress_priv_user2;

These seem not to be necessary for the test.

You're right. Removed in the attached updated patch.

Thanks for updating the patch! Barring any objection,
I will commit this fix and backport it to all supported versions.

BTW, I found that LOCK TABLE on the parent table checks the permission
of its child tables. This also needs to be fixed (as a separate patch)?

Commit ac33c7e2c13 and a past discussion ([1], [2], resp.) appear to
disagree with that position, but I would like to agree with you
because the behavior you suggest would be consistent with other
commands. So, I'm attaching a patch for that too, although it would
be better to hear more opinions before accepting it.

Yes. I'd like to hear more opinion about this. But
since the document explains "Inherited queries perform access
permission checks on the parent table only." in ddl.sgml,
that also seems a bug to fix...

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

On 2020/01/23 22:14, Fujii Masao wrote:

On 2020/01/22 16:54, Amit Langote wrote:

Fujii-san,

Thanks for taking a look.

On Fri, Jan 10, 2020 at 10:29 AM Fujii Masao <masao.fujii@gmail.com>
wrote:

On Tue, Jan 7, 2020 at 5:15 PM Amit Langote <amitlangote09@gmail.com>
wrote:

I tend to agree that TRUNCATE's permission model for inheritance
should be consistent with that for the other commands.  How about the
attached patch toward that end?

Thanks for the patch!

The patch basically looks good to me.

+GRANT SELECT (f1, fz), UPDATE (fz) ON atestc TO regress_priv_user2;
+REVOKE TRUNCATE ON atestc FROM regress_priv_user2;

These seem not to be necessary for the test.

You're right.  Removed in the attached updated patch.

Thanks for updating the patch! Barring any objection,
I will commit this fix and backport it to all supported versions.

Attached are the back-port versions of the patches.

- patch for master and v12

0001-Don-t-check-child-s-TRUNCATE-privilege-when-truncate-fujii-pg12-13.patch

- patch for v11

0001-Don-t-check-child-s-TRUNCATE-privilege-when-truncate-fujii-pg11.patch

- patch for v10

0001-Don-t-check-child-s-TRUNCATE-privilege-when-truncate-fujii-pg10.patch

- patch for v9.6

0001-Don-t-check-child-s-TRUNCATE-privilege-when-truncate-fujii-pg96.patch

- patch for v9.5 and v9.4

0001-Don-t-check-child-s-TRUNCATE-privilege-when-truncate-fujii-pg94-95.patch

The patch for master branch separates truncate_check_activity() into two
functions, but in v11 or before, truncate_check_activity() didn't exist and
its code was in truncate_check_rel(). So I had to write the back-port
version
of the patch for the previous versions and separate truncate_check_rel()
into three functions, i.e., truncate_check_rel(),
truncate_check_activity() and
truncate_check_perms().

Also the names of users that the regression test for privileges use were
different between PostgreSQL versions. This is another reason
why I had to write several back-port versions of the patches.

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

Fujii-san,

On Mon, Jan 27, 2020 at 11:19 AM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:

On 2020/01/23 22:14, Fujii Masao wrote:

Thanks for updating the patch! Barring any objection,
I will commit this fix and backport it to all supported versions.

Attached are the back-port versions of the patches.

The patch for master branch separates truncate_check_activity() into two
functions, but in v11 or before, truncate_check_activity() didn't exist and
its code was in truncate_check_rel(). So I had to write the back-port
version
of the patch for the previous versions and separate truncate_check_rel()
into three functions, i.e., truncate_check_rel(),
truncate_check_activity() and
truncate_check_perms().

Thank you for creating the back-port versions. I agree with making
the code look similar in all supported branches for the ease of future
maintenance.

Thanks,
Amit

On 2020/01/27 14:02, Amit Langote wrote:

Fujii-san,

On Mon, Jan 27, 2020 at 11:19 AM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:

On 2020/01/23 22:14, Fujii Masao wrote:

Thanks for updating the patch! Barring any objection,
I will commit this fix and backport it to all supported versions.

Attached are the back-port versions of the patches.

The patch for master branch separates truncate_check_activity() into two
functions, but in v11 or before, truncate_check_activity() didn't exist and
its code was in truncate_check_rel(). So I had to write the back-port
version
of the patch for the previous versions and separate truncate_check_rel()
into three functions, i.e., truncate_check_rel(),
truncate_check_activity() and
truncate_check_perms().

Thank you for creating the back-port versions. I agree with making
the code look similar in all supported branches for the ease of future
maintenance.

Thanks for the check! I pushed the patches.

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

Fujii Masao <masao.fujii@oss.nttdata.com> writes:

Thanks for updating the patch! Barring any objection,
I will commit this fix and backport it to all supported versions.

Sorry for not having paid closer attention to this thread, but ...
is back-patching this behavioral change really a good idea?

It's not that hard to imagine that somebody is expecting the old
behavior and will complain that we broke their application's security.
So I'd have thought it better to fix only in HEAD, with a
compatibility warning in the v13 release notes.

I'm afraid it's much more likely that people will complain about
making such a change in a minor release than that they will be
happy about it. It's particularly risky to be making it in what
will be the last 9.4.x release, because we will not have any
opportunity to undo it in that branch if there is pushback.

regards, tom lane

On 2020/01/31 1:02, Tom Lane wrote:

Fujii Masao <masao.fujii@oss.nttdata.com> writes:

Thanks for updating the patch! Barring any objection,
I will commit this fix and backport it to all supported versions.

Sorry for not having paid closer attention to this thread, but ...
is back-patching this behavioral change really a good idea?

It's not that hard to imagine that somebody is expecting the old
behavior and will complain that we broke their application's security.
So I'd have thought it better to fix only in HEAD, with a
compatibility warning in the v13 release notes.

I'm afraid it's much more likely that people will complain about
making such a change in a minor release than that they will be
happy about it. It's particularly risky to be making it in what
will be the last 9.4.x release, because we will not have any
opportunity to undo it in that branch if there is pushback.

Fair enough. I finally did back-patch because the behavior is clearly
documented and I failed to hear the opinions to object the back-patch.
But I should have heard and discussed such risks more.

I'm OK to revert all those back-patch. Instead, probably the document
should be updated in old branches.

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

On 2020/01/31 1:28, Fujii Masao wrote:

On 2020/01/31 1:02, Tom Lane wrote:

Fujii Masao <masao.fujii@oss.nttdata.com> writes:

Thanks for updating the patch! Barring any objection,
I will commit this fix and backport it to all supported versions.

Sorry for not having paid closer attention to this thread, but ...
is back-patching this behavioral change really a good idea?

It's not that hard to imagine that somebody is expecting the old
behavior and will complain that we broke their application's security.
So I'd have thought it better to fix only in HEAD, with a
compatibility warning in the v13 release notes.

I'm afraid it's much more likely that people will complain about
making such a change in a minor release than that they will be
happy about it.  It's particularly risky to be making it in what
will be the last 9.4.x release, because we will not have any
opportunity to undo it in that branch if there is pushback.

Fair enough. I finally did back-patch because the behavior is clearly
documented and I failed to hear the opinions to object the back-patch.
But I should have heard and discussed such risks more.

I'm OK to revert all those back-patch. Instead, probably the document
should be updated in old branches.

I'm thinking to wait at least half a day before reverting
the back-patch just in case someone can give opinion
during that period.

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

Fujii Masao <masao.fujii@oss.nttdata.com> writes:

On 2020/01/31 1:28, Fujii Masao wrote:

On 2020/01/31 1:02, Tom Lane wrote:

Sorry for not having paid closer attention to this thread, but ...
is back-patching this behavioral change really a good idea?

I'm thinking to wait at least half a day before reverting
the back-patch just in case someone can give opinion
during that period.

Sure, other opinions welcome. We still have a week before the
back-branch releases.

regards, tom lane

On Fri, Jan 31, 2020 at 1:28 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2020/01/31 1:02, Tom Lane wrote:

Fujii Masao <masao.fujii@oss.nttdata.com> writes:

Thanks for updating the patch! Barring any objection,
I will commit this fix and backport it to all supported versions.

Sorry for not having paid closer attention to this thread, but ...
is back-patching this behavioral change really a good idea?

It's not that hard to imagine that somebody is expecting the old
behavior and will complain that we broke their application's security.
So I'd have thought it better to fix only in HEAD, with a
compatibility warning in the v13 release notes.

I'm afraid it's much more likely that people will complain about
making such a change in a minor release than that they will be
happy about it. It's particularly risky to be making it in what
will be the last 9.4.x release, because we will not have any
opportunity to undo it in that branch if there is pushback.

Fair enough. I finally did back-patch because the behavior is clearly
documented and I failed to hear the opinions to object the back-patch.
But I should have heard and discussed such risks more.

I'm OK to revert all those back-patch. Instead, probably the document
should be updated in old branches.

I could find only this paragraph in the section on inheritance that
talks about how access permissions work:

9.4:

"Note how table access permissions are handled. Querying a parent
table can automatically access data in child tables without further
access privilege checking. This preserves the appearance that the data
is (also) in the parent table. Accessing the child tables directly is,
however, not automatically allowed and would require further
privileges to be granted."

9.5-12:

"Inherited queries perform access permission checks on the parent
table only. Thus, for example, granting UPDATE permission on the
cities table implies permission to update rows in the capitals table
as well, when they are accessed through cities. This preserves the
appearance that the data is (also) in the parent table. But the
capitals table could not be updated directly without an additional
grant. In a similar way, the parent table's row security policies (see
Section 5.7) are applied to rows coming from child tables during an
inherited query. A child table's policies, if any, are applied only
when it is the table explicitly named in the query; and in that case,
any policies attached to its parent(s) are ignored."

Do you mean that the TRUNCATE exception should be noted here?

Thanks,
Amit

On 2020/01/31 13:38, Amit Langote wrote:

On Fri, Jan 31, 2020 at 1:28 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2020/01/31 1:02, Tom Lane wrote:

Fujii Masao <masao.fujii@oss.nttdata.com> writes:

Thanks for updating the patch! Barring any objection,
I will commit this fix and backport it to all supported versions.

Sorry for not having paid closer attention to this thread, but ...
is back-patching this behavioral change really a good idea?

It's not that hard to imagine that somebody is expecting the old
behavior and will complain that we broke their application's security.
So I'd have thought it better to fix only in HEAD, with a
compatibility warning in the v13 release notes.

I'm afraid it's much more likely that people will complain about
making such a change in a minor release than that they will be
happy about it. It's particularly risky to be making it in what
will be the last 9.4.x release, because we will not have any
opportunity to undo it in that branch if there is pushback.

Fair enough. I finally did back-patch because the behavior is clearly
documented and I failed to hear the opinions to object the back-patch.
But I should have heard and discussed such risks more.

I'm OK to revert all those back-patch. Instead, probably the document
should be updated in old branches.

I could find only this paragraph in the section on inheritance that
talks about how access permissions work:

9.4:

"Note how table access permissions are handled. Querying a parent
table can automatically access data in child tables without further
access privilege checking. This preserves the appearance that the data
is (also) in the parent table. Accessing the child tables directly is,
however, not automatically allowed and would require further
privileges to be granted."

9.5-12:

"Inherited queries perform access permission checks on the parent
table only. Thus, for example, granting UPDATE permission on the
cities table implies permission to update rows in the capitals table
as well, when they are accessed through cities. This preserves the
appearance that the data is (also) in the parent table. But the
capitals table could not be updated directly without an additional
grant. In a similar way, the parent table's row security policies (see
Section 5.7) are applied to rows coming from child tables during an
inherited query. A child table's policies, if any, are applied only
when it is the table explicitly named in the query; and in that case,
any policies attached to its parent(s) are ignored."

Do you mean that the TRUNCATE exception should be noted here?

Yes, that's what I was thinking.

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

On Fri, Jan 31, 2020 at 9:39 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2020/01/31 13:38, Amit Langote wrote:

On Fri, Jan 31, 2020 at 1:28 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

Fair enough. I finally did back-patch because the behavior is clearly
documented and I failed to hear the opinions to object the back-patch.
But I should have heard and discussed such risks more.

I'm OK to revert all those back-patch. Instead, probably the document
should be updated in old branches.

I could find only this paragraph in the section on inheritance that
talks about how access permissions work:

9.4:

"Note how table access permissions are handled. Querying a parent
table can automatically access data in child tables without further
access privilege checking. This preserves the appearance that the data
is (also) in the parent table. Accessing the child tables directly is,
however, not automatically allowed and would require further
privileges to be granted."

9.5-12:

"Inherited queries perform access permission checks on the parent
table only. Thus, for example, granting UPDATE permission on the
cities table implies permission to update rows in the capitals table
as well, when they are accessed through cities. This preserves the
appearance that the data is (also) in the parent table. But the
capitals table could not be updated directly without an additional
grant. In a similar way, the parent table's row security policies (see
Section 5.7) are applied to rows coming from child tables during an
inherited query. A child table's policies, if any, are applied only
when it is the table explicitly named in the query; and in that case,
any policies attached to its parent(s) are ignored."

Do you mean that the TRUNCATE exception should be noted here?

Yes, that's what I was thinking.

Okay. How about the attached?

Maybe, we should also note the LOCK TABLE exception?

Regards,
Amit

On 2020/02/03 11:05, Amit Langote wrote:

On Fri, Jan 31, 2020 at 9:39 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2020/01/31 13:38, Amit Langote wrote:

On Fri, Jan 31, 2020 at 1:28 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

Fair enough. I finally did back-patch because the behavior is clearly
documented and I failed to hear the opinions to object the back-patch.
But I should have heard and discussed such risks more.

I'm OK to revert all those back-patch. Instead, probably the document
should be updated in old branches.

I could find only this paragraph in the section on inheritance that
talks about how access permissions work:

9.4:

"Note how table access permissions are handled. Querying a parent
table can automatically access data in child tables without further
access privilege checking. This preserves the appearance that the data
is (also) in the parent table. Accessing the child tables directly is,
however, not automatically allowed and would require further
privileges to be granted."

9.5-12:

"Inherited queries perform access permission checks on the parent
table only. Thus, for example, granting UPDATE permission on the
cities table implies permission to update rows in the capitals table
as well, when they are accessed through cities. This preserves the
appearance that the data is (also) in the parent table. But the
capitals table could not be updated directly without an additional
grant. In a similar way, the parent table's row security policies (see
Section 5.7) are applied to rows coming from child tables during an
inherited query. A child table's policies, if any, are applied only
when it is the table explicitly named in the query; and in that case,
any policies attached to its parent(s) are ignored."

Do you mean that the TRUNCATE exception should be noted here?

Yes, that's what I was thinking.

Okay. How about the attached?

Thanks for the patches! You added the note just after the description
about row level security on inherited table, but isn't it better to
add it before that? Attached patch does that. Thought?

Maybe, we should also note the LOCK TABLE exception?

Yes.

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

On Mon, Feb 3, 2020 at 2:07 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2020/02/03 11:05, Amit Langote wrote:

Okay. How about the attached?

Thanks for the patches! You added the note just after the description
about row level security on inherited table, but isn't it better to
add it before that? Attached patch does that. Thought?

Yeah, that might be a better flow for that paragraph.

Maybe, we should also note the LOCK TABLE exception?

Yes.

Note that, unlike TRUNCATE, LOCK TABLE exception exists in HEAD too,
but maybe you're aware of that.

Thanks,
Amit

On 2020/02/03 14:26, Amit Langote wrote:

On Mon, Feb 3, 2020 at 2:07 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2020/02/03 11:05, Amit Langote wrote:

Okay. How about the attached?

Thanks for the patches! You added the note just after the description
about row level security on inherited table, but isn't it better to
add it before that? Attached patch does that. Thought?

Yeah, that might be a better flow for that paragraph.

Pushed! Thanks!

Maybe, we should also note the LOCK TABLE exception?

Yes.

Note that, unlike TRUNCATE, LOCK TABLE exception exists in HEAD too,
but maybe you're aware of that.

Yes, so I will review your patch getting rid of
LOCK TABLE exception.

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

On Fri, Feb 7, 2020 at 1:16 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2020/02/03 14:26, Amit Langote wrote:

On Mon, Feb 3, 2020 at 2:07 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2020/02/03 11:05, Amit Langote wrote:

Okay. How about the attached?

Thanks for the patches! You added the note just after the description
about row level security on inherited table, but isn't it better to
add it before that? Attached patch does that. Thought?

Yeah, that might be a better flow for that paragraph.

Pushed! Thanks!

Thank you.

Maybe, we should also note the LOCK TABLE exception?

Yes.

Note that, unlike TRUNCATE, LOCK TABLE exception exists in HEAD too,
but maybe you're aware of that.

Yes, so I will review your patch getting rid of
LOCK TABLE exception.

Attached updated patch.

Regards,
Amit