Fix for AD account backend group enumeration issue

Review Request #1152 — Created Oct. 5, 2009 and submitted

smat
Review Board
reviewboard
This patch updates the search query to use CN rather than sAMAccountName so that groups which have CNs that differ from their sAMAccountNames (e.g. if a "pre-Windows 2000" group name is set in Active Directory) will be included in the search results. The memberOf property used here to find groups specifies groups by DN, so searching on the CN component should always be the correct behavior (as far possible, using the full DN would be safer).
Tested on local installation of Review Board 1.0.3
chipx86
  1. Are there any chances of regressions from this? It'll be important to know for documentation purposes.
  2. reviewboard/accounts/backends.py (Diff revision 3)
     
     
    Please make sure all lines wrap to about 79 characters.
    
    Also, blank line before this.
  3. 
      
Review request changed

Change Summary:

Fixed formatting as per Christian's instructions. I am assuming the 79 characters starts from the beginning of the comment as this would result in absurdly formatted comments for deeply indented sections.

Diff:

Revision 4 (+5 -1)

Show changes

  1. Christian,
    
    The only scenario I can think of in which this would cause a problem for existing installations is if the sAMAccountName for the group with the cn specified by memberOf does not match that cn, but a group with a sAMAccountName identical to the first group's cn exists - in which case the group returned by the search is not actually the group specified by memberOf.
    1. To clarify: in the situation that I described, authentication for users might succeed currently but would fail if this patch is applied. I don't believe that this is a reason not to change the behavior as currently those users are authenticated because RB believes they have group memberships the users don't actually have.
    2. Fair enough. Thanks.
  2. 
      
chipx86
  1. 
      
  2. reviewboard/accounts/backends.py (Diff revision 4)
     
     
     
     
    It actually is 79 from the start of the line. We have our terminals set to 80 characters and don't like to see wrapping.
    
    Not a big deal though. I'll format before committing. Thanks!
  3. 
      
chipx86
  1. Committed to release-1.0.x (r88ab896)
  2. 
      
Loading...