nus-oss-test / testrepo4

TEAMMATES system is online at
http://teammatesv4.appspot.com
0 stars 0 forks source link

Introduce joining for instructors: send invitation email instead of requiring google ID as an input #1754

Closed damithc closed 10 years ago

damithc commented 10 years ago

From OTS.Nexus on March 12, 2014 22:23:53

To ease the process of adding new instructors. Not required to know the google ID before hand.

Original issue: http://code.google.com/p/teammatespes/issues/detail?id=1711

damithc commented 10 years ago

From dam...@gmail.com on March 15, 2014 00:12:23

Blocking: teammatespes:1576

damithc commented 10 years ago

From OTS.Nexus on March 24, 2014 05:39:56

Hi, Shawn. I just saw that you made some changes to the Emails class. So should I use the new addEmailToTaskQueue() method instead of the old sendEmail() method to send out invitation email to instructor? Or there is different usage for both of them?

Cc: leeshaw...@gmail.com

damithc commented 10 years ago

From leeshaw...@gmail.com on March 24, 2014 06:20:18

Hi Theong Siang, it depends on whether you need the emails to be sent using task queues (for example if there are many emails that need to be sent which might risk a timeout). If there are very few emails that needs to be sent you can just use the sendEmail() method. Otherwise, you can use addEmailToTaskQueue() or the sendEmails() method (with an 's'), which will automatically queue all the emails to a task queue =)

damithc commented 10 years ago

From OTS.Nexus on March 24, 2014 06:42:03

I see. So if just sending a single email then sendEmail() method should be fine i guess. Thanks a lot =)

damithc commented 10 years ago

From OTS.Nexus on March 25, 2014 00:16:13

ReadyForReview: https://codereview.appspot.com/79710043/ Staging server with the deployed updates: https://teammates-ots.appspot.com/ Details & changes: -Registration key for instructor is generated using the email and courseID, e.g. "user@email.com%CS2103", as to follow the primary key format (googleID@courseID) -The generation of registration key is done implicitly in the Instructor entity layer during creation of new instructor. This is make it as similar to Student registration key. So that it is possible to change the format in future without affecting other parts at all. -The primary key for Instructor entity cannot be changed. Hence there is no way for generating random registration key as for the case of Student entity. -Legacy instructor data will have null value for registration key, but this doesn't matter as we only need the registration key for joining feature

-A join invitation email will be sent to instructor immediately after he/she has been added as an instructor in a course. -Editing feature is currently disable for new instructors who hasn't joined the course (we might want to consider enable it in future) -"Send Invite" link replaces the "Edit" link in the instructor table in CourseEditPage. It allows the invitation email to be sent again to the instructor.

Status: ReadyForReview
Cc: -leeshaw...@gmail.com
Labels: -Difficulty-High Difficulty-VeryHigh

damithc commented 10 years ago

From dam...@gmail.com on March 25, 2014 00:27:52

Staging server supposed to send emails right? I didn't get any. It fails if I use leading/trailing spaces in the name/email (not sure which one is causing the error) The key generation is not secure. There is a window during which any person can hijack the instructor privileges by guessing the key.

Status: ChangesRequested

damithc commented 10 years ago

From dam...@gmail.com on March 25, 2014 00:43:39

I received the emails. It took a while. :-)

damithc commented 10 years ago

From dam...@gmail.com on March 25, 2014 00:45:36

Also consider the case where a person uses the same Google account to join as multiple instructors to the same course.

damithc commented 10 years ago

From dam...@gmail.com on March 25, 2014 00:48:53

Need to rephrase this in the email "Please do not share it with your classmates"

This too: "The instructor has been added successfully." --> "The instructor {name here} has been added successfully. An email containing how to 'join' this course will be sent to {email here} in a few minutes".

damithc commented 10 years ago

From OTS.Nexus on March 25, 2014 00:58:50

Fixing the mentioned issues now.

Regarding the security issue, isn't that the users are not able to see the key due to the encryption? It is only decrypted in the database layer before updates of instructor.

Regarding the case of sharing single GoogleId, shall we enforce that each instructor in a course must have different GoogleId? I think the joining for students might also share the same problem, as it doesn't check whether the Google Id has been used for other students in the same course.

damithc commented 10 years ago

From dam...@gmail.com on March 25, 2014 01:11:30

Security issue: You may have to add a random piece of string before encrypting. Otherwise, anyone with the right knowhow can generate the key.

Multiple joins: Yes, create a separate issue for it.

damithc commented 10 years ago

From OTS.Nexus on March 25, 2014 16:44:59

Fixed the security issues and other issues.

Registration key is now generated by the combining the primary key (email+courseID) with a cryptographically strong random number. https://codereview.appspot.com/79710043/#ps60001

Status: ReadyForReview

damithc commented 10 years ago

From dam...@gmail.com on March 26, 2014 02:23:34

Status: ReadyToMerge

damithc commented 10 years ago

From OTS.Nexus on March 26, 2014 07:25:25

This issue was updated by revision 613308660d52 .

damithc commented 10 years ago

From OTS.Nexus on March 26, 2014 10:23:09

Status: Delivered

damithc commented 10 years ago

From dam...@gmail.com on March 29, 2014 03:03:27

Status: Deployed
Labels: Milestone-V4.94