Skip to content
This repository has been archived by the owner on Oct 31, 2018. It is now read-only.

block ip+uid combination instead of only ip #29

Merged
merged 2 commits into from
Jan 10, 2018
Merged

Conversation

karakayasemi
Copy link
Contributor

implementation of #27

@DeepDiver1975
Copy link
Contributor

Nice! THX a lot!

@codecov-io
Copy link

codecov-io commented Dec 20, 2017

Codecov Report

Merging #29 into master will increase coverage by 1.19%.
The diff coverage is 85.18%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #29      +/-   ##
============================================
+ Coverage     70.56%   71.76%   +1.19%     
- Complexity       70       73       +3     
============================================
  Files            12       12              
  Lines           282      301      +19     
============================================
+ Hits            199      216      +17     
- Misses           83       85       +2
Impacted Files Coverage Δ Complexity Δ
lib/Db/DbService.php 100% <100%> (ø) 8 <2> (+2) ⬆️
lib/Throttle.php 89.47% <50%> (-10.53%) 7 <4> (+1)
lib/Hooks.php 81.81% <66.66%> (ø) 6 <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 6e8eaf2...d43a5f1. Read the comment docs.

@DeepDiver1975
Copy link
Contributor

no database schema change required? @karakayasemi

@karakayasemi
Copy link
Contributor Author

No change required for this PR. We are already storing uid in database.

Copy link

@individual-it individual-it left a comment

Choose a reason for hiding this comment

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

works generally but after a successful access from that IP the blacklist is cleared for the IP not the IP/UID combination
https://github.com/owncloud/security/blob/8d3c1b4bd824c1f398933f5b62e338c9d2d2416f/lib/Db/DbService.php#L142

@individual-it
Copy link

@karakayasemi still does not work for me. In Hooks.php you still calling the old delete function

    public function postLoginCallback() {
        $this->throttle->clearSuspiciousAttemptsForIp($this->request->getRemoteAddress());
    }

some end-to-end tests would be good here to test all this cases

@karakayasemi
Copy link
Contributor Author

@individual-it Thanks for your help. I believe, I solved the problem this time. I have no experience in end-to-end testing, but I would like to add. Where should these tests place in the folder structure?

@individual-it
Copy link

@karakayasemi we put the usually into tests/ui
Currently we are using Behat & Mink to run UI tests on travis and saucelabs.com
The documentation about core UI tests can be found here https://doc.owncloud.org/server/10.0/developer_manual/core/ui-testing.html
and an example how the setup works for an app in the texteditor app https://github.com/owncloud/files_texteditor
I'm happy to help if you have any questions

@karakayasemi
Copy link
Contributor Author

@DeepDiver1975 I guess we can merge this. I will create a new issue for end-to-end tests.

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

Successfully merging this pull request may close these issues.

None yet

6 participants