Supporting dashboard impersonations

 






 

 


  Review Request #3207
  —
  Created July 12, 2012 and discarded 

 


















 

  

   Information

  





 
 

  atagar
 









 
 

  Review Board
 









 
 

  
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
Around a year back I made a little hack so that administrators could see what
users do on their dashboard via a 'user' parameter. This has come in handy for
diagnosing issues on several occasions so checking if you would like to have it
upstream.

This is available in my 'dashboard_impersonation' branch...
https://github.com/atagar/ReviewBoard/tree/dashboard_impersonation


 








  


 
 

  
We've been running this change with ReviewBoard 1.5, and it hasn't been
exercised with 1.6. To run it simply provide a user argument such as...
http://reviews.reviewboard.org/dashboard/?user=atagar


 







 









 





  








 









 






  

 








 














 





 

  

   




chipx86




   

   





  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Thanks for the patch, but I'm pretty concerned about monkey-patching request.user like that, as it could interfere with other bits of code. I'd rather have proper support for passing a desired user (defaulting to request.user) through the dashboard code.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Sorry about the delay, been juggling lots of things at work. Setting the requester.user seems like the cleanest way of making all following code faithfully behave as though the user was something else. Are you suggesting plumbing through a 'user' argument throughout the codebase?

      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      Not the whole codebase, but what we really care about are the queries for the dashboard and counts, right? I haven't looked too deeply into this, but I'd think we could keep it mostly contained to the dashboard's datagrid class. If there are functions we call from there where a user makes more sense than a request, I'm up for changing them.
      
 
      

      3. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      > Not the whole codebase, but what we really care about are the queries for the dashboard and counts, right?

Usually. Most questions I've received are around group membership or the counts. I could think of some other things the datagrid wouldn't cover, for instance "ReviewBoard is showing the wrong name in the upper right corner" or "I'm a first time user and it's redirecting me to the settings page" (believe it or not we've gotten two or three tickets about that). However, those aren't things I'd need impersonation for.

I'll look into passing the argument to the datagrid, though it'll be a few weeks before I can dedicate time to that.
      
 
      

      4. 



    4. 
 
 
      
 
      
  
 
      
 
      
  
      Viewing this as a tool for use by an administrator to reproduce user experience (and as a ReviewBoard instance administrator), I actually prefer an approach that replaces requester.user . It seems like changing the dashboard's datagrid class won't get you the same "this is what they see" sort of experience.
      
 
      

      5. 



    5. 
 
 
      
 
      
  
 
      
 
      
  
      I've continued to use this as-is for the last couple months and I'm inclined to agree with Josh. This is the simplest and most faithful method of providing impersonation support I've spotted. If this really isn't of interest upstream then feel free to flag this review as discarded.
      
 
      

      6. 



    6. 
 
 
      
 
      
  
 
      
 
      
  
      Maybe we can make an extension out of this that adds this patch as a custom middleware. That way, sites can use it as required and it can be turned off at will.
      
 
      

      7. 



    7. 
 
 
      
 
      
  
 
      
 
      
  
      That would be fine with me. Chris, would that make you happy with merging this patch? If so then any hints on where I should look for an example on how this is done?
      
 
      

      8. 


 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   





 JS





   

   





  

 



 

  


   

   

    
 
jsintal


    
   


  

  

  



    




  1. 
 
    
  
    
   
    I tested this on (1.7 RC1) and it works as described. I noticed that there was disagreement of using this patch, after having tested it. Though I'd show my input.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   





 AT





   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Discarded

  
 







 


 










  

 









     

    

   

  

  

   
   Loading...