Non-core contributed modules

Mike - Similar to your other block to create parents. I think you should also set parent to be the default in this one too. I'm going to go ahead and set things up in CVS now. Peace - Anthony

p.s. - Where are my manners? It's late and I'm tired but that is no excuse for not saying thank you for sharing this code with the community.

Also, could you post the links to the M&P entry and docs page here. You may also want to create a README.txt file.

code added to cvs, component created, cvs access granted - resolving as fixed. Thanks again Mike and do not hesitate to let me know if you need anything else. You are becoming a block expert! Peace - Anthony

I didn't really intend this to link parents, I really intended it for personal tutors.

At my college each student is taught several subjects by subject teachers plus attends tutorial time once a week which allows the personal tutor to track general progress and other non-subject specific things (university applications etc). This block does this well (rather than the createparent block) because the tutors already have accounts (they are also teachers).

I'm also hesitant to start 'advertising' this in M&P etc with the input as it is- uncleaned. By default only admins can upload a file but anyone who can upload a file could cause any sort of damage, as well as accidentally having things like apostrophies could cause all sorts of problems (I've no looked that closely but maybe an accidental sql injection whereby the where clause gets missed off leading to a value being changed for a whole table or something?). Any ideas on how to sort this?

Thanks,
Mike

Mike - I remember you had mentioned some possible security concerns. I think as long as you mention them in a README.txt file or in the description of the M&P that it would be fine; however, there is certainly no obligation to create an M&P module if you would rather not. I trust your judgment. In terms of parent accounts, I envision someone uploading a list of users for the parent accounts and then using a separate csv file creating the role assignments with your block. So tutors or parents are very similar and I've been emailing one person who wants to create both and I think your block here would be a great time saver for him. Keep up the great work, and if you want create some issues in the tracker for yourself to address the issues of cleaning the data and verifying it. I think if you use the import user code as a model you should be in good shape. If there is a particular area of code that you want me to take a look at, just give me the line numbers and I will. Peace - Anthony

Oops, clicked close on the wrong issue- I'm still working on the security of this

Mike - Per the discussion at http://moodle.org/mod/forum/discuss.php?d=115943#p521232, I changed the link to a relative rather than incorrect absolute link and committed. Peace - Anthony

I've just been having a look at the security of this, it seems that mysql is not vulnerable to the very worrying type of attack:

You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ';drop table mdl_user;select 'a' LIMIT 100' at line 1

SELECT * FROM mdl_user WHERE idnumber = '35000198546722';drop table mdl_user;select 'a'

• line 686 of lib/dmllib.php: call to debugging()
• line 481 of lib/dmllib.php: call to get_recordset_sql()
• line 421 of lib/dmllib.php: call to get_record_sql()
• line 37 of blocks/tutorlink/process.php: call to get_record()

However I'm not sure that all databases are.

As far as the 'mess up the where clause' type of attack, it seems that it would have to be an intentionally crafted attack (I don't think we have any parents like this: http://xkcd.com/327/). Would just stripping all single quotes protect against this, or would there be other possible ways in?

Thanks,
Mike

Mike,

Generally speaking, all input data should be validated. The optional_param and required_param calls help a great deal with this. So in the example you give if I have idnumber coming in Moodle should verify that it is an int before passing on to the query. It is worrisome that this is not happening in the tutor relationships block.

Peace - Anthony

Mike - Taking a quick look, since you are processing the data from a file you will need to ensure that the data in the file is of the type expected before proceeding. Otherwise, as you noted you open yourself up for sql injection possibilities. Now since the block itself should be limited to those with administrative access we might hope that they might not try to sabotage their own site but it is something good to be aware of. It is a good rule to never trust the user to put in what you want them to put in. Peace - Anthony

Is it correct to enforce idnumbers being integers? It is stored in the user table as a varchar and I would guess is used in certain places with various letters etc in the unique reference. I think I'm going to have to treat it as a string, even if that string happens to be a number.

Thanks
Mike

Mike - You are right, I was confusing idnumber with the user's id (i.e. the id field in users table). You would want to make sure that it is ALPHA_TEXT (or something like that off the top of my head. In other words, make sure it is escaped to remove any non alphanumeric characters like the semicolor, single quotes, etc. that make sql injection possible. Peace - Anthony

I've just commited an update that uses clean_param with PARAM_ALPHANUM; I think this will fix the security issue. I've not missed anything have I?

Thanks,
Mike

I've just commited yet another update, now it can work in the background on cron (checking a configurable location for the file, like flatfile enrolment).

I think this is now ready to go in M&P

http://moodle.org/mod/data/view.php?d=13&rid=2395

Mike - I think the latest commit looked good for checking the param type. I've approved the M&P entry. Thanks for sharing your work with the community. Peace - Anthony

Note to readers: The tutor block is now called 'CSV user relationships block' but once installed is called 'Upload Tutor Relationships'. It is located at Modules and plugins page: http://moodle.org/mod/data/view.php?d=13&rid=2395
The file attachment here at contrib-1025 is an OLDER version. Put the whole folder inside the blocks folder. AT your website Add the block which is called 'Upload Tutor Relationships' The csv text file does NOT need a heading, just Tutor Id#,Tutee ID#, add or
TutorID#, tuteeID#,del

Jamie - Have you added these comments to the Moodle Docs for this block? It might also be helpful to create a CHANGELOG.txt file for the block so that folks have a little history of the block's development. Thanks for the update and your continued work on this useful block. Peace - Anthony

Hi Mike and Anthony. sorry, but a major issue I uncovered today. Most of the data I'm sent works perfectly but a few cells had extra paragraph marks. My vba program outputs each cell to a text file. The paragraph marks cause it to interpret all blanks as an ADMIN account, me, and pairs it as a mentee or mentor when it encounters each space. See below for an example:
What would be nice is: blanks are Never admins AND if the row does not have all 3 items, then it shows as an error.
I edited out the teacher and student names but look at admin! That's me!

output file shows:
1110921,36279,del
1110921
,36279,add
1111206,35890,del

executed I get: I almost let it go because it seemed to work below, but look closely at admin . . .
Relationship sucessfully deleted("teacher name edited"->"student name edited")
Relationship sucessfully created("teacher name edited"->Admin User)
Relationship sucessfully created(Admin User->"student name edited")
Relationship sucessfully deleted("teacher name edited"->"student name edited")

I fixed it with this file
1110921,36279,del
1110921, ,del
,36279,del
1111206,35890,del

and got these results
Relationship sucessfully deleted("teacher name edited"->"student name edited")
Relationship sucessfully deleted("teacher name edited"->Admin User)
Relationship sucessfully deleted(Admin User->"student name edited")
Relationship sucessfully deleted("teacher name edited"->"student name edited")

Jamie - Thanks for reporting this. Sounds like a "garbage in, garbage out" problem but perhaps Mike can do something to tighten up some of the checks. It may be best to create a new issue for that. Let me know if I can be of help. Peace - Anthony

Closing all of my resolved issues. Peace - Anthony