The build group has an LDAP group, cn=buildteam,ou=groups,dc=mozilla. Can we get this group hooked up to the l10n dashboard so we can do things like get l10n changesets for releases.
We already check the scm_l10n group for localizers at http://hg.mozilla.org/l10n/django-site/file/dcde3f307692/l10n_site/auth/backends.py#l116, we should do the same thing to pick up people from the buildteam group and put them into the build group we have in l10n_site.
Making this a P1, this is dogfood.
This is a tad trickier than I hoped, in fact. There's a difference between scm_l10n and buildteam, the first is a posix group, the latter is a groupOfNames, and they store the members in different fields, and different versions of it. I'll attach a patch for review/feedback, testing for both ldap_id and username, but I'm not sure that matches the thinking of how things are stored in the backend. Wil, Aravind, I'd love to get your feedback on that.
Created attachment 446502 [details] [diff] [review] cache the actual member lists, for both localizers and buildTeam Factoring the caching of the groups into two different methods, and using a generic ldap group cacher. I'm checking if either username or ldap_id are in the memberlist, independent of which is stored in the posixGroup or groupOfNames. I hope that's least fragile without being ambiguous.
Aravind is your guy here, I know very little about the ldap backends. I do know wenzel is working on an SSO solution, but it's still a little ways out. Might be something you'll want to talk to him about though.
Axel & Wil: I am not sure what the intended use of this app is, or what kind of things it checks against.. Per the first comment, catlee is looking to get access to some sort of dashboard? How is that dashboard access controlled? If the application is apache basic auth controlled, we can simply specify multiple groups in the apache config and that's usually all that's needed. Am I missing the point here?
Hi Aravind, this is the web app that gandalf and I talked to you about in the old MV office a while back. We're using it to manage shipping the localized versions of our apps, and more. The authentication is against ldap, credentials is bug 528043. Right now, we're checking scm_l10n to see if an account is a localizer, and adjust permissions on the site to that extent. We'd like to add a check for buildteam to grant those a different set of permissions, without needing to have them log-in, and manually tweak the settings of each person in the build team. Those permissions are needed to do some administrative tasks as part of the actual build of the release bits.
@Axel: What happens when we change (add/delete) folks to the scm_l10n group in LDAP. You are caching the query results and reusing them for subsequent checks. I don't know Django well enough to know if those variables are re-initialized every time someone logs in, but I'd look into that before this is pushed live. In general LDAP is pretty quick with bind/search/result type operations. So, if you want you can simply query LDAP everytime someone logs in.
Yeah, the caching problem sounded unfortunate to me, too. Not that that's a new problem. How about hitting ldap on the actual permission checks, and new user creation? Those should only happen on a few occasions, and are cached for the lifetime of the request. We could do that with overloading MozLdapBackend.get_group_permissions
Created attachment 462080 [details] [diff] [review] check ldap groups whenever a permission check is done This is an alternative patch. I moved the ldap group checks into a single connection, which is checked whenever the django site actually does permission checks. It then updates the group list, both addition and removal, and calls the super class to actually map the current groups to the permissions. Multiple calls to permission checks on one view will be cached in the _group_perm_cache, so this shouldn't happen more than once per call. Aravind, what do you think?
@Axel: you could make ldap do the work for you, instead of fetching the localizers/build list and then searching through it. To look if a user is a localizer, set the filter to this "(&(cn=scm_l10n)(memberUidemail@example.com))". Check the result to see if it contains anything, if it does - the user is a localizer. Similarly for the build group the filter would be (&(cn=buildteam)(firstname.lastname@example.org)). You can combine the two into one filter and not worry about what kind of group it is, with this filter. '(&(cn=scm_l10n)(|(memberUidemail@example.com)(firstname.lastname@example.org)))' The second patch you attached confuses me. I am not sure how that get_group_permissions function ever gets called. The rest of this review is just for attachement 446502. You seem to be checking against some sort of local database with "if local_user.has_usable_password():". Where are these local users coming from? Why do you have them there? Other than that, I think that patch is fine. If you decide to change it to filter like I suggested, it will make it a little bit more efficient.
Comment on attachment 446502 [details] [diff] [review] cache the actual member lists, for both localizers and buildTeam The patches here don't apply no more, let's obsolete them.
Also, I'm not currently working on this.
Is this something that is still needed? ccing Jabba as the current LDAP owner, too.
Yes, we should still do it.
Created attachment 651944 [details] [diff] [review] match groups scm_l10n and buildteam What this does is that it changes the ldap authentication backend quite a bit. The logic with which it interacts with Django's User model remains the same. 1. First, it connects with a binduser 2. It searches for the user by the mail address 3. It observes the 'uid' and stores givenName, cn, mail for later. 4. Using the uid it does a search to find all matches where the uid=<uid from above> AND (cn=scm_l10n OR cn=buildteam) 5. Reduce the result to conclude which groups this user belongs to 6. Using the uid again, it does a second bind to test if the supplied password is correct. Note, the commented out XXX comment. What we could do is make it possible for folks to log in with their email alias (e.g. email@example.com) as well as the regular LDAP email address. Test coverage is 100% but I've also tested it manually by inserting hardcoded `username="firstname.lastname@example.org"` into the function to see that the groups are fetched correctly and assigned.
Comment on attachment 651944 [details] [diff] [review] match groups scm_l10n and buildteam Review of attachment 651944 [details] [diff] [review]: ----------------------------------------------------------------- I like the general direction, r- for the unsetting of groups outside of l10n and build, see below. Given that the tests didn't catch that, I'm going to ask for more of those, too. I'd love to keep a utility method to get ldap details, I commonly use __getRecord today in a python shell (well, it's encumbered name) to see if hg accounts are disabled and such. I don't mind fixing up the code my old branch at https://github.com/Pike/elmo/compare/develop...feature;stale-accounts, but it's a good indication which extra data from ldap I'm looking at (with svnAccessDate added). ::: lib/auth/backends.py @@ +89,5 @@ > + # first, figure out the uid > + mail_filter = self.make_search_filter(dict(mail=mail)) > + # XXX with these lines we'd make it possible to log in with your alias > + #alias_filter = self.make_search_filter(dict(emailAlias=mail)) > + #search_filter = '(|%s%s)' % (mail_filter, alias_filter) I don't think we should do this, I'm afraid that just complicates the user experience for sites where we don't allow this. Unless IT recommends it, as it's on elsewhere. @@ +209,5 @@ > > + if in_localizer_group: > + user.groups = (Group.objects.get(name='Localizers'),) > + if in_build_group: > + user.groups = (Group.objects.get(name='build'),) Users can be both l10n and releng, email@example.com is a good example. Also, this would unset WebLocalizer, drivers, SignoffReviewers, right? We should also unset groups if they're no longer priviledged, though I'd probably love to know who'd be affected, comparing the local vs the remote ldap state. Of course only for those two groups :-/ It'd be good to add that to the tests, that is, "don't mess non-ldap groups" as well as "add ldap group to existing user" and "not add non-granted ldap group" etc. ::: lib/auth/tests.py @@ +73,2 @@ > }) > ] Why is this commented out? Also, the mock responses don't seem to match the results from the ldap server at this point, mind making them more equal again?
Created attachment 656512 [details] [diff] [review] Groups adding and removing smarter now Note the new constant GROUPS_MAPPING at the top.
Comment on attachment 656512 [details] [diff] [review] Groups adding and removing smarter now Review of attachment 656512 [details] [diff] [review]: ----------------------------------------------------------------- This looks great, thanks. Do you have an idea how I could keep a tool to query ldap through the shell in a smooth way? I do that remarkably often, and I'd prefer to not have to construct the ldap queries by hand. Not really part of this patch per se, but refactoring the code might be easier if we put the creation of self.ldo into the constructor, and only bind() in the actual methods. I also have some comments below to make the group queries a bit tighter. Not sure if the list() on the queryset for the current group helps a lot, seems like 'in' hits the _result_cache, but the code looks weird. Probably either way. Django result caching always confuses me. ::: lib/auth/backends.py @@ +216,2 @@ > else: > + user.groups.remove(Group.objects.get(name=django_name)) I guess we could optimize the queries here a bit? I was thinking along the lines of django_groups = list(user.groups .filter(name__in=GROUP_MAPPINGS.keys()) .values_list('name', flat=True)) groups_to_add =  groups_to_remove =  for django_name, ldap_name in GROUP_MAPPINGS.iteritems(): if ldap_name in in_groups: if django_name not in django_groups: groups_to_add.append(django_name) else: if django_name in django_groups: groups_to_remove.append(django_name) if groups_to_add: user.groups.add(*list(Group.objects.filter(name__in=groups_to_add))) if groups_to_remove: user.groups.remove(*list(Group.objects.filter(name__in=groups_to_remove))) I doubt we're feeling it with two groups, but in the optimized scenario, we do 1 query instead of two.
Created attachment 657482 [details] [diff] [review] Now with a practical management command called ldap_lookup Example output when running `./mane.py ldap_lookup firstname.lastname@example.org` LOCAL USER ------------------------------------------------------------------- Username email@example.com Email firstname.lastname@example.org First name Axel Last name Hecht Active True Superuser True Staff True Groups: Localizers, drivers, SignoffReviewers IN LDAP ---------------------------------------------------------------------- uid axel objectClass inetOrgPerson, ldapPublicKey, posixAccount, svnAccount, sambaAccount, hgAccount, mozComPerson, bugzillaAccount, position cn Axel Hecht hgAccessDate 20120831122623.573732Z hgHome /tmp mail email@example.com hgAccountEnabled TRUE svnAccountEnabled TRUE svnHome /tmp sshPublicKey ssh-dss AAAAB3NzaC1kc3MAAACBALr/NuSAsyY9STMSKkJs4/DL7P7znRiDDZQyJ1gkHyTOg1rRQrjuX10hWbMW2ohBDtcMsFbeVk3KhnPRE4qsQgnB20gaU4T/Qs287CPwn2nY62KCD4h2li6xSDO50ijcEaeuhST73zTAScUuQM58kR6g6pLHPUzMHBhF/1B0QGRFAAAAFQDTVAYuY/SmLjA6uAZ6oC3+yqEyCwAAAIAm0rG05P7tZlwBhJRJSftM3r+s6esErMM1GZw0WwZSpbxbyeRB1eU10skqM0Orn3buT3ZHH/9WE06EF02PM3uvRZyEXDrFI+KIx70bveqK/V0ljWuA+sEUXuFUweA0IX5Gvk4E4eX/tLTiN2mHx+bmHrNjQOovIl9MkNXPGJ7MQQAAAIAUuPc9j9r5kXPoJcGsp8Rulg3QlFjSDyH7aR2yGhgtGn74SGpMKV38iJMrt5l+inolrnKcgvjLiSF48egaktYvS79hn3nljiiAa8goymcvvylkbN86j3rm7McTUvGZN39e66PqkkZLbjBrW2piE4wvU1A6Nu3Eu6zuGLKy5wey/Q== hecht@hilbert svnAccessDate 20120424203012.019975Z bugzillaEmail firstname.lastname@example.org sn Hecht givenName Axel LDAP GROUPS ------------------------------------------------------------------ scm_l10n -> Localizers
Comment on attachment 657482 [details] [diff] [review] Now with a practical management command called ldap_lookup Review of attachment 657482 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for hanging in here, I think I found two more that I didn't catch in the previous reviews. Also to some extent because I didn't read up what initialize() actually done until sometime mid-way through the reviews here. That's actually not doing any network-y stuff, so it should be fine to keep the ldo around and just pair bind and unbind. With the nits below, r=me and ready to land. ::: lib/auth/backends.py @@ +154,5 @@ > + > + # Now we know everything we need to know about the user but lastly we > + # need to check if their password is correct > + self.ldo = ldap.initialize(self.host) > + self.ldo.set_option(ldap.OPT_PROTOCOL_VERSION, 3) These should still be around from the connect(), I think. @@ +161,4 @@ > except ldap.INVALID_CREDENTIALS: # Bad password, credentials are bad. > return > except ldap.UNWILLING_TO_PERFORM: # Bad password, credentials are bad. > return Sorry, didn't notice this in the earlier reviews, the bind is missing an unbind in an else:, probably.
Commits pushed to develop at https://github.com/mozilla/elmo https://github.com/mozilla/elmo/commit/1731024cec728be6bea8bfe5276595a1306d4f21 bug 566468 - new auth backend that looks up groups efficiently, r=Pike https://github.com/mozilla/elmo/commit/9bf4552c7d5479f4eb6c8ffd31d0d985f52afcec bug 566468 - ldap_lookup management command, r=Pike https://github.com/mozilla/elmo/commit/e946480db7fbaed1acf93399c7fb8e16b33edbe0 Merge branch 'feature/bug566468-map-ldap-groups' into develop