Better task security
Review Request #203 - Created March 13, 2012 and submitted
Information | |
---|---|
Adrian Budau | |
infoarena | |
Reviewers | |
hackers | |
Modified the security for tasks. Before it was just a property(hidden) which was true or false. This led to hard-codings and surprises(users could add contest problems in user-round). Now security works as for textblocks. 1) Private tasks, tasks which can only be seen by their owner or an admin/intern 2) Protected tasks, task which can be seen but can not be used by normal-users in user-rounds. These tasks can have tags which will not be seen by normal users. (Thus allowing us to set tags for a problem before the contest without trouble). 3) Public tasks which are basically problems which can be found in archives. Tasks are made private when a round is waited, protected when the round starts and public when they are added to archives. They also become protected when removed from all archive rounds. Also made some lint fixes(lines over 80 chars, removing ?> etc) Also fixed a cache crash regarding the rating property of a task not being inserted into the task array on creation.
Made a contest. Private tasks became protected. Stopped it. Protected tasks became private. Tried adding a protected task(by force using the chrome browser "inspect element") which ended in an error. Also tried searching by tags -> protected and private tasks do not appear.
Change Summary:
Fixed some problems with the fact that a user couldn't directly update the security of a task and the "(" which broke the task filtering by tags.
Diff: |
Revision 3 (+98 -103) |
---|
Hey, didn't look through it in detail yet, but it does look good. It seems elegant enough :) Can you automatically make tasks public whenever they are added to a archive round? Make them protected if they are removed from ALL archive rounds, as well.
Change Summary:
Fixed some serrious issues with the cache. Due to the way tasks we're made visible(without updating the cache) we had problems during contests. Updated description
Description: |
|
|||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+171 -130) |
Change Summary:
Also repaired the task creation process. The rating was not set to anything when it was added to the cache and this led to errors with the *stars* parameter.
Diff: |
Revision 5 (+172 -130) |
---|
The code looks good, however I don't agree with the decision to not re-protect tasks once they have been removed from archive rounds. This comes down to the principle of least surprise. If I'm an admin that doesn't know implementation details and I know that in order for people to fully see a problem I can add it to an archive, I *will* expect it to not be fully visible when I remove it. If in one case you do something automatically, but in the other you do not, that's inconsistent behavior. So either do them both, or not do either one.
-
trunk/common/db/round.php (Diff revision 5) -
This parameter doesn't really make a lot of sense. You are making an assumption that it will only be true for archive rounds. If the round was not an archive round and the argument was true, then it would not make sense to update task securities to 'public'. Either just do if ($type == 'archive') and remove the parameter, or add more complex logic than this.
-
trunk/common/db/task.php (Diff revision 5) -
I don't understand what purpose 'check' serves?
-
trunk/common/db/task.php (Diff revision 5) -
parent_rounds
Change Summary:
Tasks now become protected when removed from archive rounds.
Description: |
|
|||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+183 -132) |
Change Summary:
Fixed some bugs with the last patch. Was checking for round different than user-defined instead of archive rounds. This led to protected tasks when changing from penalty-round to classic and viceversa.
Diff: |
Revision 7 (+183 -132) |
---|
Excellent.
-
trunk/common/db/task.php (Diff revision 7) -
Nope, its shows possession, it's means it is.
-
trunk/common/security.php (Diff revision 7) -
Uhm, looking at the code that was previously here... It seems like we were already doing proper checks for 'classic' contests. You shouldn't have been able to add tasks from Algoritmiada into virtual contests like was claimed. Was this code malfunctioning?
-
trunk/common/security.php (Diff revision 7) -
Add a || $is_intern as well since you have it in can_view_source?
-
trunk/common/security.php (Diff revision 7) -
I think it would make more sense to have $job['task_security'] != 'private' && $job['task_open_source']
-
trunk/common/task.php (Diff revision 7) -
Mention in the commit log that you fixed the new task crash bug.
-
trunk/www/controllers/round.php (Diff revision 7) -
what do you mean?
Forgot to ship it.
Description: |
|
|||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 8 (+185 -133) |