Submissions api#1121
Conversation
…when calling endpoint
…xpiration with timezone
…into account differrent tasks previously did not differentiate tasks. The API would send the best result for each course, but not all tasks.
…hout a taskid given
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 3 critical 2 high |
| Complexity | 1 medium |
🟢 Metrics 41 complexity · 0 duplication
Metric Results Complexity 41 Duplication 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
anthonygego
left a comment
There was a problem hiding this comment.
I did a first pass of things that shoud be addressed before going further.
| JWT_SECRET = "your-secret-key" | ||
| JWT_ALGORITHM = "HS256" |
There was a problem hiding this comment.
This should be put in the webapp config
| JWT_SECRET = "your-secret-key" | ||
| JWT_ALGORITHM = "HS256" | ||
| TOKEN_LIFETIME = datetime.timedelta(days=365) # Token lifetime of 1 year |
There was a problem hiding this comment.
This should be put in the webapp config, and absolutely not hardcoded and repeated in files
| return handler(*args, **kwargs) | ||
|
|
||
|
|
||
| class APITokenAuthPage(APIAuthenticatedPage): |
There was a problem hiding this comment.
Why do you inherit from APIAuthenticatedPage and override the _verify_authentication method ? The goal is to replace the session-based authentication that is not relevant anymore.
| except jwt.InvalidTokenError: | ||
| raise APIForbidden("Invalid token, please generate a new one.") | ||
|
|
||
| self.user = User.objects(username=payload["username"]).first() |
There was a problem hiding this comment.
It's probably a good practice to check against the database too.
This is a good security practice. Imagine the secret key is lost (or discovered hardcoded in the public codebase...). Hackers can forge new tokens themselves, making the leak even more dangerous as everyone will be concerned.
By the way, you need to allow invalidating tokens before they expire.
| {{ _("Your API token allows external applications and scripts to authenticate as you and interact with " | ||
| "INGInious on your behalf, without needing your password. Keep it secret: anyone who has it can " | ||
| "access the API using your account permissions. If you believe your token has been compromised, " | ||
| "generate a new one below to invalidate the old one immediately. See the INGInious documentation " | ||
| "for usage.") }} |
There was a problem hiding this comment.
- The token is more or less a clear password with an expiration date that you store somewhere in automated scripts.
- This means you ideally need to provide a token per external source of authentication and not an only one.
This constraint can be imposed in a first implementation as this is not so critical but rather a convenience issue. However, there should be a way for a user to simply invalidate its curent token without generating a new one.
| user = User.objects(username=session["username"]).first() | ||
|
|
||
| try: | ||
| payload = jwt.decode(user.apitoken, JWT_SECRET, algorithms=[JWT_ALGORITHM]) |
There was a problem hiding this comment.
This, here, is a critical security issue and should be addressed.
I'm not sure people store tokens in their clear form in database. They rather show the clear token once it is generated and then keep a hash and expiration date in database, leaving the users on their own if they lose it.
As said before tokens may be seen as clear passwords. If there is a database leak or unauthorized access for a reason or another, the tokens can be used freely to act as another user.
Of course you can invalidate tokens once you know they have been leaked. But you need to know that. Leaks can be orchestrated by "legitimate" people having access to database backups for instance.
| class APISubmissionsTest(APITokenAuthPage): | ||
|
|
||
| def API_GET(self): | ||
| """ | ||
| Test endpoint. | ||
| Returns a 200 OK if the endpoint is reachable and the user has access to it. | ||
| Returns 403 Forbidden if the user does not have access to the course/task. | ||
| Returns 404 Not Found if the course/task does not exist. | ||
| """ | ||
|
|
||
|
|
||
| # Does APITokenAuthPage automatically checks for APIForbidden? -> normally yes, | ||
|
|
||
| # TODO : get user from token, for now we use a hardcoded username | ||
|
|
||
| return 200, {"message": "Submissions endpoint is reachable and user has access."} |
This PR adds two endpoints to retrieve submissions using a token and a way for a user to generate it.
The token can be generated from the user preferences, in the page "API token". It is implemented using a JWT, allowing an automatic expiration.
The endpoints :
/api/v0/token/courses/<courseid>/submissions/api/v0/token/courses/<courseid>/<taskid>/submissionsBoth are POST endpoint due to the use of parameters passed through a JSON body. Passing them as arguments or parameters through the url could become cumbersome.
That body can contain :
Yet to bee implemented :