Add config option for always sending emails from server email.

 






 

 


  Review Request #9209
  —
  Created Sept. 22, 2017 and submitted 

 


















 

  

   Information

  





 
 

  bleblan2
 









 
 

  Review Board
 









 
 

  release-3.0.x
 









 
 

  4578
 









 
 

  9216
 









 
 

  175f7c2...
 






 




 

  

   Reviewers

  





 
 

  reviewboard, students
 









 
 

  
 






 








 





  


 
 

  
Review Board tries to send e-mails on behalf of users by spoofing their

e-mail address, in order to offer better comment threads. While it tries

to be smart about when to do this (by checking DNS records to see whether

it can safely spoof the e-mail address), it's sometimes ideal to simply

turn off this behavior and send from the server's configured e-mail

address instead.


This change adds an option to force the server's e-mail address to be

used for all e-mails. When enabled, e-mails will still appear to come

from the user, but only by name (using "<Name> via Review Board

server@email" as the From address).


In order for this change to correctly send e-mails using the "<Name> via

Review Board server@email" format, the mail message in djblets must be

updated to send e-mails using that format when email_smart_spoofing is

set to False.

 








  


 
 

  
The notifications tests were updated to check that the email used is

settings.DEFAULT_FROM_EMAIL instead of settings.SERVER_EMAIL. The

reviewboard.notifications.tests suite was run. Tests added for new

review emails and review reply emails with enable_smart_spoofing

disabled.

 







 









 





  








 






 
 
 

  

   

    

     
    

    
    
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
    

   

   

    
     

      Description
      From
      Last Updated
     

    
    



     

      
       
       
The description should wrap at 70-75 characters. I'd like to see some changes to how this is described. How about: …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
This change needs unit tests that cover the new option. There should be tests for loading/saving the option (based on …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Not sure whether we should be switching the default to SERVER_EMAIL in this change. That will have an impact on …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
E501 line too long (82 > 79 characters)

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
E501 line too long (82 > 79 characters)

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
E501 line too long (82 > 79 characters)

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
E501 line too long (82 > 79 characters)

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
I'd suggest reversing this option and making it about sending on behalf of users, enabled by default. So something like …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Missing a period at the end of the sentence.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Blank line between these. You want one separating block-creating statements (like an if/else) from other code (like an assignment).

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
EmailMessage takes an enable_smart_spoofing option, which you should turn off based on the setting. Otherwise, it's going to try to …

      		
      
       chipx86chipx86
      
      
       
      
     


    
   

  

 







 






  

 



















 




 

  


   

   

    
 
Checks run (1 failed, 1 succeeded)


    
   


  

  

  





 

 flake8
 failed.
 





 

 JSHint
 passed.
 













 
flake8

 


 










  

 





 














 



 

  

   




bleblan2




   

   

  

 



 















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 







  2. 
 
    
  
  
    
  
   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    The description should wrap at 70-75 characters.
    

    I'd like to see some changes to how this is described. How about:
    

    Review Board tries to send e-mails on behalf of users by spoofing their e-mail
address, in order to offer better comment threads. While it tries to be smart
about when to do this (by checking DNS records to see whether it can safely
spoof the e-mail address), it's sometimes ideal to simply turn off this behavior
and send from the server's configured e-mail address instead.

This change adds an option to force the server's e-mail address to be used for
all e-mails. When enabled, e-mails will still appear to come from the user, but
only by name (using "<Name> via Review Board <server@email>" as the From address).

    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 





  3. 
 
    
  
  
    
  
   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    This change needs unit tests that cover the new option. There should be tests for loading/saving the option (based on my suggestions in this review), and for sending e-mail with spoofing disabled.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 





  4. 
 
    
  
  
    
  
   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Not sure whether we should be switching the default to SERVER_EMAIL in this change. That will have an impact on existing Review Board installs. Best to keep this change solely focused on the new option instead. In general, you want your changes to be heavily focused and not do "Also fixed ..." work (this isn't a black-and-white rule, but is a good goal).
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I added the change from settings.SERVER_EMAIL to settings.DEFAULT_FROM_EMAIL on @brennie's suggestion. According to @brennie, the root@localhost email is showing up on production.
      
 
      

      2. 


 
    


    

 
    

    5. 















  5. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/admin/forms.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    I'd suggest reversing this option and making it about sending on behalf of users, enabled by default. So something like mail_enable_smart_spoofing (to keep consistent with internal terminology and changing the label/help text accordingly.
    

    Right now, we actually have something in setting.py for this, which is EMAIL_ENABLE_SMART_SPOOFING. Some companies are going to have this turn on/off, so we should back the siteconfig value from this setting and sync both ways (look in the settings_map variable in admin/siteconfig.py).
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 






  6. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/admin/forms.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Missing a period at the end of the sentence.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 






  7. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/notifications/email/message.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Blank line between these. You want one separating block-creating statements (like an if/else) from other code (like an assignment).
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    8. 






  8. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/notifications/email/message.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    EmailMessage takes an enable_smart_spoofing option, which you should turn off based on the setting. Otherwise, it's going to try to see if it needs to spoof the default header instead.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    9. 




  9. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    10. 




  

 
















 



 

  

   




bleblan2




   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Making some small updates and landing. Thanks!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




bleblan2




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 




 
Change Summary:

 
Pushed to release-3.0.x (b75e73c)





 


 










  

 









     

    

   

  

  

   
   Loading...