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

Hint request: do not allow unsigned requests #824

Closed
smadbe opened this issue Jan 19, 2022 · 9 comments · Fixed by #954
Closed

Hint request: do not allow unsigned requests #824

smadbe opened this issue Jan 19, 2022 · 9 comments · Fixed by #954
Assignees

Comments

@smadbe
Copy link
Contributor

smadbe commented Jan 19, 2022

The request for hint does not have to support unsigned (as json) requests. If the task has no public key for its platform, we can just consider this service cannot be called.

Please update the implementation and the doc (and the tests?) accordingly.


Initial issue:

As Thomas reported in France-ioi/AlgoreaFrontend#856, I think we have a problem with the format of the hint tokens for the hint token service.

This service takes as an arg a task token and a hint token. If we send both as JWTs, we get a "Invalid hint_requested: json: cannot unmarshal string into Go value of type map[string]formdata.Anything" error from the backend. However, if the hint token is decoded and sent a json, it works.

I suppose this is not the expected behavior of the service. Let me know if there is a misunderstanding from my side.

@smadbe smadbe added the Type: Bug Something isn't working label Jan 19, 2022
@zenovich
Copy link
Collaborator

It looks like the public key of the platform linked to the item is not set in the DB. When a platform's public key is null, the hint_requested parameter is expected to be JSON, not JWT

@zenovich
Copy link
Collaborator

mysql> select platform_id from items where id=773396873861865944;
+-------------+
| platform_id |
+-------------+
|           0 |
+-------------+
1 row in set (0.21 sec)

mysql> select * from platforms where id=0;
+----+---------+----------+------------+--------+----------+
| id | name    | base_url | public_key | regexp | priority |
+----+---------+----------+------------+--------+----------+
|  0 | default | NULL     | NULL       | .*     |        0 |
+----+---------+----------+------------+--------+----------+
1 row in set (0.21 sec)

@zenovich zenovich moved this from Todo to Blocked (waiting for Mathias, Michel or Damien's input) in Continuous Development Jan 20, 2022
@zenovich zenovich moved this from Blocked (waiting for Mathias, Michel or Damien's input) to Todo in Continuous Development Jan 21, 2022
@zenovich zenovich moved this from Todo to In progress in Continuous Development Jan 21, 2022
@smadbe smadbe assigned smadbe and unassigned zenovich Jan 28, 2022
@smadbe smadbe moved this from In progress to Blocked (waiting for Mathias, Michel or Damien's input) in Continuous Development Jan 28, 2022
@smadbe smadbe assigned zenovich and unassigned smadbe Feb 4, 2022
@smadbe smadbe moved this from Blocked (waiting for Mathias, Michel or Damien's input) to Todo in Continuous Development Feb 4, 2022
@smadbe smadbe changed the title Invalid expected format for hint token Hint request: do not allow unsigned requests Feb 4, 2022
@smadbe smadbe removed the Type: Bug Something isn't working label Feb 4, 2022
@zenovich zenovich moved this from Todo to In progress in Continuous Development Sep 20, 2022
@smadbe smadbe assigned GeoffreyHuck and unassigned zenovich Jan 26, 2023
@GeoffreyHuck
Copy link
Contributor

The same process/code is used by the service items saveGrade with a scoreToken. Should the change be made for this case too?

@smadbe
Copy link
Contributor Author

smadbe commented May 2, 2023

I have to ask @mblockelet ...

@smadbe
Copy link
Contributor Author

smadbe commented May 2, 2023

Michel's answer:

Lorsque le frontend appelle task.gradeAnswer, il reçoit deux éléments score et scoreToken, score étant un score de 0 à 100 toujours renseigné, scoreToken étant null pour les tâches sans token.
Du coup, pour les tâches sans clé, on enregistre simplement le score renvoyé ; sur l'ancienne plateforme, on vérifie quand même qu'il s'agit bien d'une tâche pour laquelle on n'a pas de clé avant d'autoriser ça.

@GeoffreyHuck
Copy link
Contributor

GeoffreyHuck commented May 3, 2023

Ok so if I understand correctly, there is no need to be able to pass either score_token in saveGrade, nor hint_token in AskHint as clear JSON.

If there is no key for the platform associated with the item:

  • askHint should return an error
  • saveGrade should only use the score attribute, and hint_token should not be present

Do I understand correctly?

@smadbe
Copy link
Contributor Author

smadbe commented May 10, 2023

Yes, looks good. At least, that's my understanding as well.

Just a small question for validation for @mblockelet : if the task uses keys but the platform does not have the key configured... when there is a saveGrade request with score+token do we reject the query (as Geoffrey suggested) or do we just ignore the token and let the score pass ?

@mblockelet
Copy link
Contributor

I'd say we ignore the token and let the plain score go through.

The score token is here to sign the score ; if a platform admin cares enough to require authenticated scores from specific tasks, then they should configure the key accordingly, but if they don't care then it's fine to allow for tasks which normally use tokens imo.

@smadbe
Copy link
Contributor Author

smadbe commented May 10, 2023

Ok. Note that the score hint request will still fail in such as case.

GeoffreyHuck added a commit that referenced this issue May 15, 2023
…if there is no key for the platform associated with the item

See discussion here: #824
GeoffreyHuck added a commit that referenced this issue May 15, 2023
…if there is no key for the platform associated with the item

See discussion here: #824
Continuous Development automation moved this from In progress to Done May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging a pull request may close this issue.

4 participants