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

[enh] Simplify the whole LDAP interface thing #721

Merged
merged 8 commits into from May 22, 2019

Conversation

Projects
None yet
2 participants
@alexAubin
Copy link
Member

commented May 9, 2019

The problem

c.f. YunoHost/moulinette#183

Solution

c.f. YunoHost/moulinette#183

This is a cherry-pick from #585 for the part that handle LDAP authentication using root instead of asking for the admin password each time. Considering that the whole group permission PR is likely to take some time, yet this thing is a pretty interesting independent feature ...

I started iterating on it to also get rid of the whole 'auth' everywhere madness and simplify other related thing in the actionmap (the mysterious 'authenticate' which nobody knows how to deal with anymore or what it does exactly)

So to summarize the two major benefits right now are :

  • simpler / monre consistent UX (no weird behavior for CLI user where some commands would ask you the admin password even tho you are already authenticated as root/admin)
  • simplify code by removing the need to pass the old auth object everywhere (e.g. for every function where you would like to later call domain_list or app_ssowatconf in a sub-sub-sub-sub-call)

PR Status

Made a quick test and it looks like this is a working, but considering this is a touchy part I want to do moar tests. Also I might find some fancy stuff to factorize a bit the part where we fetch the auth object.

How to test

... Checkout this branch (and the corresponding one). You need to make sure that the ldap conf is updated otherwise this won't work ... then try any step that usually requires to enter the admin password (e.g. postinstall or user create)

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :

@alexAubin alexAubin changed the title Use root UID to authenticate to LDAP [enh] Simplify the whole LDAP interface thing May 10, 2019

@alexAubin

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

Currently this PR creates a weird issue occuring after calling yunohost user list :

Exception TypeError: "'NoneType' object is not callable" in <bound method Authenticator.__del__ of <moulinette.authenticators.ldap.Authenticator object at 0x7f03b507d310>> ignored

It doesn't seem to be so troublesome appart that it's a pretty annoying message ... the most puzzling thing is that I can't figure out why that seem to happen only with yunohost user list and not with yunohost domain list or any other function ...

@Josue-T

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

Well, why we don't just go forward with the group permission. I know that I don't have much time these days, but the problem with that is that after we might need fix many conflict.

The other problem is that with this PR we also need to update the LDAP config still problematic.

About the auth object I don't really understand why remove this because as I understand we need a an object which is a LDAP connection to the LDAP server. So without I don't know how can it work...

@alexAubin

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

Well, why we don't just go forward with the group permission. I know that I don't have much time these days, but the problem with that is that after we might need fix many conflict.

Yes, I too would like to move forward with group permissions. But there are important bugs to be fixed and I'm not comfortable / expert enough with the code and the whole LDAP thing to handle it ... So in the meantime, this feature of auth through root is just independent and easy to merge ...

I know that there will be many conflict, yes. In fact I recently had to basically redo a PR from scratch because it was one year old and too outdated... We just terribly lack humanpower to work on boring general interest tasks such as the incoming migration to Buster, the migration to Python 3, and in general reviewing/fixing/finishing opened PRs ... And despite this I'd really like to have some time some day to work on important features such as the high-level diagnosis thing...

The other problem is that with this PR we also need to update the LDAP config still problematic.

I don't think it is because it just changes the slapd.conf, which should be handled automatically by the regenconf ? It doesn't need a schema update

About the auth object I don't really understand why remove this because as I understand we need a an object which is a LDAP connection to the LDAP server. So without I don't know how can it work...

Please read the diff. I just fetch the ldap interface right when we actually need it instead of passing the auth argument all over the place

@Josue-T

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

But there are important bugs to be fixed and I'm not comfortable / expert enough with the code and the whole LDAP thing to handle it ...

For this, I could help you if it's the only issue.

So in the meantime, this feature of auth through root is just independent and easy to merge ...

I just think that if the time that you past over this PR was passed on the group-permission probably less work would be needed. And why this feature is soo important ? I just feel with this PR that the group-permission will be never merged if we continue like that. And in my point of view passing the time to do 2 time the same thing is really a wast of time.

I don't think it is because it just changes the slapd.conf, which should be handled automatically by the regenconf ? It doesn't need a schema update

Yes but as same as for the group-permission, we need to think about a user which as customized his config. In this case Yunohost might be completely broken...

We just terribly lack humanpower to work on boring general interest tasks such as the incoming migration to Buster, the migration to Python 3.

I hope to find some time to work on this, this summer when the group-permission is completely finished.

Please read the diff. I just fetch the ldap interface right when we actually need it instead of passing the auth argument all over the place Merge state

Sorry I didn't see really quickly the diff.

@alexAubin

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

I don't know what to tell you... Let's get things straight : we're all in the same boat. A large part of my implication in the technical side of YunoHost consist in trying to help merge as many PR as I can so the project moves forward and hopefully I can at some point win the right to implement the cool stuff I'm dreaming of... I'm not trying to jeopardize stuff, especially considering that the group permission feature is an important thing that people ask for. I myself already took the time to review and fix what I could and it took me at least 2 days from what I can remember ?

There is nothing "so important" about the present PR, it's just something quite independent of the whole group permission thing, and it's nice to have as it removes weird UX behavior and simplify code. It's a small thing and is easily mergeable so we can just benefit it immediately and move forward, and then that's one thing less to validate in the group permission thing. It "only" took me basically half a day to test (the existing part from the group permission PR) and implement the other small code simplification. The additional commits are not related to the group permission thing either ... It's just complementary work to the whole 'use root to authenticate on the LDAP' idea...

I perfectly understand your frustration that the group permission don't get merged more quickly even tho most of the work is done. But to me that's just a classic illustration of the Pareto principe applied to development : the first 80% of the work will take only 20% of the development time (or if you prefer : the last 20% will take 80% of the development time) - because edge cases are hard to handle, things must be tested and reviewed, and so on. We see this everyday with many non-trivial PRs both in core and apps.

If you are worried about the conflicts then :

  • the bad news is that I would probably not expect this present PR to be the worst conflict that'll need to be solved (c.f. the service/regenconf decoupling which I expect will be more tricky to handle, + c.f. existing conflicts + others to come)
  • the good news is that I can the conflict fix if you're really afraid of them. This I can easily do.

But as stated I don't really master the whole permission/LDAP thing. For instance about this issue I found I can't really say anything more than "I don't know how to fix it", and several comments are related to the fact that it's not clear for me what's the intention / design choice / ... Not because it's bad or anything, just that the feature is big and complex, so it's expected that there's a need to iterate.

@Josue-T

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

Ok, so going forward with this PR.

Quickly see the code and it look me like ok.

I tried on my side and it work but only if I use the branch sasl_authentication of the moulinette.

I didn't get your issue about the user list command.

@alexAubin

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

Yes indeed, forgot to mention that the sasl_authentication branch is also needed 😉

That's weird though somehow a good news that you don't have this issue, maybe I should re-try with a clean setup, I might have tweaked something weird somewhere

@Josue-T

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

For we still have the problem that we can't be sure that the regen-conf is executed (if the used has customized is config). So what is your solution about that ?

@alexAubin

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

Meh I dunno, I don't really have a solution about this, other than saying that I expect this to only correspond to a small minority of technical user (even more technical than tweaking the nginx or ssh conf) so meh ... We can just explain those people to force the regenconf of ldap and everything shall be back to normal :/

@Josue-T

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

Meh I dunno, I don't really have a solution about this, other than saying that I expect this to only correspond to a small minority of technical user (even more technical than tweaking the nginx or ssh conf) so meh ... We can just explain those people to force the regenconf of ldap and everything shall be back to normal :/

OK, might give a good statistic for the next step with group-permission how many instance is impacted 😄

@@ -338,9 +304,6 @@ domain:
dns-conf:
action_help: Generate DNS configuration for a domain
api: GET /domains/<domain>/dns
configuration:
authenticate:
- api

This comment has been minimized.

Copy link
@Josue-T

Josue-T May 10, 2019

Contributor

Do you know exactly what is this ?

This comment has been minimized.

Copy link
@alexAubin

alexAubin May 10, 2019

Author Member

Meh not really :/ This stuff is quite mystic ... my understand is that it's legacy code which did not get reimplemented properly once the authenticate thing was implemented ? And the part about 'api' seem to suggest that authentication should be done when used from the API ... which sounds kinda the same as other stuff anyway so ...

Idk ¯\_(ツ)_/¯

@alexAubin

This comment has been minimized.

Copy link
Member Author

commented May 17, 2019

Ran some more tests + a few fixes, now the PR should be complete

@Josue-T

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

Ran some more tests + a few fixes, now the PR should be complete

Did you run the unit test ?

@alexAubin

This comment has been minimized.

Copy link
Member Author

commented May 17, 2019

Yes ?

@alexAubin

This comment has been minimized.

Copy link
Member Author

commented May 22, 2019

Soooo idk, planning to merge during next week, feel free to review / provide more feedback

@alexAubin

This comment has been minimized.

Copy link
Member Author

commented May 22, 2019

Sooo in fact I'm gonna yolomerge this because we really need to test this on the battle field and the sooner the better ... (and also that'll allow to move forward with other PRs).

Like with any other merged PR, we can still iterate if needed anyway ...

@alexAubin alexAubin merged commit b3d2923 into stretch-unstable May 22, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@alexAubin alexAubin deleted the authenticate-as-root branch May 22, 2019

@alexAubin alexAubin referenced this pull request Jun 4, 2019

Merged

Add indexes for fields listed by slapd in the logs #729

0 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.