🐛 BUG: User data leaks

[PatrickSkowronek]

User Story

As a: User

I want: that my user data are secured enough to trust the platform.

The getUser route https://github.com/h-da/geli/blob/b9a07690d21dfbecd552196ce59abb50259634bf/api/src/controllers/UserController.ts#L270 returns a User object with data which should not see other students. Like progress, E-Mail and lastVisitedCourse.

My suggestions would be the following constaltion:

Student:
Can see Names and Profile picture from any other profile.
Can see all Information from his own Profile, and can change them, expect uid

Teacher:
Can see Names, E-Mail, uid, Profilepic
If the teacher is course admin or teacher in a course where is progress from a student then this should send as well.

Something like lastvisitedCourse should not be send!
Admin:
Same as Teacher, maybe more.

At least the uid is not send to students and the password as well.
@torss what do you think?

[torss]

Seems sensible!

I reckon creating multiple specific interfaces - i.e. as done in the recent PR #664 to fix the course leaks - would not make any sense here? (Especially not, if we consider other user roles in the future. Or perhaps even a more generalized system altogether.)

So the only suggestion I have right now is to rename the issue to "🐛 BUG: getUser leak" (or anything else with the "🐛 BUG: " prefix), since I would consider it unintended behavior i.e. a bug. Do you agree?

[PatrickSkowronek]

@torss sure, sounds good, and I tried to change the title but ubuntu dosen't like it, but you can do it!

[torss]

It appears that user data leaks occur in other places besides getUser.
E.g. when editing a course, all members (students & teachers) are transmitted https://github.com/h-da/geli/blob/53c8ba08f2ce7a9bcf3351c54e70112a228de622/api/src/controllers/CourseController.ts#L334-L335 (PR #654 didn't address / consider user leaks) and the (course edit) user search https://github.com/h-da/geli/blob/53c8ba08f2ce7a9bcf3351c54e70112a228de622/api/src/controllers/UserController.ts#L178 is of course unfiltered as well. (Note that both of these routes are restricted to teachers / admins, thus these particular leaks are not really critical.)

I currently intend to put new generalized filter functions in User.ts / Course.ts and do some UserController.ts refactoring (not necessarily related to this issue) while we are at it. (And I will rename this issue accordingly.)

[PatrickSkowronek]

@torss good idea. I know there are properly even more leaks 😞 . But the populate methodes are needed by getUser and getCourse for #598 . So to change that this will be leak free should be the main focus.

[torss]

This will also require at least some front end changes, since the email address is currently used in https://github.com/h-da/geli/blob/19afbb653729391511c679ef827a9c781ed1b00b/app/webFrontend/src/app/models/User.ts#L27 to derive the hash for the gravatar URL.

[PatrickSkowronek]

@torss uhh that's bad, we properly move this to the backend. Or use a stock image for all new User. We should discuss that

[torss]

@PatrickSkowronek The current workaround in the branch is to send the email hash for gravatar separately if the email is meant to be unavailable: https://github.com/h-da/geli/blob/2d213d8304c70b399d3e89bdc4fdc15701011347/app/webFrontend/src/app/models/User.ts#L27 https://github.com/h-da/geli/blob/826b6b76a206575b87e6a33b3f91117ff308c93c/shared/models/IUserSubSafe.ts#L1-L14
This is of course somewhat messy and it would probably be better to unify this for all users.
I see two plausible options for this unification:

1. Option: Give the IUser a gravatar field and put that in the back end schema as a virtual field. The front end User.ts would practically remain as it is now in the branch.
2. Option: Create a generalized IPicture (or maybe IImage) interface that dynamically specifies / stores its type, which could then be either a gravatar URL part or an IFile. Replace the IUser* picture: IFile; type(s) with that new interface and change all depended code accordingly. This would certainly be a larger change than option 1, but it would also add more flexibility for any possible future changes.

In both cases the current IUserSubSafe would become redundant and be replaced by IUserSubSafeBase (with slight modifications to match the modified IUser interface).

However, for now we might just want to keep the current workaround in the branch as it is and improve this later in a separate issue (especially when considering option 2), due to time constraints.
What do you think?

[PatrickSkowronek]

@torss yeah we should use the id instead of the e-mail, since people who don't upload a image don't really care that image is displayed.