CS3219-AY2324S1 / ay2324s1-course-assessment-g05

A collaborative technical interview preparation platform that is going to boost your interview performance!
MIT License
0 stars 3 forks source link

Create session manager for collaboration service #86

Closed weidak closed 1 year ago

weidak commented 1 year ago
c0j0s commented 1 year ago

Navigating to collab service.

  1. When both parties are ready, the owner will choose to start.
  2. on start, owner will be in loading view, partner will remain with a loading status.image
  3. The owner contacts the problem service for a question.
  4. Once a question is selected, the owner will fire a notify_start event with the problem id.

The issue now is that once the notify event is fired, both client will leave and enter collab service together. So who will be setting up the room and how to notify the partner once the room is ready?

Currently i could think of 3 approaches:

@weidak @tryyang2001 what you guys think?

weidak commented 1 year ago

matching service will generate the room id, so both client can contact the collab service at the same time. Collab service can join the client socket to the room id immediately.

I think this approach sounds the best in terms of separating the microservices as well as performing the room creation service inside collab because it is quicker and much less coupled with your matching service as compared to the other 2. There will also be little implementation changes in this case and you can still discard the room id after firing it to notify_start event because the roomId is then cached on our side along with the two user's ids. Once we cache them, we will let the users join the room.

I feel that the collaboration workspace/session will open even if one of them drops/disconnect/doesn't join at all, because they can always just reconnect with the same user id so long as it can be found in the cache. Our UI currently tells the user whether they are connected to the service, and who is our partner (with the DP) but we can also add in features to tell the other party that his partner is disconnected.

Anyways i think having an owner that decides the creation of room has more error handling to be done because if he creates a room and he drops, we will assign the partner the new owner, and this brings in another level of management of inheriting the session rights. Do let me know if im missing out on any concepts though

c0j0s commented 1 year ago

Agreed, I also feel that this approach is most clear in terms of separating the services, plus since i have some encoding function in matching service alr.

So to confirm, the matching lobby will now redirect both to /collab/room with a unique roomid, partnerid, problemid and language.

weidak commented 1 year ago

Ok can

c0j0s commented 1 year ago

On second thought, partnerId seems to be redundant since both will enter the room with the unique room id.

this is sufficient? /collaboration/room/<roomId>?q=<questionId>&l=<language>

tryyang2001 commented 1 year ago

No, partnerId is important for collaboration so that the page knows which user avatar it should render for the partner. Therefore we definitely need the partner id. Unless you provide an API which I can query the participants by room id, but I doubt you want to create database for matching service right?

c0j0s commented 1 year ago

the issue is that if we supply a partner in the param, will collab fetch other users' data if someone modifies it manually? changing the questionId and language is fine as it makes sense to be modifiable.

will it be more secure for the partner to broadcast who they are once they join a room?

tryyang2001 commented 1 year ago

will collab fetch other users' data if someone modifies it manually? changing the questionId and language is fine as it makes sense to be modifiable.

So you are saying the user might modify the url parameters right? We need a way to prevent this, although I currently have no idea how to do that haha. But I don't think that even make sense for a user to be able to modify the question/language/partner even before the collaboration workspace has been created.

will it be more secure for the partner to broadcast who they are once they join a room?

How do u think the broadcast can be done? Through sending a message in the socket?

weidak commented 1 year ago

@c0j0s you make a valid point with the changing of variables userId actually (and I was gonna comment that I could handle the broadcasting of userId in the socket) but @tryyang2001 also makes another valid point cos if a user can change the other variables it will also inevitably break something (ie they can be coding in different languages/questions)

Will it be most secure if our services communicate through redis then...

weidak commented 1 year ago

Our TA for our tutorial did say that his group focused too much on security and it was just not very significant in the whole package so they ditched it.. (the ones who demoed in lecture) I would say let's not worry too much and decide on a relatively nonsecure way so this does not impede our development

c0j0s commented 1 year ago

how abt this, ill pass this over /collaboration/room/<roomId>?p=<partnerId>&q=<questionId>&l=<language>

but the roomId is a checksum of ownerId, partnerId, questionId and language, then collab page just needs to compute the checksum to see if they match before creating the room.

since the roomId will get discarded after the collab session, issues with id collision are not significant.

what you guys think?

weidak commented 1 year ago

Ok that sounds like a good idea! Lmk the checksum u used

tryyang2001 commented 1 year ago

So instead of passing to a non-existing /collaboration page, you will directly pass to the dynamic /collaboration/room/:roomId page with the proper params right? By the way I know that you are just illustrating the idea, but I will encourage we used full name for query parameters for clarity, like questioId=<questionId>, language=<language>, what do you think? Meanwhile to prevent the path is too obvious for the user to change, we can hash the params.

c0j0s commented 1 year ago

yes, to the room directly, I'll use fullnames in the path. but what do you mean by hash the params?

tryyang2001 commented 1 year ago

Hmm, like sometimes we see the path name is like /collaboration/room/aHjnfdapkNKpskan4jLNF (ok this is just an example of a random string). I am not sure if we can hash the details of roomId, partner, question, and language?

c0j0s commented 1 year ago

I dont think we can hash the params, as hashing is unidirectional, we won't have a way to dehash it. unless u meant encryption... which is another level of complication 😅.

tryyang2001 commented 1 year ago

I think there is a library called bcrypt that can help us restore the hash easily, as long as we agree to a salt? Not sure if I remembered this correctly, might want to ask @xingjie99 or @tlyi as they are more familiar with that package haha.

tryyang2001 commented 1 year ago

So if that is doable, I am more prone to we compute a checksum + hashing, what do you think?

weidak commented 1 year ago

...i don't think its a common practice to hash parameters. It's on them for breaking their own session

tryyang2001 commented 1 year ago

Can refer to this forum post, this is what I mean haha. I think I might deliver the wrong message as I am not planning to hash just the value of each parameter, but the entire query params. See the HMAC approach.

But yea, I totally agree that security is not our top priority, if you think implementing all these is too complicated and does not add many benefits, I am fine with just ignoring it.`

tryyang2001 commented 1 year ago

Closed this issue as the related PR has been merged to master.