Add enrolment via a secret key #90
Conversation
There was a problem hiding this comment.
Thanks @alasdairwilson. I really like the idea of the enrollment key, this will work really well :) I've requested a few changes below, I think the enrolment dialog would be better written as a (very simple) form. Also could you add a component test for EnrolDialog as well?
components/EnrolDialog.tsx
Outdated
| @@ -45,6 +59,39 @@ const EnrolDialog: React.FC<Props> = ({ event, show, onEnrol}) => { | |||
| }) | |||
| }; | |||
|
|
|||
| const enrolWithKey = () => { | |||
| const enrolKey = (document.getElementById('enrol-key') as HTMLInputElement).value; | |||
There was a problem hiding this comment.
I didn't think you could use getElementByID in React because it manages its own virtual DOM?
This is basically a form, so I would use the https://react-hook-form.com/ library, which we use in other places in the codebase: eg. https://github.com/OxfordRSE/gutenberg/blob/main/components/content/Challenge.tsx
There was a problem hiding this comment.
components/EnrolDialog.tsx
Outdated
| @@ -57,8 +104,32 @@ const EnrolDialog: React.FC<Props> = ({ event, show, onEnrol}) => { | |||
| </Modal.Header> | |||
| <Modal.Body> | |||
| <Content markdown={event.enrol} /> | |||
| You should have received an enrolment key from the course organiser: | |||
| <div className="flex"> | |||
There was a problem hiding this comment.
Use our Stack component here: https://github.com/OxfordRSE/gutenberg/blob/main/components/ui/Stack.tsx
This is used to render a vertical or horizontal stack of components with spacing between them, e.g.
gutenberg/components/content/Thread.tsx
Line 200 in 95beaec
prisma/schema.prisma
Outdated
| @@ -100,6 +100,8 @@ model Event { | |||
| summary String @default("") | |||
| enrol String @default("") | |||
| content String @default("") | |||
| enrolKey String @default("") | |||
There was a problem hiding this comment.
| enrolKey String @default("") | |
| enrolKey String @default(uuid()) |
There was a problem hiding this comment.
perhaps set this by default to a random string / uuid?, and the instructor key as well
There was a problem hiding this comment.
I guess this would affect the migration as well
There was a problem hiding this comment.
Ironically, by changing the enroldialog to use a form, it means you can't submit a blank string anyway (if the enrolkey is blank it wont match the undefined form submission for submitting a blank string) but I will make this change.
I had initially thought about hashing the key but this makes the edit event page significantly more complex as I would need a separate method of submitting the keys as you cant have them populated by default and therefore submitting the changes without setting a key would change the key.
I figured since that actually without the hashing there is no real layer of security; the keys are exposed in the browser memory because the Event object is there so I just left it, but yeah this change is fine.
There was a problem hiding this comment.
ahhh yes, probably with the form this doesn't really add much :) I agree with your thoughts re hashing, probably not worth implementing this as we don't need high security (at least I don't think we do.....)
prisma/schema.prisma
Outdated
| @@ -100,6 +100,8 @@ model Event { | |||
| summary String @default("") | |||
| enrol String @default("") | |||
| content String @default("") | |||
| enrolKey String @default("") | |||
| instructorKey String @default("@mHJdxza218sxzd014s8ASDsa0axv0sdfmBJNLM=3asfdnmYHL1234adczcz") // just in case this is unset then we don't want to let blank keys let users in | |||
There was a problem hiding this comment.
| instructorKey String @default("@mHJdxza218sxzd014s8ASDsa0axv0sdfmBJNLM=3asfdnmYHL1234adczcz") // just in case this is unset then we don't want to let blank keys let users in | |
| instructorKey String @default(uuid()) |
This adds a secret enrolment key and instructor key to events, if the student provides these when selecting enrol then it automatically sets their status to STUDENT (or INSTRUCTOR). The old button for request enrollment is still there if they have not received a key.
I considered hashing these keys, because they are available in the client? I really do not see it being an issue though, happy to be convinced otherwise.
closes #88