Re: Add has_large_object_privilege function

On Mon, 9 Sep 2024 22:59:34 +0900
Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)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(at)sraoss(dot)co(dot)jp>