Protect public links password page #90

karakayasemi · 2019-11-14T13:41:53Z

Fixes #57

Legacy OC\User hooks changed with EventDispatcher events
Brute force protection added for public link share authentications.
Brute force protection settings now common for login and public link share auth.

codecov · 2019-11-14T17:51:35Z

Codecov Report

Merging #90 into master will increase coverage by 1.87%.
The diff coverage is 76.51%.

@@             Coverage Diff              @@
##             master      #90      +/-   ##
============================================
+ Coverage     70.00%   71.87%   +1.87%     
- Complexity       39       53      +14     
============================================
  Files            11       13       +2     
  Lines           170      256      +86     
============================================
+ Hits            119      184      +65     
- Misses           51       72      +21

Impacted Files	Coverage Δ	Complexity Δ
appinfo/Migrations/Version20191109111104.php	`0.00% <0.00%> (ø)`	`2.00 <2.00> (?)`
lib/BruteForceProtectionConfig.php	`100.00% <ø> (ø)`	`8.00 <0.00> (ø)`
templates/settings-admin.php	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
lib/Throttle.php	`93.33% <91.30%> (+1.33%)`	`11.00 <10.00> (+3.00)`
lib/Db/FailedLinkAccessMapper.php	`100.00% <100.00%> (ø)`	`6.00 <6.00> (?)`
lib/Db/FailedLoginAttemptMapper.php	`100.00% <100.00%> (ø)`	`6.00 <2.00> (ø)`
lib/Hooks.php	`100.00% <100.00%> (+17.64%)`	`8.00 <6.00> (+3.00)`
lib/Jobs/ExpireOldAttempts.php	`100.00% <100.00%> (ø)`	`2.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 a142614...a56cce2. Read the comment docs.

karakayasemi · 2019-12-06T11:33:22Z

@DeepDiver1975 when you have time, would you like to review this community request implementation?

mmattel · 2020-01-15T20:13:52Z

Any update?

karakayasemi · 2020-01-24T10:58:08Z

@mmattel I need to rebase and resolve conflicts. After that, you can help me with testing the pr. Finally, we can merge this after your approval.

mmattel · 2020-02-04T09:20:52Z

@karakayasemi ping ?

karakayasemi · 2020-02-04T11:48:51Z

@mmattel, I rebased the PR and resolved conflicts. The PR is all green again and ready to merge after an approval. You can test the feature now.
Actually, I made quite some changes in codebase. A second eye on the code would be good. Thanks for helping me.

phil-davis

It's very good that the existing acceptance tests pass.
We should add some acceptance tests to demonstrate what happens when someone tries to brute-force a public link.

lib/Db/FailedLinkAccessMapper.php

phil-davis · 2020-02-04T14:23:27Z

I added QA-team label etc. Someone can look at adding acceptance tests tomorrow - maybe @haribhandari07 you were already working in this app this week.

haribhandari07 · 2020-02-06T03:56:44Z

I added QA-team label etc. Someone can look at adding acceptance tests tomorrow - maybe @haribhandari07 you were already working in this app this week.

I am on it @phil-davis

phil-davis · 2020-02-27T04:23:40Z

@haribhandari07 what is the status?
Please assign yourself if you are working on this, or talk to the scrum-master to get someone else assigned.

phil-davis · 2020-03-16T04:22:27Z

@haribhandari07 what happened so far?

appinfo/Migrations/Version20191109111104.php

VicDeo · 2020-04-02T09:07:31Z

appinfo/Migrations/Version20191109111104.php

@@ -0,0 +1,39 @@
+<?php
+namespace OCA\brute_force_protection\Migrations;


Any reason why brute_force_protection is not in CamelCase?

MigrationService cannot find migration classes that has '_' character in app name, it may indicate a bug on core. Because of that, I have been followed same syntax with previous migration class of this app.

phil-davis · 2020-05-26T12:43:44Z

Probably we will need to wait overnight until the new daily-master-qa tarball is generated. That will have the core changes in it. (But maybe you get lucky and the changes already work in CI?)

phil-davis · 2020-05-26T12:46:47Z

https://drone.owncloud.com/owncloud/brute_force_protection/973/17/7

The app now requires core 10.5 or later. So the drone test matrix needs to be adjusted.
.drone.star around in the acceptance webUI` section around line 48 needs:

			'servers': [
				'daily-master-qa'
			],

so that it only tests against daily-master-qa and not against latest (which is 10.4.1)

phil-davis · 2020-05-27T03:40:13Z

I added a commit to remove testing against latest 10.4.1

phil-davis · 2020-05-27T05:20:40Z

https://drone.owncloud.com/owncloud/brute_force_protection/975/17/10

  Scenario: access to public link is not blocked after too many invalid requests                                                                                                                               # /var/www/owncloud/testrunner/apps/brute_force_protection/tests/acceptance/features/apiBruteForceProtection/bruteforceprotection.feature:91
    Given user "Alice" has uploaded file with content "user1 file" to "/PARENT/randomfile.txt"                                                                                                                 # FeatureContext::userHasUploadedAFileWithContentTo()
    When user "Alice" creates a public link share using the sharing API with settings                                                                                                                          # FeatureContext::userCreatesAPublicLinkShareWithSettings()
      | path     | PARENT   |
      | password | %public% |
    And the public download of the last publicly shared file using the new public WebDAV API with password "12345" should fail with HTTP status code "401"                                                     # PublicWebDavContext::theLastPublicSharedFileShouldNotBeAbleToBeDownloadedWithPassword()
    And the public download of the last publicly shared file using the new public WebDAV API with password "12345" should fail with HTTP status code "401"                                                     # PublicWebDavContext::theLastPublicSharedFileShouldNotBeAbleToBeDownloadedWithPassword()
    And the public download of the last publicly shared file using the new public WebDAV API with password "123455" should fail with HTTP status code "401"                                                    # PublicWebDavContext::theLastPublicSharedFileShouldNotBeAbleToBeDownloadedWithPassword()
      HTTP status code 500 is not the expected value 401
      Failed asserting that 500 matches expected '401'.
    And the public should be able to download file "/randomfile.txt" from inside the last public shared folder using the new public WebDAV API with password "%public%" and the content should be "user1 file" # PublicWebDavContext::shouldBeAbleToDownloadFileInsidePublicSharedFolderWithPassword()

--- Failed scenarios:

    /var/www/owncloud/testrunner/apps/brute_force_protection/tests/acceptance/features/apiBruteForceProtection/bruteforceprotection.feature:91

34 scenarios (33 passed, 1 failed)
222 steps (220 passed, 1 failed, 1 skipped)

This is failing due to issue #112 - I will adjust the test...

phil-davis · 2020-05-27T11:50:37Z

I pushed more commits for acceptance test changes. I will see the CI result, and then rebase/squash/cleanup the commits into something more logical.

…orce

phil-davis · 2020-05-27T12:51:42Z

Commits squashed. Works with core 10.5.0. Ready for developer review.

appinfo/Migrations/Version20191109111104.php

lib/Db/FailedLinkAccessMapper.php

lib/Throttle.php

jvillafanez · 2020-05-27T16:42:17Z

lib/Throttle.php

+		if ($this->linkAccessMapper->getFailedAccessCountForTokenIpCombination($token, $ip) >=
+			$this->config->getBruteForceProtectionFailTolerance() &&
+			$banUntil > $this->timeFactory->getTime()) {
+			throw new LinkAuthException($this->l->t("Too many failed attempts. Try again in %s.",


It's questionable to have translated messages in the exception.

If the exception isn't caught this likely means that any other exception that could happen around here won't be caught neither, with the risk of exposing unneeded info

If it's caught, it should be the responsibility of whoever caught the exception (probably a controller) to provide a proper response with the translated message.

@jvillafanez I agree with your point, but I do not think we can easily implement consistent exception catching mechanism since an event can be dispatched from multiple places and multiple endpoints.
I followed the same way with login protection here. As far as I know, we have some translated exceptions like HintException in the codebase also. I would suggest keeping this one in that way for now to reduce effort.

ok, you might want to include a comment instead so we know the text will be shown to the users

tests/unit/Db/FailedLinkAccessMapperTest.php

jvillafanez · 2020-05-27T16:58:03Z

Just saying there are some new files with copyright of 2019... we should adjust that date

phil-davis · 2020-05-31T15:45:00Z

drone CI got the "random" issue #98
https://drone.owncloud.com/owncloud/brute_force_protection/989/17/10
I will restart drone.

karakayasemi · 2020-06-01T09:15:23Z

drone CI got the "random" issue #98
https://drone.owncloud.com/owncloud/brute_force_protection/989/17/10
I will restart drone.

Looks like the random issue is not random anymore. I could not get a green CI with many attempts.

karakayasemi · 2020-06-08T08:23:39Z

@jvillafanez CI is green, I corrected most of the issues that you mention in your review. Please, review it again. Thank you.

karakayasemi force-pushed the protect-shares branch 3 times, most recently from 901dcff to 253f355 Compare November 14, 2019 17:51

karakayasemi force-pushed the protect-shares branch 2 times, most recently from e22ee09 to ea11b0b Compare November 15, 2019 09:15

karakayasemi self-assigned this Nov 15, 2019

karakayasemi added the 3 - To review label Nov 16, 2019

karakayasemi requested a review from DeepDiver1975 December 6, 2019 11:32

karakayasemi force-pushed the protect-shares branch from ea11b0b to bc41b02 Compare February 4, 2020 11:33

karakayasemi force-pushed the protect-shares branch 3 times, most recently from fd69463 to 0a03d37 Compare February 4, 2020 13:00

phil-davis reviewed Feb 4, 2020

View reviewed changes

lib/Db/FailedLinkAccessMapper.php Outdated Show resolved Hide resolved

karakayasemi force-pushed the protect-shares branch from 0a03d37 to ba649fe Compare February 4, 2020 14:20

phil-davis added QA:p2 QA:team labels Feb 4, 2020

haribhandari07 requested review from haribhandari07 and removed request for haribhandari07 February 6, 2020 03:54

haribhandari07 mentioned this pull request Mar 17, 2020

Public share protect #111

Closed

VicDeo reviewed Apr 2, 2020

View reviewed changes

haribhandari07 and others added 2 commits May 27, 2020 18:33

[Tests-only] Add acceptance tests to protect public share from brutef…

9a8aa72

…orce

Bump minor version and update min server version

14c28fb

phil-davis force-pushed the protect-shares branch from 8d1dd61 to 14c28fb Compare May 27, 2020 12:49

phil-davis requested a review from VicDeo May 27, 2020 12:49

phil-davis removed QA:p2 QA:team labels May 27, 2020

phil-davis requested a review from jvillafanez May 27, 2020 12:50

phil-davis mentioned this pull request May 27, 2020

Public share protect 5 #125

Closed

jvillafanez reviewed May 27, 2020

View reviewed changes

refactor db mapper and throttle classes

a56cce2

karakayasemi force-pushed the protect-shares branch from 54a19ee to a56cce2 Compare May 31, 2020 14:31

karakayasemi requested a review from jvillafanez June 8, 2020 08:22

jvillafanez approved these changes Jun 8, 2020

View reviewed changes

karakayasemi merged commit 5f83f8f into master Jun 12, 2020

delete-merged-branch bot deleted the protect-shares branch June 12, 2020 09:06

mmattel mentioned this pull request Jun 12, 2020

core 10.3 is out - make new BF release available ? #85

Closed

phil-davis mentioned this pull request Jun 12, 2020

brute_force_protection 1.1.0 #129

Closed

2 tasks

karakayasemi mentioned this pull request Jun 12, 2020

Document share link protection feature of brute force protection app owncloud/docs#2682

Open

2 tasks

HanaGemela added this to the QA milestone Jun 19, 2020

davitol mentioned this pull request Sep 3, 2020

Tracking Testing Results for Brute Force Protection 1.1.0. #132

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Protect public links password page #90

Protect public links password page #90

karakayasemi commented Nov 14, 2019

codecov bot commented Nov 14, 2019 •

edited

Loading

karakayasemi commented Dec 6, 2019

mmattel commented Jan 15, 2020

karakayasemi commented Jan 24, 2020

mmattel commented Feb 4, 2020

karakayasemi commented Feb 4, 2020 •

edited

Loading

phil-davis left a comment

phil-davis commented Feb 4, 2020

haribhandari07 commented Feb 6, 2020

phil-davis commented Feb 27, 2020

phil-davis commented Mar 16, 2020

VicDeo Apr 2, 2020

karakayasemi Apr 11, 2020

phil-davis commented May 26, 2020

phil-davis commented May 26, 2020

phil-davis commented May 27, 2020

phil-davis commented May 27, 2020

phil-davis commented May 27, 2020

phil-davis commented May 27, 2020 •

edited

Loading

jvillafanez May 27, 2020

karakayasemi Jun 8, 2020

jvillafanez Jun 8, 2020

jvillafanez commented May 27, 2020

phil-davis commented May 31, 2020

karakayasemi commented Jun 1, 2020 •

edited

Loading

karakayasemi commented Jun 8, 2020

		@@ -0,0 +1,39 @@
		<?php
		namespace OCA\brute_force_protection\Migrations;

Protect public links password page #90

Protect public links password page #90

Conversation

karakayasemi commented Nov 14, 2019

codecov bot commented Nov 14, 2019 • edited Loading

Codecov Report

karakayasemi commented Dec 6, 2019

mmattel commented Jan 15, 2020

karakayasemi commented Jan 24, 2020

mmattel commented Feb 4, 2020

karakayasemi commented Feb 4, 2020 • edited Loading

phil-davis left a comment

Choose a reason for hiding this comment

phil-davis commented Feb 4, 2020

haribhandari07 commented Feb 6, 2020

phil-davis commented Feb 27, 2020

phil-davis commented Mar 16, 2020

VicDeo Apr 2, 2020

Choose a reason for hiding this comment

karakayasemi Apr 11, 2020

Choose a reason for hiding this comment

phil-davis commented May 26, 2020

phil-davis commented May 26, 2020

phil-davis commented May 27, 2020

phil-davis commented May 27, 2020

phil-davis commented May 27, 2020

phil-davis commented May 27, 2020 • edited Loading

jvillafanez May 27, 2020

Choose a reason for hiding this comment

karakayasemi Jun 8, 2020

Choose a reason for hiding this comment

jvillafanez Jun 8, 2020

Choose a reason for hiding this comment

jvillafanez commented May 27, 2020

phil-davis commented May 31, 2020

karakayasemi commented Jun 1, 2020 • edited Loading

karakayasemi commented Jun 8, 2020

codecov bot commented Nov 14, 2019 •

edited

Loading

karakayasemi commented Feb 4, 2020 •

edited

Loading

phil-davis commented May 27, 2020 •

edited

Loading

karakayasemi commented Jun 1, 2020 •

edited

Loading