Hi!

On Friday 30 January 2009 16:41:17 Christian Hammond wrote:
> Hi Alexey.
> We have a patch up that I haven't reviewed yet (I'd like other
> LDAP-knowledgeable users to look into it if possible) that may address your
> concerns. Would you be able to give it a try?
>
> http://reviews.review-board.org/r/704/

Well, I would propose another way to solve this problem:

http://reviews.review-board.org/r/729/

As the review description says this solution still allows to use complex LDAP 
hierarchies. Suppose one has a big organization where all users divided into
different departments or "organization units" (ou's). So for Joe his "login" 
string (I'm not sure how this entity is properly called in LDAP) 
is 'uid=joe,ou=research_development,ou=users,dc=company,dc=com" while for 
Jane her login looks 
like 'uid=jane,ou=sales_marketing,ou=users,dc=company,dc=com"

Searching prior to binding allow to determine an exact login string for a 
given "shortcut" name.

Also I would like to propose another fix 

http://reviews.review-board.org/r/730/

When I first switched to LDAP based auth I discovered that I can't login 
as 'admin' anymore. Short investigation showed that the problem is in 
exception handling. django.contrib.auth.authenticate() (as of 1.0.2) catches 
only TypeErrors from a backend authenticate() so if another error occures 
(IndexError in my case) it simply stops processing of an authentication chain 
(in case of LDAP in reviewboard this chain is 'LDAP, builtin'). Probably the 
similar appoach should be applied to all auth backeds too.

While submitting these two patches I also stepped into few other issues :)

First I couldn't type certain letters (e.g. 'a' and 'A') into Summary and 
Description fields when diffviewer is active (e.g. on 
http://reviews.review-board.org/r/729/diff/ ) Probably these keys are 
intercepted as hotkeys by diffviewer component. I use Firefox-3.0.4 on Linux.

Also I found that submitting a patch is somewhat hard and cumbersome . The 
problem is how a patch is parsed upon upload. Since reviewboard repo type is 
SVN the patch processing machinery assumes that the patch is made by svn (or 
svntool), so it has additional fields in patch header, particularly file 
revision. This obviously isn't always true, suppose the patch is made against 
an installed file or it was made in another SCM (git in my case). I had to 
look through reviewboard code to understand what it expects to see in the 
patch and modify the patches accordingly.

Maybe it's worth to re-arrange the whole process. I would propose smth like 
this:

when a submitter uploads a patch he or she is presented with an interface 
which allows to choose a place (either a branch/tag or a single revision) 
where the patch should be applied. Since reviewboard knows exactly how to 
exctract this information from the repository this shouldn't be too hard to 
implement. Another option is to upload a modified file (or files) instead of 
a patch. Probably this interface should have SCM-specific parts (or 
callbacks) (yes I dream of git commits-based reviews :))

Unfortunately reviewboard is the first django app I deal with so I can't 
implement it myself, at least right now.

Yours respectfully,
Alexey Morozov

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups 
"reviewboard" group.
To post to this group, send email to reviewboard@googlegroups.com
To unsubscribe from this group, send email to 
reviewboard+unsubscr...@googlegroups.com
For more options, visit this group at 
http://groups.google.com/group/reviewboard?hl=en
-~----------~----~----~----~------~----~------~--~---

Reply via email to