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

List and Search Blacklist IP Addresses #18

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

basebandit
Copy link
Contributor

This PR adds three features to the blacklist command. One can now:

  • add one or more ips to the blacklist
  • search for an existing ip address in the blacklist
  • list all blacklisted ip addresses

the return statement in the if clause already guards against executing the code outside the clause
The upstream test file has changed
this will avoid redundancy in code that need to redeclare the same variables
this allows the caller to add one or more ips to the blacklist
exist subcommand checks if the given ip exists in the blacklist returns true if it exists otherwise false
checks whether the cli params passed in are also equal to 2 to cater for the blacklist 'exists' subcommand whose cli params length is exactly 2
initially passed in the subcommand value instead of the ip value whose index is 1
@JamesCullum
Copy link
Collaborator

JamesCullum commented Jan 17, 2021

Is it finished and tested?
Can you add tests for all new commands?

Please also make sure that all checks pass.

removed unused column slice
@basebandit
Copy link
Contributor Author

Is it finished and tested?
Can you add tests for all new commands?

Please also make sure that all checks pass.

Sure will do.

@basebandit
Copy link
Contributor Author

basebandit commented Jan 17, 2021

Let me add tests for the new blacklist features.Had tested them manually. oops!

@JamesCullum
Copy link
Collaborator

This condition is a bit useless: test.assertLogContains("", "Can list blacklisted ips") //we expect none at first since blacklist mode is off

Also keep in mind that you need to clear the log for assertions, if they could potentially be contained in previous responses.

As this test does not require the login flow, can you put it in a different test case, so that it can potentially be run in parallel? Would be great if you could as well add a test case that checks if the blacklist actually blocks a user (eg blocking localhost, surfing and verifying that the connection fails, then unblocking and trying again).

@basebandit
Copy link
Contributor Author

This condition is a bit useless: test.assertLogContains("", "Can list blacklisted ips") //we expect none at first since blacklist mode is off

Also keep in mind that you need to clear the log for assertions, if they could potentially be contained in previous responses.

As this test does not require the login flow, can you put it in a different test case, so that it can potentially be run in parallel? Would be great if you could as well add a test case that checks if the blacklist actually blocks a user (eg blocking localhost, surfing and verifying that the connection fails, then unblocking and trying again).

Found this error today when running the tests. I am not sure about the username, earlier the tests were passing but after testing for the third time in a row, this came up.
invalid_login
I have logged the contents of the response body

@JamesCullum
Copy link
Collaborator

Looks like it should pass - maybe thats due to some local constraints?

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

Successfully merging this pull request may close these issues.

2 participants