France-ioi / AlgoreaBackend

Backend for the new Algorea platform
MIT License
1 stars 2 forks source link

Save grade: use score/answer as auth #1084

Closed GeoffreyHuck closed 2 months ago

GeoffreyHuck commented 2 months ago

fixes #1079

Remove authentication with access token for the service saveGrade. Use either the answer token or the score token.

Also added tests to check:

Representation of inputs in the service

The method is the same as the one that was already in use. If the input is answer token+score, we construct a score token from it. Then, in the rest of the service, we only use the score token to access the inputs. This avoid avoid if/else depending of whether the input consists of answer token+score or score token.

Unmarshalling of score token need to use the LocalItemID

The current system checks the signature at the same time of unmarshalling tokens. Problem: we need to use the LocalItemID inside the score token to know which platform key to use to unmarshal the score token itself. So a method token.GetUnsafeFromToken has been added, to be able to retrieve a parameter inside the token without checking the signature. Unsafe was added in the name to make sure this method isn't use in wrong situations.

Parsing and validation of tokens

The same method that was use to parse and validate the fields idUser, idItemLocal and idAttempt of task tokens and put them in a Converted sub structure during unmarshalling has been reused for answer tokens. This way is more consistent, and we are sure we are dealing with valid information when we access the content of the token.

Little uncertainty about score tokens

Some tests are using a score token with an undefined AttemptID and LocalItemID. So for now the parsing and validation of score token works if there is no AttemptID or LocalItemID. But maybe it should fail? Can it happen in reality?

Review

Easier to review all modifications at once as some refactoring were made. But the details of the changes can be seen in commit messages.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (a9f7dc3) to head (0352163).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1084 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 236 237 +1 Lines 14321 14329 +8 ========================================= + Hits 14321 14329 +8 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

smadbe commented 2 months ago

Some tests are using a score token with an undefined AttemptID and LocalItemID. So for now the parsing and validation of score token works if there is no AttemptID or LocalItemID. But maybe it should fail? Can it happen in reality?

But I don't get how you set the score to the result entry if the item_id or attempt_id is missing, so I cannot see how it may "work". If it is only on the token parsing, that's an internal decision you have to make, but I don't see cases where no having item_id or attempt_id makes sense.

smadbe commented 2 months ago

the PR closing was a mistake

GeoffreyHuck commented 2 months ago

The unmarshalling now fails if there is no idAttempt or idLocalItem, because score tokens are used only in this service, and they're required for the service to work.