osohq / oso

Deprecated: See README
Apache License 2.0
3.47k stars 179 forks source link

Q: Filtering ManyToMany in Django authorize_model? #1621

Open Natureshadow opened 2 years ago

Natureshadow commented 2 years ago

Given the following models:

class Tag(Model):
    name = CharField()

class Article(Model):
    tags = ManyToManyField(Tag)

How would I write an Oso policy that allows access on a tag with a certain name being present on an article?

authorize_model(request, Article)

What I want to do is something like this:

allow(user, "GET", article) if
    article.tags.filter(name: "public").exists()

But calling methods for authorized models is not allowed.

gj commented 2 years ago

Hi @Natureshadow 👋

This would probably best be written as something like:

allow(_user: User, "GET", article: Article) if
  tag in article.tags and
  tag.name = "public";
Natureshadow commented 2 years ago
tag in article.tags and

Unfortunately, this yields

InvalidIteratorError: myapp.Tag.None is not iterable

The culprit probably being that the ManyToManyField is not handled properly:

QUERY: _tag_359 in <django.db.models.fields.related_descriptors.create_forward_many_to_many_manager.<locals>.ManyRelatedManager object at 0x7f1238889c90> TYPE `UNKNOWN`, BINDINGS: {}
gj commented 2 years ago

~Hm... is article.tags nullable? I.e., the null state would be article.tags: null instead of article.tags: []?~

Disregard the above; I was misreading your latest message.

class Tag(Model):
    name = CharField()

class Article(Model):
    tags = ManyToManyField(Tag)

Is Model here django_oso.models.AuthorizedModel?

Natureshadow commented 2 years ago

On Mon, Sep 12, 2022 at 08:20:04AM -0700, Gabe Jackson wrote:

Hm... is article.tags nullable? I.e., the null state would be article.tags: null instead of article.tags: []?

No. In fact, the error occurs both for a ManyToMany relation that has entries and for an empty relation.

gj commented 2 years ago

Hi @Natureshadow,

I edited that comment, but the edit may've not made it to your email inbox.

Natureshadow commented 2 years ago

Is Model here django_oso.models.AuthorizedModel?

No, it's not. Also, I am not using authorize_model here, but simply authorize on one single instance.

gj commented 2 years ago

No, it's not. Also, I am not using authorize_model here, but simply authorize on one single instance.

Sorry, I assumed from the title of the issue that you were using authorize_model. If you're calling authorize on a single Article instance, I'm surprised that the code you pasted earlier didn't work:

allow(user, "GET", article) if
    article.tags.filter(name: "public").exists();

Are you able to share a reproducible example with me?

Natureshadow commented 2 years ago

Sorry, I assumed from the title of the issue that you were using authorize_model. If you're calling authorize on a single Article instance, I'm surprised that the code you pasted earlier didn't work:

allow(user, "GET", article) if     article.tags.filter(name: "public").exists();

That works with authorize. But my goal is ultimately to use authorize_model. If I rewrite the policy as proposed by you, authorize stops working as well.

gj commented 2 years ago

Ah, okay. Unfortunately there isn't a great pattern right now for supporting both authorize and authorize_model with the same policy in the Django adapter.

You could manage two separate Oso policies loaded into two separate Oso instances and use one for "concrete" authorization decisions (authorize()) and one for authorizing collections using data filtering (authorize_model()).

That sounds a bit cumbersome / error-prone, so you might also consider exclusively using authorize_model, since that will give you good performance in the case where you want an authorization decision about a single resource and the case where you want to authorize a collection. If you go that route, getting an authorization decision for a concrete Article would involve calling authorize_model() and then applying a filter to the returned Django Q object to scope the query down to the specific Article in question.

Natureshadow commented 1 year ago

The (slightly more complex) instance of the problem is this policy here:

https://edugit.org/AlekSIS/official/AlekSIS-Core/-/blob/492-follow-up-view_persons_rule-not-usable-due-to-use-of-queryset_rules_filter/aleksis/core/policy/permissions.polar

Its goal is to glue Oso and Django permissions together.

It explodes both in line 16 an line 31, with auth.Group.None is not iterable and auth.Permission.None is not iterable, when using authorize_model.