owncloud / brute_force_protection

Brute-force protection app for ownCloud
GNU General Public License v2.0
6 stars 5 forks source link

LinkAuthException should inherit from LoginException for proper catching on core #135

Closed karakayasemi closed 4 years ago

karakayasemi commented 4 years ago

To be able to show error message and return correct return response, LinkAuthException should inherit from LoginException

codecov[bot] commented 4 years ago

Codecov Report

Merging #135 into release-1.1.0 will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff                @@
##             release-1.1.0     #135   +/-   ##
================================================
  Coverage            71.87%   71.87%           
  Complexity              53       53           
================================================
  Files                   13       13           
  Lines                  256      256           
================================================
  Hits                   184      184           
  Misses                  72       72           

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 00d387b...2bc1387. Read the comment docs.

phil-davis commented 4 years ago

Looks good: Public-link-brute-force-password 2020-09-13 10-11-29

But I see that underneath there is a 503 status returned. Maybe issue #112 is related? The bug-demo scenarios at https://github.com/owncloud/brute_force_protection/blob/master/tests/acceptance/features/apiBruteForceProtection/bruteforceprotection.feature#L92 are still passing. I was hoping that maybe they would also "fix themselves"?!

Is the fix for #112 in some similar area and could also be done? Or is it a very different code path?

karakayasemi commented 4 years ago

@phil-davis thank you for the review. The following part of the core taking care of this exception on Web UI. So, I needed to choose one of the caught exceptions in there.

https://github.com/owncloud/core/blob/640bba7ea32ace3be7d2eed94f6090054ee95988/index.php#L55-L70

I chose LoginException among them to return 403 as status response. Also, OCS api response is http 401 for LoginException.

As far as I understand for #112, the new public WebDAV API does not catch the LoginException and returns http 500 for it. I guess, this issue should resolve on core within different PR. Out of scope for this PR.

phil-davis commented 4 years ago

Looks good now - 403 "forbiddden" is returned.

Screenshot from 2020-09-14 11-47-29