User ban

Reviews
Diff

- Download Diff

Information
Submitter:	Serban Andrei Stan
Repository:	infoarena
Branch:
Bugs:
Depends On:
Reviewers
Groups:	hackers
People:

Description

I've implemented most of the banip utility. At the request of some admins, I've posted my work, to ease the debugging while there are still few lines of code.

The banip utility works in the following way:

There are two types of ban: for username, and for userip. At user login, it verifies if the user is banned. If he is banned, then access to his account is not permitted (though access to the site is unrestrained).

Now for the ban functions: 
   - migrate-ban-ip was created in order to modify the db
   - I've modified login.php to acces the verify ban functions (placed in common/ban.php)
   - The ban functions verify if a user is banned, and if he is, he is not permitted login, otherwise if he was banned and the ban date expired, he is removed from the ban tables [DONE with keeping the user in the ban tables]
   - I've created a ban form in www/views/form_ban, very similar to the login form
   - Ban.php in /controllers retrieves the information from the form and accesses the functions in common/ban.php

What remains to be done:
    1)FIRST of all, permissions for the ban page [DONE]
    2)the possibility to ban a substring for an ip (i.e. 192.168.0.*, but 192.16*.0.1 does not work) [DONE]
    3)adding a button on each user's profile page, that redirects the current admin to the ban page, autocomplete-ing the username and IP form fields [DONE]
    4)the automatic logout of a banned user [DONE]
    5)on the ban page, for each userip the possibility to show all the accounts that utilised the current ip.
    6)an unban page (the functions that work with the database are already implemented in /common/ban.php) (eventualy the possibility to show all the elements in the ban tables).

How I have tested the changes so far:
    -I've tested the ban functions directly in a mysql console, adding users and deleting them to test the variables introduced
    -I didn't test the banip form so thoroughly, seems it works though

Testing Done

Screenshots

Issues

2
30
2
All Issues: 34

Description	From	Last Updated
in php NULL is null (lowercase)	Savin Tiberiu	Oct. 28, 2011, 7:24 p.m.
in the db schema you should have "AUTO INCREMENT" on the id field which should also be primary key. This ...	Savin Tiberiu	Oct. 28, 2011, 7:26 p.m.
"for a certain user" not "current user"	Savin Tiberiu	Oct. 28, 2011, 7:27 p.m.
same thing as above.	Savin Tiberiu	Oct. 28, 2011, 7:28 p.m.
same	Savin Tiberiu	Oct. 28, 2011, 7:28 p.m.
not "current user". In /common we put function that interact with the database, but do not interact in any way ...	Savin Tiberiu	Oct. 28, 2011, 7:29 p.m.
{ } and null. Also you can just use !$rows	Savin Tiberiu	Oct. 28, 2011, 7:29 p.m.
english	Savin Tiberiu	Oct. 28, 2011, 7:30 p.m.
You should build an array with all the ips you want to test and then do something like: SELECT * ...	Savin Tiberiu	Oct. 28, 2011, 7:56 p.m.
PRIMARY KEY, AUTO INCREMENT	Savin Tiberiu	Oct. 28, 2011, 7:34 p.m.
What is this for?	Savin Tiberiu	Oct. 28, 2011, 7:34 p.m.
Permissions?? This should be admin only	Savin Tiberiu	Nov. 10, 2011, 12:01 a.m.
I think we have special functions for getting things from _GET and from _POST.	Savin Tiberiu	Oct. 29, 2011, 2:47 a.m.
align	Savin Tiberiu	Oct. 29, 2011, 2:34 a.m.
I think the function is "request('username')" - not sure though, but I am pretty sure we have something like that.	Savin Tiberiu	Oct. 29, 2011, 2:48 a.m.
do the flash after you do the ban	Savin Tiberiu	Oct. 29, 2011, 2:49 a.m.
I think we have some function called remote_ip_info() which gives you info about the ip.	Savin Tiberiu	Oct. 29, 2011, 3:10 a.m.
I don't get it, Why is this necessary?	Savin Tiberiu	Nov. 1, 2011, 12:29 a.m.
??? Move this back here. It's easier to read, as all the configs should be included at the beginning.	Savin Tiberiu	Oct. 29, 2011, 2:55 a.m.
move this comment so that it makes sense	Savin Tiberiu	Oct. 29, 2011, 2:57 a.m.
REQUEST_URI not URL is ok	Savin Tiberiu	Oct. 29, 2011, 2:57 a.m.
This should be in the controller	Savin Tiberiu	Nov. 10, 2011, midnight
No db queries in views. This should go in /common, call the function from the controller and just pass the ...	Savin Tiberiu	Oct. 29, 2011, 3:08 a.m.
is this 80 chars??	Savin Tiberiu	Oct. 29, 2011, 4:37 a.m.
In other controllers this is: $view['user'] = request('user'); I know it's not much, but let's be consistent. This also enables ...	Valentin Stanciu	Nov. 9, 2011, 11:39 p.m.
Check indentation.	Valentin Stanciu	Nov. 9, 2011, 11:40 p.m.
You don't need the information from the db_query now. You can remove $query =	Adrian Budau	Dec. 4, 2011, 1:38 a.m.
If you want a single line add to the MySql query LIMIT 1, otherwise the query will go through all ...	Adrian Budau	Feb. 29, 2012, 3:19 a.m.
use SELECT COUNT FROM so it returns just the number. It's faster	Adrian Budau	Dec. 4, 2011, 4:44 p.m.
return true Don't do extra stuff unnecessary	Adrian Budau	Dec. 4, 2011, 1:42 a.m.
the same. And also for comparing things with * you should've used the LIKE keyword. I don't see it here	Adrian Budau	Dec. 4, 2011, 1:45 a.m.
add INDEX for username and expire so the query will go faster. After PRIMARY KEY (id) and before the closing ...	Adrian Budau	Dec. 4, 2011, 1:51 a.m.
same here just for username. I don't think you'll use a query SELECT * FROM ia_user_ip WHERE ip = something.	Adrian Budau	Dec. 4, 2011, 1:51 a.m.
If the user is good and he is banned $errors is true while $user is also true. Shouldn't this else ...	Adrian Budau	Nov. 10, 2011, 3:27 a.m.

Summary:

-	user ban
+	User ban

Diff:

Revision 2 (+403 -3)

Show changes

	trunk/apache.conf
	trunk/config.php
	trunk/common/ban.php
	trunk/common/lcs
	trunk/scripts/migrate-ban-ip.php
	trunk/www/index.php
	trunk/www/url.php
	trunk/www/utilities.php
	5 more

Diff:

Revision 3 (+403 -3)

Show changes

	trunk/apache.conf
	trunk/config.php
	trunk/common/ban.php
	trunk/common/lcs
	trunk/scripts/migrate-ban-ip.php
	trunk/www/index.php
	trunk/www/url.php
	trunk/www/utilities.php
	6 more

Diff:

Revision 4 (+416 -3)

Show changes

	trunk/apache.conf
	trunk/config.php
	trunk/common/ban.php
	trunk/common/lcs
	trunk/scripts/migrate-ban-ip.php
	trunk/www/index.php
	trunk/www/url.php
	trunk/www/utilities.php
	6 more

This looks pretty good for someone who never worked with php before. Good job. There a lot of comments, but this how we all start, trust me :).

Also I think you have quite a problems with your identation, add these lines to your .vimrc file:

Add this to you vimrc.

set expandtab
set tabstop=4
set shiftwidth=4
set smartindent

Also, add screenshot. Reviewboard supports adding screenshot to a review request, if you look carefully at this page, there is an "Add screenshot" button.

trunk/apache.conf (Diff revision 4)

Why are apache.conf and config.php in the here? These should not be versioned.
CC: Bogdan Tataroiu... any idea what this is?

Bogdan-Cristian Tătăroiu Sept. 21, 2011, 10:05 p.m.

Indeed, these two files should not be versioned, they contain code that is customized for your particular installation. Whenever you need to change anything in these two files, you should edit apache.conf.sample or config.php.sample and add those changes to the repository.

You should remove trunk/common/lcs from this diff as well. That's a compiled executable of trunk/common/lcs.cpp

trunk/common/ban.php (Diff revision 4)

Coding styles: 4-spaces tabs. Also tabs mustn't be the tab character, they must be 4 spaces.

trunk/common/ban.php (Diff revision 4)

No mysql_query. Use db_query instead.

trunk/common/ban.php (Diff revision 4)

No trailing spaces

trunk/common/ban.php (Diff revision 4)

You can do WHERE username = '$safe_user' AND expire > '$current_date' and you can get rid of the of the code that tests this. This will make your thing run a lot faster for users with several bans (we have some) as the amount of data sent from mysql to php is smaller and this is usually a huge bottleneck.

Also I don't think we should remove the ban records. This table should never reach an amount of records so big to be incredibly slow, and this way we can keep records of how many bans a user had over time.

trunk/common/ban.php (Diff revision 4)

Same comment.

trunk/scripts/migrate-ban-ip.php (Diff revision 4)

align this

trunk/www/controllers/ban.php (Diff revision 4)

No tabs needed here. Move everything one tab to the left.

trunk/www/controllers/ban.php (Diff revision 4)

No trailing spaces

trunk/www/controllers/ban.php (Diff revision 4)

wtf?? align your code properly.

trunk/www/controllers/ban.php (Diff revision 4)

*This

trunk/www/controllers/ban.php (Diff revision 4)

if else construction is like this:

if (condition) {
  ...
} else {
  ...
}

trunk/www/controllers/ban.php (Diff revision 4)

Trailing spaces.

trunk/www/controllers/ban.php (Diff revision 4)

trailing spaces

trunk/www/controllers/login.php (Diff revision 4)

Align your code properly.

trunk/www/controllers/login.php (Diff revision 4)

you need to { } even if there is only one instruction. Like this

if ($ban_user) {
  $errors = true;
}

I hope this what you meant with this code.

trunk/www/controllers/login.php (Diff revision 4)

Indent.

trunk/www/controllers/login.php (Diff revision 4)

Spaces, not tab charactes.

trunk/www/controllers/login.php (Diff revision 4)

"Incearcati"? Not very Romanian. Also what was the problem with "Incearca"?. I like "Incearca" more. You can use "Ip-ul tau a fost suspendata" for consistency.

trunk/www/index.php (Diff revision 4)

Try to put this somewhere near the cases for users or login shit. Not in the middle of the newsletter cases. 

We definetly a better system to dispatch. We should put it in our "To Refactor" excel, not to forget about it.

trunk/www/views/ban.php (Diff revision 4)

Spaces

Diff:

Revision 5 (+403 -3)

Show changes

	trunk/apache.conf
	trunk/config.php
	trunk/common/ban.php
	trunk/common/lcs
	trunk/scripts/migrate-ban-ip.php
	trunk/www/index.php
	trunk/www/url.php
	trunk/www/utilities.php
	6 more

Diff:

Revision 6 (+403 -3)

Show changes

	trunk/apache.conf
	trunk/config.php
	trunk/common/ban.php
	trunk/common/lcs
	trunk/scripts/migrate-ban-ip.php
	trunk/www/index.php
	trunk/www/url.php
	trunk/www/utilities.php
	6 more

Change Summary:

I've added permissions for the ban page. If you want me to publish changes more rarely please tell me.

Description:

		I've implemented most of the banip utility. At the request of some admins, I've posted my work, to ease the debugging while there are still few lines of code.

		The banip utility works in the following way:

		There are two types of ban: for username, and for userip. At user login, it verifies if the user is banned. If he is banned, then access to his account is not permitted (though access to the site is unrestrained).

		Now for the ban functions:
		- migrate-ban-ip was created in order to modify the db
		- I've modified login.php to acces the verify ban functions (placed in common/ban.php)
~		- The ban functions verify if a user is banned, and if he is, he is not permitted login, otherwise if he was banned and the ban date expired, he is removed from the ban tables
~		- I've created a ban form in www/views/form_ban, very similar to the login form (todo: upload a corresponding image)
	~	- The ban functions verify if a user is banned, and if he is, he is not permitted login, otherwise if he was banned and the ban date expired, he is removed from the ban tables [DONE with keeping the user in the ban tables]
	~	- I've created a ban form in www/views/form_ban, very similar to the login form
		- Ban.php in /controllers retrieves the information from the form and accesses the functions in common/ban.php

~		Basically, in this state, the code MUST NOT be commited, for several reasons, but mainly that the ban page has no permission restrictions.
~
	~	What remains to be done:
	~	-FIRST of all, permissions for the ban page [DONE]
-		Now, what remains to be done:
-		-FIRST of all, permissions for the ban page
		-adding a button on each user's profile page, that redirects the current admin to the ban page, autocomplete-ing the username and IP form fields
~		-on the ban page, for each userip the possibility to show all the user accounts that utilised the current ip.
	~	-the automatic logout of a banned user
	+	-on the ban page, for each userip the possibility to show all the accounts that utilised the current ip.
		-an unban page (the functions that work with the database are already implemented in /common/ban.php) (eventualy the possibility to show all the elements in the ban tables).

		How I have tested the changes so far:
		-I've tested the ban functions directly in a mysql console, adding users and deleting them to test the variables introduced
		-I didn't test the banip form so thoroughly, seems it works though

Diff:

Revision 7 (+398 -3)

Show changes

	trunk/apache.conf
	trunk/config.php
	trunk/common/ban.php
	trunk/common/lcs
	trunk/scripts/migrate-ban-ip.php
	trunk/www/index.php
	trunk/www/url.php
	trunk/www/utilities.php
	6 more

Screenshots:

Change Summary:

The ban interface is pretty usable at this point. Even though point 5 is pretty neat, it involves some tweaking with javascript, ajax, etc. I'm not so comfortable with those, and don't really think I'll be able to do anything consistent in a short period of time. Because this ticket has already been delayed for some time now, I think it would be a good idea to skip this point, at least at the moment. Same for the unban page. 

I left the unban page behind, because I believe that point 5 (a little modified) can be very similar to it.

Description:

		I've implemented most of the banip utility. At the request of some admins, I've posted my work, to ease the debugging while there are still few lines of code.

		The banip utility works in the following way:

~		There are two types of ban: for username, and for userip. At user login, it verifies if the user is banned. If he is banned, then access to his account is not permitted (though access to the site is unrestrained).
	~	There are two types of ban: for username, and for userip. At user login, it verifies if the user is banned. If he is banned, then access to his account is not permitted (though access to the site is unrestrained).

		Now for the ban functions:
		- migrate-ban-ip was created in order to modify the db
		- I've modified login.php to acces the verify ban functions (placed in common/ban.php)
		- The ban functions verify if a user is banned, and if he is, he is not permitted login, otherwise if he was banned and the ban date expired, he is removed from the ban tables [DONE with keeping the user in the ban tables]
		- I've created a ban form in www/views/form_ban, very similar to the login form
		- Ban.php in /controllers retrieves the information from the form and accesses the functions in common/ban.php

		What remains to be done:
~		-FIRST of all, permissions for the ban page [DONE]
~		-adding a button on each user's profile page, that redirects the current admin to the ban page, autocomplete-ing the username and IP form fields
~		-the automatic logout of a banned user
~		-on the ban page, for each userip the possibility to show all the accounts that utilised the current ip.
~		-an unban page (the functions that work with the database are already implemented in /common/ban.php) (eventualy the possibility to show all the elements in the ban tables).
	~	1)FIRST of all, permissions for the ban page [DONE]
	~	2)the possibility to ban a substring for an ip (i.e. 192.168.0., but 192.16.0.1 does not work) [DONE]
	~	3)adding a button on each user's profile page, that redirects the current admin to the ban page, autocomplete-ing the username and IP form fields [DONE]
	~	4)the automatic logout of a banned user [DONE]
	~	5)on the ban page, for each userip the possibility to show all the accounts that utilised the current ip.
	+	6)an unban page (the functions that work with the database are already implemented in /common/ban.php) (eventualy the possibility to show all the elements in the ban tables).

		How I have tested the changes so far:
		-I've tested the ban functions directly in a mysql console, adding users and deleting them to test the variables introduced
		-I didn't test the banip form so thoroughly, seems it works though

Diff:

Revision 8 (+313 -9)

Show changes

	trunk/common/ban.php
	trunk/common/lcs
	trunk/scripts/migrate-ban-ip.php
	trunk/www/index.php
	trunk/www/url.php
	trunk/www/utilities.php
	trunk/www/controllers/ban.php
	trunk/www/controllers/login.php
	6 more

Screenshots:

trunk/common/ban.php (Diff revision 8)

Show all issues

in php NULL is null (lowercase)

trunk/common/ban.php (Diff revision 8)

Show all issues

in the db schema you should have "AUTO INCREMENT" on the id field which should also be primary key. This way you can just do:

INSERT INTO ia_user_ip (username, userip) VALUES ($user, $ip)

trunk/common/ban.php (Diff revision 8)

Show all issues

"for a certain user" not "current user"

trunk/common/ban.php (Diff revision 8)

Show all issues

same thing as above.

trunk/common/ban.php (Diff revision 8)

Show all issues

same

trunk/common/ban.php (Diff revision 8)

Show all issues

not "current user". In /common we put function that interact with the database, but do not interact in any way with the web context.

trunk/common/ban.php (Diff revision 8)

Show all issues

{ } and null. Also you can just use !$rows

trunk/common/ban.php (Diff revision 8)

Show all issues

english

trunk/common/ban.php (Diff revision 8)

Show all issues

You should build an array with all the ips you want to test and then do something like:

SELECT *
FROM ia_ban_ip
WHERE (user_ip = $ip[1] OR user_ip = $ip[2] ..) AND expire > $current_date

It's not good to do so many mysql queryies because the network latency would be very high. Bassically for each query your program has to do the following things:
1. open db connection
2. send query
3. do query
4. send results
5. close connection

In your code you are doing all theese steps 16 times, while in my code each one is done only once, increasing a bit the time for steps 2 and 3, but it's not noticeable.

Also use SELECT COUNT(*) ...
This you count how many rows you have that pass the filters in the db and check if that value is 0 or not.

trunk/scripts/migrate-ban-ip.php (Diff revision 8)

Show all issues

PRIMARY KEY, AUTO INCREMENT

trunk/scripts/migrate-ban-ip.php (Diff revision 8)

Show all issues

What is this for?

Serban Andrei Stan Oct. 29, 2011, 2:19 a.m.

This table was build with the purpose of storing information for an unban page. (which wasn't yet implemented)

Another purpose is to redirect the admin, from a user's page, to the ban page, with the ip field completed.

trunk/www/controllers/ban.php (Diff revision 8)

Show all issues

Permissions?? This should be admin only

Serban Andrei Stan Oct. 29, 2011, 3:58 a.m.

It is. The ban options only appears to admins.

trunk/www/controllers/ban.php (Diff revision 8)

Show all issues

I think we have special functions for getting things from _GET and from _POST.

Serban Andrei Stan Oct. 29, 2011, 2:32 a.m.

Are you sure? I ran a grep search, and found the _GET and _POST functions in many other places, accompanied by "isset".

Serban Andrei Stan Oct. 29, 2011, 2:44 a.m.

Seems as "request" is working alternative.

trunk/www/controllers/ban.php (Diff revision 8)

Show all issues

align

trunk/www/controllers/ban.php (Diff revision 8)

Show all issues

I think the function is "request('username')" - not sure though, but I am pretty sure we have something like that.

trunk/www/controllers/ban.php (Diff revision 8)

Show all issues

do the flash after you do the ban

trunk/www/controllers/login.php (Diff revision 8)

Show all issues

I think we have some function called remote_ip_info() which gives you info about the ip.

trunk/www/controllers/login.php (Diff revision 8)

Show all issues

I don't get it, Why is this necessary?

Serban Andrei Stan Oct. 29, 2011, 3:58 a.m.

I need to retrieve the last ip for a certain user, in order to include it in the ban page.

trunk/www/index.php (Diff revision 8)

Show all issues

??? Move this back here. It's easier to read, as all the configs should be included at the beginning.

trunk/www/index.php (Diff revision 8)

Show all issues

move this comment so that it makes sense

trunk/www/utilities.php (Diff revision 8)

Show all issues

REQUEST_URI not URL is ok

trunk/www/views/ban.php (Diff revision 8)

Show all issues

This should be in the controller

Serban Andrei Stan Oct. 29, 2011, 3:58 a.m.

What exactly should be in the controller? The flash_error? The if statement?

Savin Tiberiu Oct. 29, 2011, 7:02 a.m.

The entire thing should go into the controller. The if, the flash_error, and the redirect. Once you enter the view it means that all permissions are done, and that page the user is going to get is the one built by that particular view. Permission checks, session vars (flash_error just sets a session variable with the text you pass to it) , redirects must be done in the controller.

trunk/www/views/user.php (Diff revision 8)

Show all issues

No db queries in views. This should go in /common, call the function from the controller and just pass the data to the controller.

trunk/www/views/user.php (Diff revision 8)

Show all issues

is this 80 chars??

Serban Andrei Stan Oct. 29, 2011, 3:44 a.m.

Exactly 2^7. Should there be a limit of 80 chars to a line?

Adrian Budau Oct. 29, 2011, 3:56 a.m.

It's one of the indenting rules on IA developing. Also found in the google coding style

Serban Andrei Stan Oct. 29, 2011, 4:40 a.m.
```
I see. Still, it's a pretty old thing.
```

Adrian Budau Oct. 29, 2011, 4:43 a.m.

You'll see it's usefulness when using vim and vsp command(it splits the screen in 2 by a vertical line). It's easier to make modifications and watch a file at the same time with this command.

Savin Tiberiu Oct. 29, 2011, 7:02 a.m.

Even though those rules were made a long time ago, they still hold, all of them. The 80 chars rule is actually used very wide in all companies (google, facebook, everywhere). It useful when you are using vsplit, although at first it was introduced because if you are using linux in tty mode you can only see 80 chars at most. To sum up, this is a very important rule and there are no exceptions to it, and even if there were exceptions to the rule, your case would not qualify. This line is actually is pretty easy to break.

trunk/www/controllers/ban.php (Diff revision 8)

Never do security in the viewer!
The fact that ban options only appear to admins is not secure. Maybe some other developer decides to implement another viewer that uses this same controller. And maybe he doesn't check admin right. Or what if a user forges a POST to a page that calls this controller? :)
Well, you get the idea, always (re)check security in the controllers.

trunk/www/controllers/ban.php (Diff revision 8)

Show all issues

In other controllers this is:
$view['user'] = request('user');

I know it's not much, but let's be consistent. This also enables the controller to be more flexible and callable by POST as well as GET.
You could change the other gettattr($_GET, ...) with request(...). You can leave the $_POST the same if it was for security reasons.

trunk/www/url.php (Diff revision 8)

Show all issues

Check indentation.

Diff:

Revision 9 (+310 -8)

Show changes

	trunk/common/ban.php
	trunk/common/lcs
	trunk/scripts/migrate-ban-ip.php
	trunk/www/index.php
	trunk/www/url.php
	trunk/www/utilities.php
	trunk/www/controllers/ban.php
	trunk/www/controllers/login.php
	6 more

Fix it!

trunk/common/ban.php (Diff revision 9)

Show all issues

You don't need the information from the db_query now. You can remove $query =

trunk/common/ban.php (Diff revision 9)

Show all issues

If you want a single line add to the MySql query LIMIT 1, otherwise the query will go through all the records.

Adrian Budau Nov. 10, 2011, 1:06 a.m.

You can also get only the third column. Don't just SELECT everything when you dont need. This goes for everything.

trunk/common/ban.php (Diff revision 9)

Show all issues

use SELECT COUNT FROM so it returns just the number. It's faster

trunk/common/ban.php (Diff revision 9)

Show all issues

return true
Don't do extra stuff unnecessary

trunk/common/ban.php (Diff revision 9)

Show all issues

the same.
And also for comparing things with * you should've used the LIKE keyword. I don't see it here

Adrian Budau Nov. 10, 2011, 2:26 a.m.

Nevermind the thing with the LIKE keyword

trunk/scripts/migrate-ban-ip.php (Diff revision 9)

Show all issues

add INDEX for username and expire so the query will go faster. After PRIMARY KEY (id) and before the closing bracket add this 
, INDEX username(`username`)
, INDEX expire(`expire`)

trunk/scripts/migrate-ban-ip.php (Diff revision 9)

Show all issues

same here just for username. I don't think you'll use a query SELECT * FROM ia_user_ip WHERE ip = something.

trunk/www/controllers/login.php (Diff revision 9)

Show all issues

If the user is good and he is banned $errors is true while $user is also true. Shouldn't this else go elsewhere?

Is this the last version of the diff? I see a review from Adrian with issues marked resolved and dropped but no update.

You have a pending review.

Review Board 2.5.13.1

Screenshots

Files

Issues

Summary:

Diff:

Diff:

Diff:

Diff:

Diff:

Change Summary:

Description:

Diff:

Screenshots:

Change Summary:

Description:

Diff:

Screenshots:

Diff: