Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include random str in responses when listing submissions #10

Closed
aalmanza1998 opened this issue Feb 23, 2020 · 11 comments
Closed

Include random str in responses when listing submissions #10

aalmanza1998 opened this issue Feb 23, 2020 · 11 comments
Assignees
Labels
vserver Vulnerable server

Comments

@aalmanza1998
Copy link
Contributor

aalmanza1998 commented Feb 23, 2020

We need to include the random str in the responses of /api/submissions/<course_id>/<assignment_id>/<student_id> and GET /api/submissions/<course_id>/<assignment_id>

@lxylxy123456 lxylxy123456 added the vserver Vulnerable server label Feb 23, 2020
@lxylxy123456
Copy link
Contributor

@aalmanza1998 can you remind me of how random strings look like?

@aalmanza1998
Copy link
Contributor Author

They are generated using:
random_str = base64.urlsafe_b64encode(os.urandom(9)).decode('ascii')

@Lawrence37
Copy link
Contributor

Is the random string really necessary in ngshare? It seems like it's only used to prevent unauthorized access to submissions. ngshare should be able to do that without a random string.

@lxylxy123456
Copy link
Contributor

We may want to prevent the case that a student submits two requests at the same time and accidentally generates two submissions with the same timestamp (Though it is very likely, I think there is no guarantee that the timestamps will be always different). So adding a random string can decrease this possibility of collision further.

@Lawrence37
Copy link
Contributor

The timestamp has microsecond precision, so it's extremely unlikely that there will be duplicate timestamps. If we are really so concerned about such a rare event, we should generate a unique submission ID for each submission and use that instead of a random number.

@aalmanza1998
Copy link
Contributor Author

What would that ID consist of?

@lxylxy123456
Copy link
Contributor

uuid is a good choice

@lxylxy123456
Copy link
Contributor

From python help:

# make a random UUID
>>> uuid.uuid4()    # doctest: +SKIP
UUID('16fd2706-8baf-433b-82eb-8c7fada847da')

It seems to be almost collision-free. e.g. https://softwareengineering.stackexchange.com/a/130298

I think you can first continue working with the current random string. Changing this should be easy in backend and require no work in frontend.

@aalmanza1998
Copy link
Contributor Author

@Lawrence37 are you having issues with notebook_hash or make_unique_key or are you just looking at alternatives?

@Lawrence37
Copy link
Contributor

Part of it is that retrieved submissions don't store the random string, and consequently, releasing feedback lacks the required random string. It would be great if we don't introduce a way to store the string in the filesystem so that we can maintain maximum compatibility with the current exchange.

The other reason is to avoid unnecessary clutter in the API.

@lxylxy123456
Copy link
Contributor

I kind of understand. Can you demonstrate it more tomorrow during / after the meeting? If reasonable I think we can remove random str from API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vserver Vulnerable server
Projects
None yet
Development

No branches or pull requests

3 participants