V8-archive: Invalid message on public api "Unauthorized request"

[laravelbap]

Bug Report

I don't know if this is actual bug, but this gave me 2-3 hours of debugging to understand what was the issue :dagger: ;D

1. I created Collection "Product" with "Categories" Product -> M2M -> Categories.
2. I have added proper permission for product, product_categories relation.
3. When creating "Product with assigned categories" via public API i got the message "Unauthorized request". After debugging i found that i was missing "directus relation" permissions.

After changing code bellow i was uble to understand what was issue.

//\Directus\Permissions\Acl::enforceReadOnce
public function enforceReadOnce($collection)
    { 
        if (!$this->canReadOnce($collection)) {

            // Removing this if showed me actual message.
            if($this->isPublic()){
                throw new UnauthorizedException('Unauthorized request');
            }else{
                throw new Exception\ForbiddenCollectionReadException(
                    $collection
                );
            }
        }
    }

[bjgajjar]

@benhaynes - At first glance, I think we should remove the ACL check from the code base and place it into some middleware; which will check all the conditions at a once.

Currently, API is checking permission every time for every collection [Including the system as well as user-generated].

IMO; this can be resolved in 2 ways.

1. Give the permission to directus_relations [Which is not acceptable]
2. Stop checking the permissions of the core table; if the operation is executing on the user-generated collection and that collection has public permission. It seems like we can achieve this with the middleware.

Feel free to share other approaches. :)

[laravelbap]

So i should remove permission to directus_relations for Public? As this is unsafe?

[benhaynes]

This is more a question for the dev specialists... @rijkvanzanten and @WoLfulus

[rijkvanzanten]

Just as with directus_users and some others, I think we should set the permission for directus_relations to read by default.

[bjgajjar]

I think we should set the permission for directus_relations to read by default.

Do you mean to give it public access?

[rijkvanzanten]

No, we should make it the default for new roles. That combined with https://github.com/directus/app/issues/2346 should be enough to make this a little more obvious.

Strangely enough, I don't remember ever seeing this issue before from just the public role, while it sounds like something that should surface pretty often for people using the public role. Could it be that creating items has a different check from reading items?

[bjgajjar]

Could it be that creating items has a different check from reading items?

Yes. But giving the public permission to directus_relations table will resolve this issue.
So if you think to combine it with directus/app#2346 then please go ahead. I agree with you on that.

[bjgajjar]

@rijkvanzanten - 🔔 May I know the further step for this?

[rijkvanzanten]

How does it work when using the API through the rest endpoints currently? I don't remember ever having set the permission to directus_relation for the public role manually 🤔

[bjgajjar]

How does it work when using the API through the rest endpoints currently?

This issue will occur for those collections which have public permission but contain the relational field.

As we have to validate the relational payload too. And for that, the relation must be checked before the validation and that's the reason we need public permission for directus_relations collection otherwise we can go with the second approach from here too.

I don't remember ever having set the permission to directus_relation for the public role manually

Yes; agree. We implement the relational validation some time ago; that's why we didn't face this earlier.

[rijkvanzanten]

I'm a little confused: are you saying that it _works_ or _does not work_ in the /items endpoint right now when I _do not_ explicitly set read permissions for directus_relations?

[bjgajjar]

It'll work if we give the public-read permission on directus_relations collection.

[rijkvanzanten]

Everything is off by default for the public group, and I think we should keep it that way. Moving the permissions for directus_users, directus_files, and directus_relations out of the advanced hidden section and into the normal overview and adding a note in the docs about how you should take directus collections into account for public permissions should be enough.

I'd rather not have 1 collection be public unexpectedly by default while everything else is not

[benhaynes]

So should we create an App enhancement ticket for moving those system collections out of "Advanced" and then ref/closing this?

[rijkvanzanten]

Closed in favor of https://github.com/directus/app/issues/2346

[nathannau]

Something strange is when public user do a GET request with fields=..*, API don't throw an exception...