Summary

Adds ability to use x.509 certificates for authenticating users.

Review Request #938 — Created Aug. 2, 2009 and discarded

Information

Owner

nheijermans

Repository

Review Board SVN (deprecated)

Branch

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

These changes enable Review Board to use x.509 certificates to be used alongside the builtin username/password authentication backend. This functionality will be useful for deployments in environments with a Public Key Infrastructure set up.

I deliberately avoided modifying post-review to support client certificates due to a lack of decent support for them in Python's high-level libraries. Instead, I modified the user preferences view to permit authenticated users to set their Django password. Django's built-in support for multiple authentication backends will then permit them to use the builtin auth backend with post-review.

Testing Done

Tested on my test Review Board server, running Apache/mod_python,mod_ssl/Django 1.1.

Thanks for working on this! I have a laundry list of changes I'd like you to make, but most of them are style guideline stuff.

nheijermans Aug. 2, 2009, 10:21 p.m.

No problem. It's something I've wanted to do the right way for a while. Thanks for the speedy feedback.

/trunk/reviewboard/accounts/backends.py (Diff revision 1)
```
Should be in the list alphabetically.
```
1. NH
  
  nheijermans Aug. 2, 2009, 9:48 p.m.
  Fixed.

/trunk/reviewboard/accounts/backends.py (Diff revision 1)

I know the others don't do it, but this should inherit from object. The others all should too, eventually.

Also, please add a doc block here describing the purpose and use of this backend.

NH

nheijermans Aug. 2, 2009, 10:12 p.m.
```
Done.
```

/trunk/reviewboard/accounts/backends.py (Diff revision 1)
```
This should make use of the siteconfig stuff. Only grab this value as needed.
```
1. NH
  
  nheijermans Aug. 2, 2009, 9:56 p.m.
  Done.

/trunk/reviewboard/accounts/backends.py (Diff revision 1)

Need to wrap this in a try/catch. If the user specifies an invalid regular expression, this will fail.

nheijermans Aug. 2, 2009, 9:56 p.m.

Good call. Fixed it. What's the preferred way of getting feedback to the user on errors?

/trunk/reviewboard/accounts/backends.py (Diff revision 1)
```
This should be prefixed with the backend name. Also, no "!" is needed.
```
1. NH
  
  nheijermans Aug. 2, 2009, 9:56 p.m.
  Fixed.
/trunk/reviewboard/accounts/backends.py (Diff revision 1)
```
No blank line here.
```
1. NH
  
  nheijermans Aug. 2, 2009, 9:56 p.m.
  Fixed.
/trunk/reviewboard/accounts/backends.py (Diff revision 1)
```
Should just do that settings lookup here.
```
1. NH
  
  nheijermans Aug. 2, 2009, 9:57 p.m.
  Fixed.
/trunk/reviewboard/accounts/backends.py (Diff revision 1)
```
Trailing whitespace.
```
1. NH
  
  nheijermans Aug. 2, 2009, 10:09 p.m.
  Fixed.
/trunk/reviewboard/accounts/views.py (Diff revision 1)
```
Can you change this to "can_change_password"?
```
1. NH
  
  nheijermans Aug. 2, 2009, 9:57 p.m.
  Done.

/trunk/reviewboard/accounts/views.py (Diff revision 1)

Can collapse this to:

if can_change_password and form.cleaned_data['password1']:

NH

nheijermans Aug. 2, 2009, 9:57 p.m.
```
Good call. Done.
```

/trunk/reviewboard/admin/forms.py (Diff revision 1)
```
"Username Field"
```
1. NH
  
  nheijermans Aug. 2, 2009, 9:58 p.m.
  Fixed.
/trunk/reviewboard/admin/forms.py (Diff revision 1)
```
correspond *to*
```
1. NH
  
  nheijermans Aug. 2, 2009, 9:58 p.m.
  Fixed.

/trunk/reviewboard/admin/forms.py (Diff revision 1)

"extracted." should align with the beginning of the string on the previous line.

Also, "will be extracted," rather than "is to be extracted."

NH

nheijermans Aug. 2, 2009, 9:59 p.m.
```
Fixed.
```

/trunk/reviewboard/admin/forms.py (Diff revision 1)

"Username Regex"

Do we really need to expose this to the user? How often will they need to do something custom? If it's generally in e-mail form or something, I'd rather keep it simple.

nheijermans Aug. 2, 2009, 10:20 p.m.

It is a bit ugly, but I couldn't think of any other way to get a 100% solution. Since the format of the certificate fields is completely up to the issuer, I don't think that taking care of the general case will be sufficient. In most cases, the email will be present in the certificate, although it isn't required. I wanted to make sure this backend would fit everybody's needs.

Would it be acceptable to keep this setting, but not expose it in the admin UI? We could default to getting the username from the email address, or perhaps just matching on the email address, but leave the regular expression syntax in there for the outlier cases. This could still be set in the settings_local.py.

/trunk/reviewboard/admin/forms.py (Diff revision 1)
```
Alignment.
```
1. NH
  
  nheijermans Aug. 2, 2009, 10:03 p.m.
  Fixed.
/trunk/reviewboard/admin/forms.py (Diff revision 1)
```
Alignment.
```
1. NH
  
  nheijermans Aug. 2, 2009, 10:04 p.m.
  Fixed.
/trunk/reviewboard/admin/forms.py (Diff revision 1)
```
Remove "this" on "this Review Board.
```
1. NH
  
  nheijermans Aug. 2, 2009, 10:04 p.m.
  Fixed.
/trunk/reviewboard/admin/middleware.py (Diff revision 1)
```
Blank line between these.
```
1. NH
  
  nheijermans Aug. 2, 2009, 10:05 p.m.
  Done.
/trunk/reviewboard/admin/middleware.py (Diff revision 1)
```
Should subclass object.
```
1. NH
  
  nheijermans Aug. 2, 2009, 10:05 p.m.
  Done.

/trunk/reviewboard/admin/middleware.py (Diff revision 1)

"TODO:"

Also, I'd like to bail from this as early as possible. We should check the auth backend immediately and determine whether we should execute any of this code.

nheijermans Aug. 2, 2009, 10:20 p.m.

Sounds good. Is SiteConfig.objects.get_current() the preferred way of doing this? It seems like hitting the database would be more expensive than a couple of dictionary lookups... I haven't fixed this yet, as I'm not sure what the best way to do it is.

/trunk/reviewboard/admin/middleware.py (Diff revision 1)

I'd prefer getattr(settings, "X509_USERNAME_FIELD", None), so that we don't break if this setting isn't at all in there.

Also, this can be combined with the line below.

NH

nheijermans Aug. 2, 2009, 10:07 p.m.
```
Done.
```

/trunk/reviewboard/templates/accounts/prefs.html (Diff revision 1)
```
Should be aligned at column 0.
```
1. NH
  
  nheijermans Aug. 2, 2009, 10:09 p.m.
  Fixed.
/trunk/reviewboard/templates/accounts/prefs.html (Diff revision 1)
```
Same here.
```
1. NH
  
  nheijermans Aug. 2, 2009, 10:09 p.m.
  Fixed.

Change Summary:

Fixed all issues from Christian's review except for bailing early from the process_request() function in the middleware.

Diff:

Revision 2 (+162 -10)

Show changes

	/trunk/reviewboard/settings.py
	/trunk/reviewboard/accounts/backends.py
	/trunk/reviewboard/accounts/views.py
	/trunk/reviewboard/admin/forms.py
	/trunk/reviewboard/admin/middleware.py
	/trunk/reviewboard/admin/siteconfig.py
	/trunk/reviewboard/templates/accounts/prefs.html

/trunk/reviewboard/admin/forms.py (Diff revision 2)

Just a minor remark, but I think the correct spelling is "X.509", not "x.509"

NH

nheijermans Aug. 23, 2009, 6:38 a.m.
```
Fixed.
```

/trunk/reviewboard/accounts/backends.py (Diff revision 2)
```
Parameters should be aligned.
```
1. NH
  
  nheijermans Aug. 23, 2009, 6:27 a.m.
  Fixed.
/trunk/reviewboard/accounts/backends.py (Diff revision 2)
```
Same here. No need for str(e), though, as the "%s" covers that.
```
1. NH
  
  nheijermans Aug. 23, 2009, 6:27 a.m.
  Fixed.
/trunk/reviewboard/accounts/views.py (Diff revision 2)
```
Remove this blank line.
```
1. NH
  
  nheijermans Aug. 23, 2009, 7:47 a.m.
  Done.

/trunk/reviewboard/accounts/views.py (Diff revision 2)

We should store form.cleaned_data['password1'] in a variable and then check that variable here and use it in the sha1 code below.

NH

nheijermans Aug. 23, 2009, 6:51 a.m.
```
Fixed.
```

/trunk/reviewboard/admin/forms.py (Diff revision 2)
```
X.509
```
1. NH
  
  nheijermans Aug. 23, 2009, 6:27 a.m.
  Fixed.
/trunk/reviewboard/admin/forms.py (Diff revision 2)
```
X.509
```
1. NH
  
  nheijermans Aug. 23, 2009, 6:27 a.m.
  Fixed.
/trunk/reviewboard/admin/forms.py (Diff revision 2)
```
X.509
```
1. NH
  
  nheijermans Aug. 23, 2009, 6:28 a.m.
  Fixed.
/trunk/reviewboard/admin/forms.py (Diff revision 2)
```
X.509
```
1. NH
  
  nheijermans Aug. 23, 2009, 6:28 a.m.
  Fixed.
/trunk/reviewboard/admin/middleware.py (Diff revision 2)
```
X.509
```
1. NH
  
  nheijermans Aug. 23, 2009, 6:28 a.m.
  Fixed.

/trunk/reviewboard/admin/middleware.py (Diff revision 2)

str(None) isn't going to work so well...

We should do:

x509_settings_field = getattr(settings, "X509_USERNAME_FIELD")

if x509_settings_field:
    x509_field = env.get(x509_settings_field)

    ....

NH

nheijermans Aug. 23, 2009, 6:36 a.m.
```
Fixed.
```

Change Summary:

This change adds the implementation of the code review suggestions from Thilo and Christian.

Diff:

Revision 3 (+164 -10)

Show changes

	/trunk/reviewboard/settings.py
	/trunk/reviewboard/accounts/backends.py
	/trunk/reviewboard/accounts/views.py
	/trunk/reviewboard/admin/forms.py
	/trunk/reviewboard/admin/middleware.py
	/trunk/reviewboard/admin/siteconfig.py
	/trunk/reviewboard/templates/accounts/prefs.html