566468 - [l10n_site] map build LDAP group to build django group

Status: RESOLVED FIXED

(Webtools Graveyard :: Elmo, defect, P2)

Description

[Chris AtLee [:catlee]]

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.

Comment 1

[Axel Hecht [:Pike]]

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.

Comment 2

[Axel Hecht [:Pike]]

Making this a P1, this is dogfood.

Comment 3

[Axel Hecht [:Pike]]

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.

Comment 4

[Axel Hecht [:Pike]]

Attachment: 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.

Comment 5

[Wil Clouser [:clouserw]]

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.

Comment 6

[Aravind Gottipati [:aravind]]

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?

Comment 7

[Axel Hecht [:Pike]]

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.

Comment 8

[Aravind Gottipati [:aravind]]

@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.

Comment 9

[Axel Hecht [:Pike]]

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

Comment 10

[Axel Hecht [:Pike]]

Attachment: 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?

Comment 11

[Axel Hecht [:Pike]]

Aravind, ping?

Comment 12

[Aravind Gottipati [:aravind]]

@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)(memberUid=[e-mail]))".  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)(member=[e-mail])).  You can combine the two into one filter and not worry about what kind of group it is, with this filter.

'(&(cn=scm_l10n)(|(memberUid=[e-mail])(member=[e-mail])))'

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 13

[Axel Hecht [:Pike]]

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.

Comment 14

[Axel Hecht [:Pike]]

Also, I'm not currently working on this.

Comment 15

[Laura Thomson :laura]

Is this something that is still needed?  ccing Jabba as the current LDAP owner, too.

Comment 16

[Axel Hecht [:Pike]]

Yes, we should still do it.

Comment 17

[Peter Bengtsson [:peterbe]]

Attachment: 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. [e-mail]) as well as the regular LDAP email address. 

Test coverage is 100% but I've also tested it manually by inserting hardcoded `username="[e-mail]"` into the function to see that the groups are fetched correctly and assigned.

Comment 18

[Axel Hecht [:Pike]]

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, [e-mail] 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?

Comment 19

[Peter Bengtsson [:peterbe]]

Attachment: Groups adding and removing smarter now

Note the new constant GROUPS_MAPPING at the top.

Comment 20

[Axel Hecht [:Pike]]

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.

Comment 21

[Peter Bengtsson [:peterbe]]

Attachment: Now with a practical management command called ldap_lookup

Example output when running `./mane.py ldap_lookup [e-mail]`


LOCAL USER -------------------------------------------------------------------
Username             [e-mail]
Email                [e-mail]
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                 [e-mail]
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        [e-mail]
sn                   Hecht
givenName            Axel

LDAP GROUPS ------------------------------------------------------------------
scm_l10n         ->  Localizers

Comment 22

[Axel Hecht [:Pike]]

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.

Comment 23

[[github robot]]

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