If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[homepage] tests break because of LDAP authentication backend actually working in tests

RESOLVED FIXED in 1.2

Status

Webtools
Elmo
--
minor
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: peterbe, Assigned: peterbe)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

7 years ago
To run all tests, including those in apps/homepage/tests.py and those in lib/auth/tests.py you need to have some ldap_settings. If you have ldap_settings, the tests in 'homepage' will try to go out on the network and connect to an LDAP server.

LDAP authentication should be allowed to be in play during tests but individual tests orthogonal to LDAP authentication should work without the network.
(Assignee)

Updated

7 years ago
(Assignee)

Comment 1

7 years ago
Created attachment 531964 [details] [diff] [review]
Patch to greatly simplify scripts/build.sh and to make all tests run without needing LDAP network access
Attachment #531964 - Flags: review?(l10n)

Comment 2

7 years ago
Comment on attachment 531964 [details] [diff] [review]
Patch to greatly simplify scripts/build.sh and to make all tests run without needing LDAP network access

Review of attachment 531964 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following comments:

::: scripts/build.sh
@@ +34,5 @@
>  " > settings/local.py
> +# these settings are overwritten by the test setups but the file must exist
> +echo "
> +LDAP_HOST = LDAP_DN = LDAP_PASS = 'test'
> +" > settings/ldap_settings.py

Make this copy over ldap_settings.py-dist instead?

That way we ensure that our example -dist data is actually useful for contributors, and I think that should work.

Which also means that we need to fix ldap_settings.py-dist to use non-empty strings for the LDAP_* settings.
Attachment #531964 - Flags: review?(l10n) → review+
(Assignee)

Comment 3

7 years ago
Created attachment 531978 [details] [diff] [review]
Use ldap_settings.py-dist instead

as discussed on irc
Attachment #531964 - Attachment is obsolete: true
Attachment #531978 - Flags: review?(l10n)

Comment 4

6 years ago
Comment on attachment 531978 [details] [diff] [review]
Use ldap_settings.py-dist instead

So those asserts in auth/backends.py aren't doing anything reliably, obviously. If you run python -O, they don't do stuff, if you run plain python, they assert. Where the web says that -O can be default, even.

Anyways, making the error reporting around our ldap backend do the right thing is a follow up bug.

I want our -dist settings to satisfy the assertions for the time being, so make them non-empty strings in ldap_settings.py-dist?
Attachment #531978 - Flags: review?(l10n) → review-
(Assignee)

Comment 5

6 years ago
(In reply to comment #4)
> flag: review?(l10n@mozilla.com) => review-Comment on attachment 531978 [details] [diff] [review]
> [details] [review]
> Use ldap_settings.py-dist instead
> 
> So those asserts in auth/backends.py aren't doing anything reliably,
> obviously. If you run python -O, they don't do stuff, if you run plain
> python, they assert. Where the web says that -O can be default, even.
> 
The asserts in auth/backends.py makes sure the developer he doesn't try to run the server without having configured his LDAP settings. 

No one runs django with `python -O` and if you do you'll just get LDAP related errors. 

> Anyways, making the error reporting around our ldap backend do the right
> thing is a follow up bug.
> 
> I want our -dist settings to satisfy the assertions for the time being, so
> make them non-empty strings in ldap_settings.py-dist?

I have another idea. Instead of this assertion business, how about a proper error raised. 

The enabling ldap_settings.py (i.e. copying from ldap_settings.py-dist) means you want to use LDAP and you must then type in the right DN, password etc. 
If you don't do this, it won't be noticed until you run the server. So how about something like this (ignore the excessively verbose comments):

# in settings/base.py

try:
    import ldap_settings
    # cool, if created, all settings must be entered
    for each in 'LDAP_HOST', 'LDAP_DN', 'LDAP_PASS':
        if not getattr(ldap_settings, each, None):
            raise ValueError('%s must be set' % each)
except ImportError:
    ldap_loaded = False

if ldap_loaded:
    # ImportErrors are not acceptable if ldap_loaded is True
    import ldap



This way, you notice straight away what's wrong if you've tried to enable LDAP. 
Thoughts?

Comment 6

6 years ago
Yeah, sounds good. Let's table the discussion on where to put that code in case we liberate our ldap auth backend for use in other webdev django projects.

That'd be:
- remove the asserts
- change base.py as above (*)
- update ldap_settings.py-dist to have non-empty strings (as they'd ValueError?)

You'll a 
from ldap_settings import *
to put the LDAP* into django settings.
Not sure if the check is better to be 
if each not in locals(): ?
You didn't make an explicit ldap_loaded=True, probably just a typo.

Just technicalities, make it work ;-)
(Assignee)

Comment 7

6 years ago
Created attachment 532271 [details] [diff] [review]
Now errors are raised early and explicitely if any ldap settings are wrong

settings/base.py is getting ugly but it works.
Attachment #531978 - Attachment is obsolete: true
Attachment #532271 - Flags: review?(l10n)
Comment on attachment 532271 [details] [diff] [review]
Now errors are raised early and explicitely if any ldap settings are wrong

Review of attachment 532271 [details] [diff] [review]:
-----------------------------------------------------------------

::: settings/base.py
@@ +278,2 @@
>      ldap_loaded = False
>  

Maybe, instead of ldap_loaded, you could do something along the lines:

try:
    import ldap_settings
except ImportError:
    pass
else:
    for each in 'LDAP_HOST', 'LDAP_DN', 'LDAP_PASS':
        if not getattr(ldap_settings, each, None):
            raise ValueError('%s must be set' % each)

    from ldap_settings import *
    # ImportErrors are not acceptable if ldap_loaded is True
    import ldap
    MIDDLEWARE_CLASSES = (MIDDLEWARE_CLASSES +
      ('django.contrib.auth.middleware.RemoteUserMiddleware',))
    AUTHENTICATION_BACKENDS = ('lib.auth.backends.MozLdapBackend',) 

Not sure if that qualifies as prettier or uglier, though :)
(Assignee)

Comment 9

6 years ago
Created attachment 532344 [details] [diff] [review]
Updated patch with Stas's feedback. No more ldap_loaded variable
Attachment #532271 - Attachment is obsolete: true
Attachment #532271 - Flags: review?(l10n)
Attachment #532344 - Flags: review?(l10n)

Comment 10

6 years ago
Comment on attachment 532344 [details] [diff] [review]
Updated patch with Stas's feedback. No more ldap_loaded variable

This patch got good enough. r=me.

I still expect that ldap_settings.py-dist needs non-'' values for the bool check to succeed, if so, land a fix to that extent with r=me assumed?
Attachment #532344 - Flags: review?(l10n) → review+
(Assignee)

Comment 11

6 years ago
Landed on origin/develop
https://github.com/mozilla/elmo/commit/afba746f4c19c1613ce58f492f6ff862e1d36b14
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

6 years ago
Also, Pike, as per your suggestion (assuming I understood it) I added default settings to ldap_settings.py-dist that aren't useless. 
https://github.com/mozilla/elmo/commit/d57fa7873a03c8357b7b00fda10b7059ede9990d

Now the whole test suite passes by running scripts/build.sh

Updated

6 years ago
Target Milestone: --- → 1.2

Updated

3 years ago
Assignee: nobody → peterbe
You need to log in before you can comment on or make changes to this bug.