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

Introduce bruteforce settings #2095

Merged
merged 9 commits into from
Apr 4, 2017
Merged

Introduce bruteforce settings #2095

merged 9 commits into from
Apr 4, 2017

Conversation

rullzer
Copy link
Member

@rullzer rullzer commented Nov 11, 2016

This adds an app with adds an admin settings. Lets the admin insert ip ranges (netmask) to exclude for throttling.

TODO:

  • Ignore ip ranges in throttler
  • Add JS tests
  • FIX CSS

@LukasReschke as discussed.

@rullzer rullzer added the 2. developing Work in progress label Nov 11, 2016
@rullzer rullzer added this to the Nextcloud 11.0 milestone Nov 11, 2016
@mention-bot
Copy link

@rullzer, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @blizzz and @LukasReschke to be potential reviewers.

@LukasReschke
Copy link
Member

Let's add this to

server/.drone.yml

Lines 22 to 34 in 9fff4b0

app-check-code:
image: nextcloudci/php7.0:php7.0-6
commands:
- ./occ app:check-code admin_audit
- ./occ app:check-code comments
- ./occ app:check-code federation
- ./occ app:check-code sharebymail
- ./occ app:check-code systemtags
- ./occ app:check-code theming
- ./occ app:check-code workflowengine
when:
matrix:
TESTS: app-check-code
as well :)

@codecov-io
Copy link

codecov-io commented Nov 13, 2016

Codecov Report

Merging #2095 into master will increase coverage by 0.01%.
The diff coverage is 91.89%.

@@             Coverage Diff              @@
##             master    #2095      +/-   ##
============================================
+ Coverage     53.96%   53.98%   +0.01%     
- Complexity    21266    21279      +13     
============================================
  Files          1259     1259              
  Lines         74136    74173      +37     
============================================
+ Hits          40009    40041      +32     
- Misses        34127    34132       +5
Impacted Files Coverage Δ Complexity Δ
lib/private/Settings/Manager.php 44.78% <100%> (+0.34%) 44 <0> (ø) ⬇️
lib/private/Security/Bruteforce/Throttler.php 48.03% <91.66%> (+17.26%) 30 <12> (+13) ⬆️
apps/comments/lib/EventHandler.php 79.16% <0%> (-8.34%) 7% <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 d56f639...e5fb478. Read the comment docs.

@rullzer rullzer force-pushed the bruteforcesetttings branch from 37715af to 0385979 Compare November 14, 2016 13:16
@rullzer rullzer added 3. to review Waiting for reviews enhancement and removed 2. developing Work in progress labels Nov 14, 2016
@rullzer
Copy link
Member Author

rullzer commented Nov 14, 2016

@LukasReschke please have a look

@rullzer rullzer force-pushed the bruteforcesetttings branch from 0385979 to 56b33e1 Compare November 18, 2016 08:27
@rullzer
Copy link
Member Author

rullzer commented Nov 18, 2016

@MorrisJobke could you have a look at the crappy CSS? For some reason it wants text in order to show the trash icons

  1. Go to admin page
  2. Go to secrutiy tab
  3. Add a subnet thingy
  4. See bad icons
  5. fix it 😉

@MorrisJobke
Copy link
Member

I will have a look once I'm in the office 😉

@MorrisJobke MorrisJobke self-assigned this Nov 18, 2016
@MorrisJobke
Copy link
Member

@rullzer The security page is empty 😢

bildschirmfoto 2016-11-18 um 21 25 12

@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Nov 18, 2016
@rullzer
Copy link
Member Author

rullzer commented Nov 19, 2016

Enable the app

@MorrisJobke
Copy link
Member

Ah ... 🙈

@icewind1991
Copy link
Member

@rullzer any update here?

@rullzer rullzer force-pushed the bruteforcesetttings branch from 9e8cefb to 9aaf02c Compare January 10, 2017 14:56
@techc0de
Copy link

Hello all,

Any ETA on this?

@MorrisJobke MorrisJobke force-pushed the bruteforcesetttings branch from 9aaf02c to 73f455d Compare March 14, 2017 01:45
@MorrisJobke
Copy link
Member

I rebased this, fixed the conflicts, improved the settings layout and it now looks like this:

bildschirmfoto 2017-03-13 um 19 43 48

hovered:

bildschirmfoto 2017-03-13 um 19 43 55

@MorrisJobke MorrisJobke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 14, 2017
@MorrisJobke MorrisJobke removed their assignment Mar 14, 2017
@icewind1991
Copy link
Member

@MorrisJobke we can still force enable apps that are in a seperate repo by bundling them for a release if needed.

@rullzer
Copy link
Member Author

rullzer commented Mar 30, 2017

Let me rip this apart soonish.

@rullzer rullzer added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 30, 2017
rullzer and others added 7 commits April 2, 2017 21:08
This adds the bruteforce settings app that allows to configure (for now)
subnets that are to be ignored when doing brute force analysis. This can
for example be the LAN since we trust people from there.

* Add app
* Add php tests
* Add js tests

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
rullzer added a commit to nextcloud/bruteforcesettings that referenced this pull request Apr 2, 2017
From nextcloud/server#2095

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
https://github.com/nextcloud/bruteforcesettings

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer rullzer force-pushed the bruteforcesetttings branch from 73f455d to e09c386 Compare April 2, 2017 19:28
@rullzer
Copy link
Member Author

rullzer commented Apr 2, 2017

@rullzer rullzer force-pushed the bruteforcesetttings branch 2 times, most recently from 78e79f7 to e5fb478 Compare April 3, 2017 06:39
@rullzer rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 3, 2017
@rullzer
Copy link
Member Author

rullzer commented Apr 3, 2017

Ok lets get this in!

],
testFiles: [
'apps/bruteforcesettings/tests/js/IPWhitelistSpec.js'
]
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be better executed in the apps repo? Otherwise the PR there aren't checked with the tests itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it should.

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Beside the one comment this is fine 👍

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer rullzer force-pushed the bruteforcesetttings branch from e5fb478 to aee2d63 Compare April 4, 2017 08:29
@@ -2,6 +2,7 @@
"shippedApps": [
"activity",
"admin_audit",
"bruteforcesettings",
Copy link
Member

Choose a reason for hiding this comment

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

@rullzer what's that used for? I don't see an app bruteforcesettings

Copy link
Member Author

Choose a reason for hiding this comment

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

Well it is just the list of shipped apps. I'm fine with removing that as well... if we decide to just ship it from the appstore...

Copy link
Member

Choose a reason for hiding this comment

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

Oh, didn't see your comment #2095 (comment). Now it makes sense!

@LukasReschke LukasReschke dismissed ChristophWurst’s stale review April 4, 2017 09:43

Has been resolved.

@LukasReschke LukasReschke merged commit e0227cb into master Apr 4, 2017
@LukasReschke LukasReschke deleted the bruteforcesetttings branch April 4, 2017 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants