Re: Add has_large_object_privilege function

On Mon, 9 Sep 2024 22:59:34 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

> 
> 
> On 2024/07/02 16:34, Yugo NAGATA wrote:
> > So, I would like to propose to add
> > has_large_object_function for checking if a user has the privilege on a large
> > object.
> 
> +1

Thank you for your looking into this!
I've attached a updated patch.

> 
> BTW since we already have pg_largeobject, using has_largeobject_privilege might
> offer better consistency. However, I'm okay with the current name for now.
> Even after committing the patch, we can rename it if others prefer has_largeobject_privilege.

I was also wandering which is better, so I renamed it because it seems at least one person,
you, have an idea that has_largeobject_privilege might be better. If it is found that majority
prefer the previous name, I'll get it back.

> 
> As for 0001.patch, should we also remove the inclusion of "access/genam.h" and
> "access/htup_details.h" since they're no longer needed?

Removed.
 
> 
> > 0002 adds has_large_object_privilege function.There are three variations whose
> > arguments are combinations of large object OID with user name, user OID, or
> > implicit user (current_user).
> 
> As for 0002.patch, as the code in these three functions is mostly the same,
> it might be more efficient to create a common internal function and have
> the three functions call it for simplicity.

I made a new internal function "has_lo_priv_byid" that is called from these
functions.
 
> Here are other review comments for 0002.patch.
> 
> +      <row>
> +       <entry role="func_table_entry"><para role="func_signature">
> +        <indexterm>
> +         <primary>has_large_object_privilege</primary>
> 
> In the documentation, access privilege inquiry functions are listed
> alphabetically. So, this function's description should come right
> after has_language_privilege.

Fixed.

> 
> + * has_large_objec_privilege variants
> 
> Typo: s/objec/object

Fixed.

> 
> + *		The result is a boolean value: true if user has been granted
> + *		the indicated privilege or false if not.
> 
> The comment should clarify that NULL is returned if the specified
> large object doesn’t exist. For example,
> --------------
> The result is a boolean value: true if user has the indicated
> privilege, false if not, or NULL if object doesn't exist.
> --------------

Fixed.

> 
> +convert_large_object_priv_string(text *priv_text)
> 
> It would be better to use "priv_type_text" instead of "priv_text"
> for consistency with similar functions.
> 
> 
> +	static const priv_map parameter_priv_map[] = {
> +		{"SELECT", ACL_SELECT},
> +		{"UPDATE", ACL_UPDATE},
> 
> parameter_priv_map should be large_object_priv_map.

Fixed.

> Additionally, the entries for "WITH GRANT OPTION" should be included here.

Fixed.

> 
> +-- not-existing user
> +SELECT has_large_object_privilege(-99999, 1001, 'SELECT');	-- false
> + has_large_object_privilege
> +----------------------------
> + t
> +(1 row)
> 
> 
> The comment states the result should be false, but the actual result is true.
> One of them seems incorrect.

I misunderstood that has_table_privilege always returns false for not-existing user, 
but it was not correct.  Actually, it returns true if the privilege is granted to public. 
I removed this check from the test at last because I don't think it is important.

Regards,
Yugo Nagata

-- 
Yugo NAGATA <nagata@sraoss.co.jp>