Problems with Likes-Implementation

changed milestone to %2021 July Sprint Sharing

added high priority label

ExerciseId and ProjectIds are still mixed up

Ok I did not see that, should be fixed now

How is it checked, that a user cannot like an exercise, he/she does not have access to? Can a user query likes from any other user?

@GetMapping("/hasLiked/{ExerciseID}") -> everybody can query for that but it only returns valid information if the user is logged in meaning a logged in user sents a request for exerciseID and then he gets the info if he has liked it or not. There is no possibility to check for other users. If a user is not logged in and he sends the request he will receive the boolean false for all ids.
@DeleteMapping("/unlikeExercise/{ExerciseID}") -> User has to be authenticated to call this and therefore the function will then use his id to delete the like so not other deletions possible
PutMapping("/likeExercise") - >Here is the same again an authorized user can send the request but the function will always take his userID with the following call: likesService.findLikesByUserIDandExerciseID(userService.getUserWithAuthorities().get().getId().intValue(), likes.getExerciseID());

LikesRepository: Performance quite often likes are queried by exerciseId (and UserId), however now database indices are defined on these columns.

I dont understand what you mean here @Michael.Breu

Naming does not really follow code conventions

In general on which code conventions are you refering to?

For the first three old = better so no change required

PUT api/likes *what is the difference to PUT api/likeExercise ? ->

The difference is that for /likes a full like object is sent and updated and with /likeExercise only the projectID is used to create or update the like -> the userID, date etc are all generated by the server I left the api/likes because maybe in future we need this endpoints right now they are only accessible by a user with admin rights

DELETE api/unlikeMapping DELETE api/user/likes/{exerciseId} see above on exerciseId with slashes Has been improved

GET api/numberOfLikes GET api/likes/{exerciseId} see above on exerciseId with slashes I would prefer to keep it like that because it is more descriptive what the method does and you

GET api/hasLiked GET api/user/likes/{exerciseId} see above on exerciseId with slashes is now changed to user/likes/hasLiked/**

How is it checked, that a user cannot like an exercise, he/she does not have access to? Can a user query likes from any other user?

@Get Mapping("/hasLiked/{ExerciseID}") -> everybody can query for that but it only returns valid information if the user is logged in meaning a logged in user sents a request for exerciseID and then he gets the info if he has liked it or not. There is no possibility to check for other users. If a user is not logged in and he sends the request he will receive the boolean false for all ids.

@DeleteMapping("/unlikeExercise/{ExerciseID}") -> User has to be authenticated to call this and therefore the function will then use his id to delete the like so not other deletions possible

PutMapping("/likeExercise") - >Here is the same again an authorized user can send the request but the function will always take his userID with the following call: likesService.findLikesByUserIDandExerciseID(userService.getUserWithAuthorities().get().getId().intValue(), likes.getExerciseID());

Dies ist leider nur die halbe Miete. Als Authentisierter Nutzer kann ich jetzt jede Aufgabe Liken, auch wenn ich keine Berechtigung dafür habe. Die ExerciseId dafür zu erraten ist nicht allzu schwierig.

Ich denke, es ist noch viel schlimmer. In

    @PutMapping("/likeExercise")
    public ResponseEntity<Likes> updateLikeWithExerciseID(@RequestBody Likes likes) {

wird nicht geprüft, ob im Likes-Objekt auch tatsächlich die UserId des authentisierten Nutzers hinterlegt ist. D.h. ein authentisierter Nutzer kann alle Likes für alle Nutzer und alle Aufgaben setzen (und vermutlich auch löschen).

Bitte unbedingt beachten: Alle Eingaben, die über REST-Services kommen sind immanent gefährlich. Es ist auf server-seite zu prüfen, ob der Zugriff zulässig ist.

Deshalb hat dieses Ticket die Priorität hoch!

LikesRepository: Performance quite often likes are queried by exerciseId (and UserId), however now database indices are defined on these columns.

I dont understand what you mean here @Michael.Breu

LikesService.findLikesByUserIDandExerciseID erzeugt ein SQL-Statement der Art ```SELECT * from LIKES WHERE user_id=... AND exercise_id='...'

Momentan sind keine Datenbank-Indices für diese Felder hinterlegt. D.h. wird die Tabelle größer, muss die Datenbank linear nach Hits durchsucht werden. Momentan bei 10 Einträgen kein Problem, bei 100.000 Einträgen sehr wohl.

Die Lösung ist, mit Liquibase einen index zu den Spalten user_id und exercise_id hinzuzufügen (https://docs.liquibase.com/change-types/community/create-index.html)

Damit reduzierst du den Suchaufwand von

n

auf

log(n)

.

Naming does not really follow code conventions

In general on which code conventions are you refering to?

For the first three old = better so no change required

Ja stimnt, die passen.

PUT api/likes *what is the difference to PUT api/likeExercise ? ->

The difference is that for /likes a full like object is sent and updated and with /likeExercise only the projectID is used to create or update the like -> the userID, date etc are all generated by the server I left the api/likes because maybe in future we need this endpoints right now they are only accessible by a user with admin rights

Hm, stimmt das?

    @PutMapping("/likeExercise")
    public ResponseEntity<Likes> updateLikeWithExerciseID(@RequestBody Likes likes) {

/likeExercise nimmt beliebige Like-Objekte (mit den Sicherheitsproblemen von oben)

DELETE api/unlikeMapping DELETE api/user/likes/{exerciseId} see above on exerciseId with slashes Has been improved

GET api/numberOfLikes GET api/likes/{exerciseId} see above on exerciseId with slashes I would prefer to keep it like that because it is more descriptive what the method does and you

GET api/hasLiked GET api/user/likes/{exerciseId} see above on exerciseId with slashes is now changed to user/likes/hasLiked/**

Das Problem ist, dass uns die momentanen Bezeichnungen das /api-Verzeichnis "zumüllen". Wenn wir so weiter machen, dann haben wir einigen hundert Bezeichner unter /api/. Schon jetzt sind es einige. Da kennt sich ziemlich schnell niemand mehr aus:

Deshalb möchte ich das mehr hierarchisch strukturieren.

mentioned in merge request !58 (merged)

assigned to @Michael.Breu and unassigned @eduard.frankford

assigned to @eduard.frankford and unassigned @Michael.Breu

Ich habe mir den Source Code durchgesehen.

Passt so weit, bis auf den Index.

Du baust einen Index aus allen 4 Spalten:

        <createIndex indexName="index_exerciseID" tableName="likes">
            <column name="exercise_id"></column>
            <column name="user_id"></column>
            <column name="id"></column>
            <column name="date"></column>

das ist nur dann sinnvoll, wenn Du nach allen vier Werten gleichzeitig suchst. Ich verstehe aber nicht, warum Du auch das Datum indizierst? Und nach der Spalte id wirst Du auch nicht zusammen mit den anderen drei Indices suchen.

Im LikesRepository gibt es nur Suchen nach exerciseId bzw. nach exerciseId und UserId zusammen.

Ich hab's jetzt korrigiert auf

    <changeSet id="20211407105122-1" author="eduard frankford">
        <createIndex indexName="index_exerciseID" tableName="likes">
            <column name="exercise_id"></column>
        </createIndex>
        <createIndex indexName="index_exerciseUserID" tableName="likes">
            <column name="exercise_id"></column>
            <column name="user_id"></column>
        </createIndex>

Ich fürchte, das wird jetzt einen Liquibase-Konflikt bei Dir geben.

Ich merge jetzt diesen Stand.

Michael

mentioned in commit fe8d51ba

closed

Problems with Likes-Implementation

Child items

Activity

ExerciseId and ProjectIds are still mixed up

How is it checked, that a user cannot like an exercise, he/she does not have access to? Can a user query likes from any other user?

LikesRepository: Performance quite often likes are queried by exerciseId (and UserId), however now database indices are defined on these columns.

Naming does not really follow code conventions

In general on which code conventions are you refering to?

How is it checked, that a user cannot like an exercise, he/she does not have access to? Can a user query likes from any other user?

LikesRepository: Performance quite often likes are queried by exerciseId (and UserId), however now database indices are defined on these columns.

Naming does not really follow code conventions

In general on which code conventions are you refering to?

Problems with Likes-Implementation

Child items

Linked items

Activity

ExerciseId and ProjectIds are still mixed up

How is it checked, that a user cannot like an exercise, he/she does not have access to? Can a user query likes from any other user?

LikesRepository: Performance quite often likes are queried by exerciseId (and UserId), however now database indices are defined on these columns.

Naming does not really follow code conventions

In general on which code conventions are you refering to?

How is it checked, that a user cannot like an exercise, he/she does not have access to? Can a user query likes from any other user?

LikesRepository: Performance quite often likes are queried by exerciseId (and UserId), however now database indices are defined on these columns.

Naming does not really follow code conventions

In general on which code conventions are you refering to?