Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[For 10.4] Add validation for sender email address #36505

Merged
merged 1 commit into from
Dec 5, 2019

Conversation

pako81
Copy link

@pako81 pako81 commented Dec 2, 2019

Description

This PR adds validation for the sender e-mail address when using the e-mail notification mechanism for public links as well as internal shares.

Motivation and Context

Given the following scenarios:

Scenario 1:

  • User1 shares a folder with User2
  • User1 does not have an e-mail address (NULL set in oc_accounts table)
  • User2 has a valid e-mail address
  • User1 clicks on the notify by email button: an error appears and the user cannot be notified

Scenario 2:

User1 (who does not have an email address) creates a brand new file or folder, creates a public link out of it and clicks on the Send link via email button in order to notify an external user. Also, in this case, the same error as above is returned.

How Has This Been Tested?

Manually by testing the two scenarios above and make sure no error is returned and e-mail is correctly delivered.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised
  • Changelog item

@pako81 pako81 requested review from micbar and karakayasemi December 2, 2019 12:59
@pako81
Copy link
Author

pako81 commented Dec 2, 2019

@owncloud/qa would be good to have this covered by tests.

@pako81 pako81 self-assigned this Dec 2, 2019
@pako81 pako81 added this to the development milestone Dec 2, 2019
@phil-davis
Copy link
Contributor

code style is failing, looks like indent issues - see https://drone.owncloud.com/owncloud/core/21653/2/6
do:

make test-php-style-fix

@phil-davis
Copy link
Contributor

@micbar if you think this can get into 10.4 then please add to the 10.4.0 project and we can add some acceptance tests this week.

@micbar
Copy link
Contributor

micbar commented Dec 2, 2019

@phil-davis Done.

@pako81 pako81 force-pushed the sender-email-validation-fix branch from b7a6feb to f8818bb Compare December 2, 2019 15:01
@phil-davis
Copy link
Contributor

@haribhandari07 please assign someone to write some relevant acceptance test scenarios

@pako81 pako81 force-pushed the sender-email-validation-fix branch from f8818bb to c769ff7 Compare December 2, 2019 15:08
@codecov
Copy link

codecov bot commented Dec 2, 2019

Codecov Report

Merging #36505 into master will increase coverage by <.01%.
The diff coverage is 85.71%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #36505      +/-   ##
============================================
+ Coverage     64.66%   64.66%   +<.01%     
- Complexity    19023    19025       +2     
============================================
  Files          1268     1268              
  Lines         74393    74398       +5     
  Branches       1311     1311              
============================================
+ Hits          48104    48108       +4     
- Misses        25903    25904       +1     
  Partials        386      386
Flag Coverage Δ Complexity Δ
#javascript 54.02% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 65.84% <85.71%> (ø) 19025 <2> (+2) ⬆️
Impacted Files Coverage Δ Complexity Δ
lib/private/Share/MailNotifications.php 96.19% <85.71%> (-0.46%) 33 <2> (+2)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 471637d...aa63dc0. Read the comment docs.

@haribhandari07 haribhandari07 self-assigned this Dec 3, 2019
@pako81 pako81 mentioned this pull request Dec 3, 2019
@pako81 pako81 force-pushed the sender-email-validation-fix branch from c769ff7 to c4c59aa Compare December 3, 2019 15:22
@pako81 pako81 force-pushed the sender-email-validation-fix branch 2 times, most recently from b0c99e9 to 527b906 Compare December 3, 2019 20:31
@phil-davis
Copy link
Contributor

https://drone.owncloud.com/owncloud/core/21690/3/7

------ ----------------------------------------- 
  Line   lib/private/Share/MailNotifications.php  
 ------ ----------------------------------------- 
  186    Undefined variable: $senderMailAdress    
  267    Undefined variable: $senderMailAdress    
 ------ ----------------------------------------- 

 [ERROR] Found 2 errors                                                         

@phil-davis
Copy link
Contributor

@haribhandari07 please be careful adding a commit with tests. This branch is still being force-pushed so it will be easy for your commit/code to get lost. Maybe work in a branch on top of this.

@phil-davis
Copy link
Contributor

@pako81 the acceptance tests added by @haribhandari07 have disappeared with your force push.
We need to first pull whatever is on GitHub, then make changes and push.

@haribhandari07 please find your local commit with the tests and add it again to this PR.

Copy link
Contributor

@karakayasemi karakayasemi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good.

Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will squash the commits.

@phil-davis phil-davis force-pushed the sender-email-validation-fix branch from 5d9321b to aa63dc0 Compare December 5, 2019 08:57
@phil-davis phil-davis changed the title Add validation for sender email address [For 10.4] Add validation for sender email address Dec 5, 2019
@phil-davis phil-davis merged commit 9fae369 into master Dec 5, 2019
@delete-merged-branch delete-merged-branch bot deleted the sender-email-validation-fix branch December 5, 2019 11:02
@pako81
Copy link
Author

pako81 commented Dec 5, 2019

thanks @phil-davis @micbar @karakayasemi @haribhandari07 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants