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

add before-after share link auth events #34158

Merged
merged 1 commit into from
Feb 7, 2019
Merged

Conversation

karakayasemi
Copy link
Contributor

Description

Adds before and after events for public share link auths.

Related Issue

The first step for:
#33542, owncloud/brute_force_protection#57

Motivation and Context

To be able to detect and interrupt public link share auth attempts.

How Has This Been Tested?

Unit test and manually.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

I am not sure about the location of the events. As far as I see, adding events in linkShareAuth function is enough. But, if you see any missing place, please let me know.

When this pr merged, I will update here: owncloud/docs#311 (comment)

@karakayasemi karakayasemi self-assigned this Jan 16, 2019
@karakayasemi karakayasemi force-pushed the add-sharelink-event branch 2 times, most recently from 8821b4e to 8fc3033 Compare January 19, 2019 13:54
@codecov
Copy link

codecov bot commented Jan 19, 2019

Codecov Report

Merging #34158 into master will decrease coverage by 16.21%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #34158       +/-   ##
=============================================
- Coverage     64.76%   48.55%   -16.22%     
=============================================
  Files          1198      109     -1089     
  Lines         69467    10525    -58942     
  Branches       1281     1281               
=============================================
- Hits          44989     5110    -39879     
+ Misses        24105     5042    -19063     
  Partials        373      373
Flag Coverage Δ Complexity Δ
#javascript 53.09% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 38.63% <ø> (-27.49%) 0 <ø> (-18351)
Impacted Files Coverage Δ Complexity Δ
lib/private/Files/Storage/DAV.php 59.45% <0%> (-21.64%) 0% <0%> (ø)
apps/updatenotification/templates/admin.php
lib/private/Encryption/Keys/Storage.php
lib/private/App/CodeChecker/NodeVisitor.php
lib/private/RedisFactory.php
apps/dav/lib/Avatars/AvatarNode.php
...s/dav/appinfo/Migrations/Version20170202213905.php
apps/dav/lib/Upload/ChunkLocationProvider.php
apps/files/lib/AppInfo/Application.php
apps/systemtags/list.php
... and 1079 more

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 3515439...8fc3033. Read the comment docs.

@codecov
Copy link

codecov bot commented Jan 19, 2019

Codecov Report

Merging #34158 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #34158      +/-   ##
============================================
+ Coverage     64.76%   64.77%   +<.01%     
  Complexity    18351    18351              
============================================
  Files          1198     1198              
  Lines         69467    69471       +4     
  Branches       1281     1281              
============================================
+ Hits          44989    44997       +8     
+ Misses        24105    24101       -4     
  Partials        373      373
Flag Coverage Δ Complexity Δ
#javascript 53.09% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 66.12% <100%> (ø) 18351 <0> (ø) ⬇️
Impacted Files Coverage Δ Complexity Δ
.../files_sharing/lib/Controllers/ShareController.php 54.37% <100%> (+0.85%) 54 <0> (ø) ⬇️
lib/private/Files/View.php 84.6% <0%> (+0.3%) 398% <0%> (ø) ⬇️
lib/private/DB/Connection.php 66.17% <0%> (+0.73%) 49% <0%> (ø) ⬇️

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 3515439...4fe6d6f. Read the comment docs.

@phil-davis
Copy link
Contributor

Unrelated drone CI fail. I restarted drone, and raised issue #34186 to look into why this is happening too often.

@karakayasemi
Copy link
Contributor Author

@phil-davis it failed again. I tried to restart the build one more time with drone.io's API, but I could not. Have you any suggestion to make CI green.

@phil-davis
Copy link
Contributor

@ownclouders rebase

@ownclouders
Copy link
Contributor

Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently ⚠️

@ownclouders
Copy link
Contributor

Automated rebase with GitMate.io was successful! 🎉

@phil-davis
Copy link
Contributor

I restart the drone build and it failed again. But I made a fresh PR #34187 (not this code) just to check the drone CI - that passed first time.

Either this code really does make some difference to the test, or something goes wrong because the failing scenarios were only recently merged to master and this PR is a few days old. I tried a rebase, maybe that will help??? But I see that you force-pushed only 1 day ago.

@karakayasemi
Copy link
Contributor Author

karakayasemi commented Jan 20, 2019

I forced pushed 1 day ago to re-trigger ci with only changing commit message. I hope rebase will work.

@phil-davis
Copy link
Contributor

Special CI magic - it passed!

@PVince81 PVince81 added this to the development milestone Jan 28, 2019
Copy link
Contributor

@sharidas sharidas left a comment

Choose a reason for hiding this comment

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

Kindly have a look at my comment.

@karakayasemi
Copy link
Contributor Author

The event dispatcher injected to ShareController class as a non-mock object in here:

$this->eventDispatcher = new EventDispatcher();

I can inject a mock EventDispatcher object and update my tests, but there are other events which are not related to this PR in this class. I should refactor their tests also.

@sharidas both of the cases fine for me. What do you think?

@sharidas
Copy link
Contributor

sharidas commented Feb 6, 2019

@sharidas both of the cases fine for me. What do you think?

Humm there are more calls to addListener() . Well just leave it then. At least we can make sure for the new test files we can mock the EventDispatcherInterface and use it in our tests.

Copy link
Contributor

@sharidas sharidas left a comment

Choose a reason for hiding this comment

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

Looks fine to me 👍

@PVince81 PVince81 merged commit 7524c78 into master Feb 7, 2019
@delete-merged-branch delete-merged-branch bot deleted the add-sharelink-event branch February 7, 2019 08:16
@PVince81
Copy link
Contributor

PVince81 commented Feb 7, 2019

@karakayasemi please backport to stable10

@phil-davis
Copy link
Contributor

Backport stable10 #34399

@lock lock bot locked as resolved and limited conversation to collaborators Mar 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants