gikeymarcia / Collector

A program for running psychology experiments on the web
32 stars 15 forks source link

check.php has some flaws #82

Open gikeymarcia opened 9 years ago

gikeymarcia commented 9 years ago

At the request of Paulo Carvalho I've been poking around in check.php and I noticed a few things wrong with it.

gikeymarcia commented 9 years ago

For a hotfix I am going to

gikeymarcia commented 9 years ago

Another mistake, fileLocations.php has the wrong folder name for /eligibility/. It is using the uppercase version /Eligibility/ which is fine and runs on windows because it is case insensitive but it breaks when we try and run it on the server. Rolling up this change into the hotfix.

gikeymarcia commented 9 years ago

$blacklist needs to work differently.

Right now if $blacklist == TRUE all users who login have their IP added to the blacklist and that will block them from logging in again. The problem is that $blacklist only controls whether IPs are added to the list, not whether the list is used. If someone tries to run in the lab and $blacklist is accidentally set to TRUE then it will not allow anyone else to login. Changing to $blacklist == FALSE will not fix the problem. The lab IP is already on the blacklist and the way to stop the blocking would be to go into /Experiment/eligibility/ and delete rejected-IPs.csv. The other option would be to figure out your lab's IP then add to the $whitelist but I doubt most people know how to do this or even that this is the solution.

This is very confusing functionality and asking for trouble. Not yet sure what the solution is but I am going to turn off IP logging/blocking. For now check.php will only do what it was originally intended to do. If a user logs in with a username contained in one of the files in /eligibility it will tell them they are not allowed and quit. By turning off IP logging they will be able to go back to the main screen and enter a different username and get into the experiment. This is temporary and not ideal but right now I think the IP blacklisting functionality is too opaque and likely to cause more harm than help.

gikeymarcia commented 9 years ago

7e866a8b1b9eb4a71a1ad76194f38698e02cdfde is the hotfix. Merging into dev

gikeymarcia commented 9 years ago

I forgot to add the IPv4 localhost to the whitelist in the hotfix. This'll be added to the dev channel when I make the new version of this file.

TysonKerr commented 9 years ago

Possible changes to check.php:

Everyone who gets to index.php immediately gets their IP checked. If its on the list of bad IPs, they are immediately stopped, before having a chance to type in a username. If they are an experimenter, they can go to Tools/ and somehow unban themselves.

Once they enter a username, that name is checked against the list of ineligible names. If there is a match, they are blocked. Then, their IP is checked. If it isn't whitelisted, their IP is added to the list of bad IPs, so that future visits are immediately stopped. But new names aren't added to the list just for logging in or participating, so that people can return for multisessions. And IPs aren't blocked unless they match a blocked name.

A lot of check.php can be moved to a different file in Tools/ , rather than having check.php constantly checking if we are coming from login.php or not. And at some point, it would be cool to also add the IPs of people filtered out by demographics to the badIP list, so that people dont try to figure out the eligibility check.

Also, when checking the files for a list of names, it should check for a "Username" column as well as a "WorkerID" column, so you can also drag and drop a session_finish.csv file.

gikeymarcia commented 9 years ago

make rejected-ip list and a completed-ip list

completed has code from done.php

check IP lists at index.php and redirect as needed

check username at login