linuxmuster / linuxmuster-linuxclient7

The new approach on connecting linuxclients to the Linuxmuster.net v7 Active Directory server.
3 stars 5 forks source link

Parse `not` attribute in `Drives.xml` GPP filters #72

Open dorianim opened 11 months ago

dorianim commented 11 months ago

The Drives.xml GPP filters also have the optional attribute not which negates the filter value. It should be handled.

kiarn commented 11 months ago

Hi Dorian,

I made this to test per role if a drive should be visible or not:

https://github.com/linuxmuster/linuxmuster-tools/blob/d782cdfc1782d9864075306d30f8e6eb763f263e/usr/lib/python3/dist-packages/linuxmusterTools/sambaTool/samba_tool.py#L38

I'm not sure how to interpret a bool="OR" attribute. BTW I'm only using it in user context.

Arnaud

dorianim commented 11 months ago

Hi Arnaud,

I don't think, your implementation will work properly. For example: I want a drive to be mapped for everyone who is in the project p_test and is NOT a student. In that case, your implementation would show the share for students in p_test because it will return at the first and condition. You always need to evaltualte all filters. Also, you only parse the FilterGroup and ignore everything else. I'd suggest always evaluating unknown filters to false. Otherwise, you may get false positives. That's how I do it in the linuxclient.

Regarding handling OR, I do it like this, not sure if it is correct, though: ((First filter) (Second filter)) (Third filter) And so on.

Please let us find another way for this.
It seems unnecessary that you are doing all this again when I have already done it. Also, it is a complicated part and new discoveries/bugs will always need work in two places, if we don't find a way to share this code. And lastly, if we both implement it, there will be diffrences and therefore inconsitencies between the linuxclient and the webui which will confuse users.

Regards, Dorian

kiarn commented 11 months ago

Hi Dorian,

I don't think, your implementation will work properly. For example: I want a drive to be mapped for everyone who is in the project p_test and is NOT a student. In that case, your implementation would show the share for students in p_test because it will return at the first and condition. You always need to evaltualte all filters. Also, you only parse the FilterGroup and ignore everything else. I'd suggest always evaluating unknown filters to false. Otherwise, you may get false positives. That's how I do it in the linuxclient.

Hum yes, I assumed the filter are only working on groups, and didn't know it can be user specific. That's the reason why I forgot to handle Filters as a whole set.

Regarding handling OR, I do it like this, not sure if it is correct, though: ((First filter) (Second filter)) (Third filter) And so on.

I don't understand what you mean.

Please let us find another way for this. It seems unnecessary that you are doing all this again when I have already done it. Also, it is a complicated part and new discoveries/bugs will always need work in two places, if we don't find a way to share this code. And lastly, if we both implement it, there will be diffrences and therefore inconsitencies between the linuxclient and the webui which will confuse users.

Yes why not, but we are writing codes for differents use, and from different points of view (server/client). This means some other adaptation ( path of the file ), and I don't imagine you will install linuxmuster-tools on the client just for this. Or do you have another idea how to do this ?

Arnaud

dorianim commented 11 months ago

I don't understand what you mean.

Let's say, we have these filters:

Will result in a condition like this:

((((user.memberof("p_test1")) and user.memberof("p_test2")) or user.memberof("student")) and user.memberof("p_test3"))

Or do you have another idea how to do this ?

Linuxmuster-tools is just one package at the moment. How about splitting it up in multiple packages, since it is already organized in different modules anyway? Each module (ldapconnector, linbo, lmnfile, ...) could be published as a seperate pip package. Like that, we could selectively share code but sill keep it all in one repo. What do you think?

kiarn commented 11 months ago

I rolled back the old basic test in the webui, i.e. only check if the share is disabled or not. That's the behaviour since the beginning.

There's now a separate file to handle drives objects in the lmntools repository:

https://github.com/linuxmuster/linuxmuster-tools/blob/main/usr/lib/python3/dist-packages/linuxmusterTools/sambaTool/drives.py

It's based on a Drive dataclass model, and a DriveManager object, which provides methods on all drives, and get the policy path at init. I think using a method visible (or other name) on a object Drive is a flexible way to test on the fly if a drive should be displayed or not. The filters are already set in the Drive object, and the method visible then only needs the user informations (groups membership, role, etc ...). ldapreader can provide such an user informations object.

I just need a test in user context, but you need to test it in computer context if I see correctly. We can write another similar method for computer, or handle the differents cases in this function.

Packaging was initially planned only as deb package. In order to make a pip package, it would be necessary to change the structure I think. We can see it in a second time. But if we can reuse it in differents contexts, it would make sense.

Can you work with this structure ?

Arnaud

dorianim commented 11 months ago

Can you work with this structure ?

I think so :+1:

How does the ldap authentication work in linuxmuster-tools? If it uses a kerberos ticket, I could probably reuse it without any changes.

kiarn commented 11 months ago

It's intended to work as root on the server, with administrator a global-binduser account, the same as the webui. I must have a look to the kerberos auth if it's possible.

dorianim commented 11 months ago

Ah, ok. I thought, every user gets their own process and kerberos ticket and that is used.

kiarn commented 11 months ago

It's done: https://github.com/linuxmuster/linuxmuster-tools/commit/c6bb2bec7f14e01d35f7cab185fcb913f8b648f7

But I have to assume that it's run from a linuxclient, because I need to get the hostname in order send the request.

The differents modules in lmntools have some dependencies from each other, e.g. ldapreader depends on lmnfile to parse the config files. So, making separate packages will be difficult, but I don't think it's a problem, because the whole is like 400kb.