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

emit events for before-after-failed link password checks #37438

Merged
merged 1 commit into from
May 26, 2020

Conversation

karakayasemi
Copy link
Contributor

Description

This PR adds 3 new events (before-fail-after) for link password validations.

Related Issue

How Has This Been Tested?

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
  • Changelog item

@karakayasemi karakayasemi added this to the development milestone May 22, 2020
@karakayasemi karakayasemi self-assigned this May 22, 2020
@update-docs
Copy link

update-docs bot commented May 22, 2020

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@karakayasemi karakayasemi force-pushed the check-link-password-events branch 2 times, most recently from e1a5cca to 1dc6cbc Compare May 22, 2020 14:59
@mmattel
Copy link
Contributor

mmattel commented May 23, 2020

Does this need a doc change/addon in the developer section ?
If so, pls file a issue in docs.

@karakayasemi
Copy link
Contributor Author

@mmattel the Symfony events never documented, there is an open issue about it but it seems inactive for a long time. See owncloud/docs#311

I will update the mentioned documentation issue with these events after the merge anyway. Thank you for reminding me.

@codecov
Copy link

codecov bot commented May 24, 2020

Codecov Report

Merging #37438 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #37438   +/-   ##
=========================================
  Coverage     64.68%   64.68%           
  Complexity    19331    19331           
=========================================
  Files          1277     1277           
  Lines         75506    75512    +6     
  Branches       1331     1331           
=========================================
+ Hits          48838    48844    +6     
  Misses        26276    26276           
  Partials        392      392           
Flag Coverage Δ Complexity Δ
#javascript 54.14% <ø> (ø) 0.00 <ø> (ø)
#phpunit 65.84% <100.00%> (+<0.01%) 19331.00 <0.00> (ø)
Impacted Files Coverage Δ Complexity Δ
lib/private/Share20/Manager.php 96.19% <100.00%> (+0.03%) 273.00 <0.00> (ø)

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 c0d1351...0dfa157. Read the comment docs.

lib/private/Share20/Manager.php Outdated Show resolved Hide resolved
@@ -3151,17 +3151,50 @@ public function testCheckPasswordInvalidPassword() {

$this->hasher->method('verify')->with('invalidpassword', 'password', '')->willReturn(false);

$calledBeforeEvent = [];
$this->eventDispatcher->addListener('share.beforelinkpasswordcheck',
Copy link
Member

Choose a reason for hiding this comment

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

Just for the record (this was probably there already and changing it will take time), it seems better to use an interface and ensure that the "dispatch" method is called with the right parameters; no need to rely on an actual implementation.
As said, no need to make this adjustment this now.

@jvillafanez
Copy link
Member

Should we include a PHPDoc to clarify when the events will be thrown? It would help with the expectations in case something changes in the future.

@phil-davis phil-davis merged commit df64083 into master May 26, 2020
@delete-merged-branch delete-merged-branch bot deleted the check-link-password-events branch May 26, 2020 10:12
@phil-davis
Copy link
Contributor

Looks good. @karakayasemi I guess you will adjust brute_force_protection so that it will receive and respond to these?

@karakayasemi
Copy link
Contributor Author

Looks good. @karakayasemi I guess you will adjust brute_force_protection so that it will receive and respond to these?

Yes, I expect brute_force_protection will be aware of all share password validations now.

@phil-davis phil-davis mentioned this pull request May 26, 2020
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.

4 participants