Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LoadFilteredPolicy not filtered as expected #5

Closed
AAZANOVICH opened this issue Jun 15, 2022 · 10 comments
Closed

LoadFilteredPolicy not filtered as expected #5

AAZANOVICH opened this issue Jun 15, 2022 · 10 comments

Comments

@AAZANOVICH
Copy link

I'm not sure whether my expectations of this functionality are incorrect or it is a defect.

Expected: The following filter should return policies where one of the policy params is equal to "data1" AND that policy assigned to user "alice".

e.LoadFilteredPolicy(&pgxadapter.Filter{ P: [][]string{{"", "data1"}}, G: [][]string{{"alice"}}, })

Actual behavior: It will return all the policies where one of the policy params is equal to "data1" (doesn't matter whether it is assigned to "alice" or not).

SQL statement generated by this filter will look like:
SELECT "p_type", "v0", "v1", "v2", "v3", "v4", "v5" FROM <table_name_here> WHERE (p_type = $1 AND (v1 = $2 )) OR (p_type = $4 AND ((v0 = $5)))

Which basically will return all records which matches P filter OR G filter.

As result further call to enforcer.GetPolicy() will return policies which were not assigned to user.

@pckhoi
Copy link
Owner

pckhoi commented Jun 16, 2022

I think your expectation for the filter functionality is not accurate. LoadFilteredPolicy is not meant to drill down to the exact row that has what you want. But rather to just narrow down the list of policies to filter through. As long as subsequent Enforce call works as expected then this function works as expected.

You also misunderstand what the filter is doing. The filter is getting P policies that has "data1" as its second value, and G policies that has "alice" as its first value. Keep in mind that P policies and G policies are kept as separate rows. If you want this function to return a policy that is both a P policy and a G policy then it will just return nothing.

Why do do you want to use GetPolicy?

@AAZANOVICH
Copy link
Author

  1. i.e. I have lots of policies for different users, on api request I just want to load policies for specific user, since I don't want to load entire dataset for all users (make sense ?). With the behavior you described LoadFilteredPolicy basically become useless since it will load entire dataset.

2)Don't know how you came to that conclusion, I gave the query which adapter will produce, it is pretty clear to me how it works. I question the design if it is not a defect.

  1. i.e. I want to create an API which will return all policies assigned to user. So, my expectation I can just load policy for specific user LoadFilteredPolicy and then call getPolicy (for simplicity let's say I just reload policy completely, so there is no policies assigned to other users).

** To be honest at this point the only peace of library which I use is just enforce part, I had to write my own code to update/read etc policies etc... most APIs are just useless for real application.. no possibility save policy in a transaction with other updates.. even loading functionality is flawed, why I cannot just pass a pgx.Pool to the adapter and it should get connections from it ? instead it will create a new connection , which will miss runtime configuration - another defect..

@pckhoi
Copy link
Owner

pckhoi commented Jun 16, 2022

  1. Please try the query that you want on the PostgreSQL and tell me what it produces.
  2. You should try GetFilteredPolicy after LoadFilteredPolicy.

You completely misunderstood what LoadFilteredPolicy does. It is described in a section called Policy Subset Loading. It loads a subset, not a single row. If you are complaining about the design then this is the wrong place friend. You should complain with Casbin creators. This repo is just an adapter.

@AAZANOVICH
Copy link
Author

AAZANOVICH commented Jun 16, 2022

  1. I copied that query from my app and removed some data, in order to show the logic, I have a little bit different columns in my table, I was hopping you can understand the logic.... Ok, Check this out http://sqlfiddle.com/#!17/cc6db0/1

"You completely misunderstood what LoadFilteredPolicy does" --- Yeah I'm loading subset for user 'alice' as defined in your example, why then it is loading policies for Admin ?... or can you explain what is the practical use of this if you understand:
e.LoadFilteredPolicy(&pgxadapter.Filter{ P: [][]string{{"", "data1"}}, G: [][]string{{"alice"}}, })

@pckhoi
Copy link
Owner

pckhoi commented Jun 16, 2022

So I thought you wanted to turn the OR query into an AND query? OR did I misunderstand? What is the exact query that you want the adapter to produce?

@AAZANOVICH
Copy link
Author

Logically I would expect something like this http://sqlfiddle.com/#!17/cc6db0/2 (not exact query, but the output)
I would check how other (official ?) adapters work, but this implementation looks strange to me.

@pckhoi
Copy link
Owner

pckhoi commented Jun 17, 2022

This adapter was made mirroring this official adapter: https://github.com/casbin/casbin-pg-adapter

Please read its code and tell me if it does something different. Not only are you wrong, you are very rude for someone who are using someone else's free work.

Guess what? You are also free to make your own software.

@pckhoi pckhoi closed this as completed Jun 17, 2022
@AAZANOVICH
Copy link
Author

AAZANOVICH commented Jun 17, 2022

First of all, I wasn't rude to you and raising problems is also a contribution, don't take it personal. I think I pretty clearly described the issue and made extra step (provided working example) to make sure my point is clear. On opposite you did't pay attention to what I wrote and attacked me from the first message... that I "completely misunderstood"..

I read official docs for casbin https://casbin.org/docs/en/policy-subset-loading, what I see there is this example:
`adapter := fileadapter.NewFilteredAdapter("examples/rbac_with_domains_policy.csv")
enforcer.InitWithAdapter("examples/rbac_with_domains_model.conf", adapter)

filter := &fileadapter.Filter{
P: []string{"", "domain1"},
G: []string{"", "", "domain1"},
}
enforcer.LoadFilteredPolicy(filter)`

In this example they use model with domains, practical use of this use case they basically brake down policies by domains, so they filter out policies and users from different domain. i.e.

p, admin, domain1, data1, read
p, admin, domain1, data1, write
p, admin, domain2, data2, read
p, admin, domain2, data2, write
g, alice, admin, domain1
g, bob, admin, domain2

It still not optimal, cos you will be loading all policies and users for the domain, but at least I can see logic here.

In your example:
e.LoadFilteredPolicy(&pgxadapter.Filter{ P: [][]string{{"", "data1"}}, G: [][]string{{"alice"}}, })
It is very confusing, since you filter by resource (or object) "data1" and user "alice" ... From this example it looks like it will load policies for specific user, but it is not true and if that the case I can't think of any practical use case where you would want to filter like this since there can be great amount of policies which mention access rule for "data1" which will by loaded. Mentioning "data1" in P filter will not filter out policies which are not associated with user "alice" and mentioning "alice" in G filter will not eliminate groups which are not related to "data1"..

So, could you please tell me what I missing or wrote incorrect, so I finally can understand ?

p.s. I appreciate that you contributing to open source software, but don't take it personal when people raising problems.

@pckhoi
Copy link
Owner

pckhoi commented Jun 19, 2022

I contributed significantly to the official adapter https://github.com/casbin/casbin-pg-adapter. So maybe I do know a lot more than you about what a correct implementation looks like. Unqualified remarks such as "defect", or "strange implementation" are not welcome at all. You better have a water-tight case when you come to me with remarks like that.

Since this is free work, the burden of reading and understanding the example and the code fall largely on the user. This adapter maintains the same interface proposed by the official documentation so I don't understand what else I need to explain. The casbin-pg-adapter has the same example, yet countless people are able to use it without any problem. My advice to you, think harder, read the documentation harder, then you will understand.

And don't complain about documentation. This is free work. Want to have better documentation? Feel free to fork.

You need to be more charitable with your thought process and how you evaluate other people's works. I really will not tolerate any more unqualified remarks.

@AAZANOVICH
Copy link
Author

AAZANOVICH commented Jun 22, 2022

-...So maybe I do know a lot more than you about what a correct implementation looks like.
You still didn't explain the logic, while I explained the case to the full.. not sure what else should I include .. you just saying you know something... I'm sorry but what I see is that you just mirrored the lib without actually __thinking and understanding___what it is doing and how it is going to be used...

  • ...Unqualified remarks such as "defect", or "strange implementation"
    What is Unqualified here ? I gave you real problems with your lib and explained use case pretty well... you can't say nothing meaningful other than "you know something".. and yeah there a few defect exists which I mentioned, but seems to be you don't care...

  • ...And how you evaluate other people's works
    This is a technical discussion, I brought specific problems to discuss, but you can't say nothing about the actual problems.

Well, good luck maintaining this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants