0

Should checks on reads / writes to records in a database by users be performed before reading / writing the actual data? Or could one just apply it all in the same query?

ie. 

assertUserCanViewClassRoom($classroomId, $userId, $userRole);
assertUserCanRemoveStudent($classroomId, $userId, $userRole);

Or Should the check be completed in the sql of the action?

viewClassRoom($classroomId, $userId, $userRole);
removeStudent($classroomId, $userId, $userRole);

Me Being Way too Verbose (aka Wordy)

Now I was going to modify all the queries, just to ensure that users doing the actions are permitted to do so, where users have access to Read / Update records in Table B that they might of created or their roles allow them to overlook records belonging to lesser users (aka administrators, teachers, students)

Currently the process allows any user to modify the records (horrible practice). Teacher A can view Teacher B info, and modify it by adding / removing students from the classroom. A simple check of teacher's id would ideally prevent this action from even occurring.

Should I place a check prior to accessing that record to verify does Teacher X does have permission to record id=Y in Table B. This sounds great, but is it really? Am I doing double the work to check first than read / write the data?

I thought it would be a simple query statement change which joins the tables based upon the user id or user role permissions to the record that is being modified.

I'm being advised to do a pre-check which sounds great, but now I am making potentially extra traffic to the database to complete what I would consider to be a simple transaction of read or write. I sort of look at this like an assert or check via sql to verify that user can perform the task than perform it.

Is this standard practice for assertions / permission checks? Check first than have a simple function for Update, Insert or Delete? (mind you Delete is never really used) If this is not the case what would be a better suggestion?

Additional Info System is set up so user's currently can read thier own data, but with some simple modifications users can actually read anyone's data.

Project is PHP with MySQL backend all Object Oriented

mcv
  • 1,380
  • 3
  • 16
  • 41

1 Answers1

2

Always check first if you're wary, especially if you're not very confident in your sanitization checks.

So, you'd do a simple

SELECT is_admin FROM users WHERE user = '{$userid}';

etc. If is_admin is true, then carry on with your select/update/insert script. If not, deny access.

Eoghan
  • 1,720
  • 2
  • 17
  • 35
  • Should these checks be the standard practice for accessing all data? Read / Writes? – mcv Jan 24 '17 at 01:10
  • 2
    It depends on what level of security your reads and writes need. If you're writing some generic static backend output to the db, then you don't need to. For reading, it fully depends on if it's sensitive material. If they should be an admin user or have special privileges to select from those rows, then yes, check beforehand. In the case of unsanitized data though, checking beforehand won't help - injection can happen either way. – Eoghan Jan 24 '17 at 01:12
  • Currently from my post above there are zero levels of security. I already easily hacked the site without much effort. I just think if User A can see User B data than manipulate it there is something fundamentally wrong and why was the user's id not used in the baseline query using a left join statement. – mcv Jan 24 '17 at 01:17
  • The same applies for reading, why not check if user A can read this data with the user id in the select statement. Why is it that check is performed and if the user is allowed, than go read it? Feels like twice the work? I get that roles can complex the queries. – mcv Jan 24 '17 at 01:19
  • 1
    If reading applies to sensitive data, then yes, you should always check for permissions. There's rarely any reason to run an expensive merge query to get all the data back in one view - just check for permissions on the backend, then run the actual query afterwards. – Eoghan Jan 24 '17 at 03:28