commit for issue #12 #84
Conversation
Added the server-side validation. If a client is new joinee,he can create a new chatroom by sending NEW command or validate by CHKUSR command. The chatroom are generated at a single point,hence the disperancy coming in earlier case is resolved.
Modified the notify_mesej() at the server side. Currently the client establish a websocket to the server and server has multiple websockets for each client. Client's message is forwarded to all the users, not only to those that are present in the chatroom. Hence, I have modified it to forward to only those websockets corr. to same chatroom. Now client can't communicate outside his chatroom as well.
|
This contains two commit as first we have to apply the commit for issue#39 and then only for issue#12. |
| @@ -9,6 +9,7 @@ | |||
|
|
|||
| sess = PromptSession() | |||
| sepr = chr(969696) | |||
| websocket = None | |||
There was a problem hiding this comment.
I would strongly recommend against using such kind of initialization as that mutates the data class of the said object.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
Just a small question, Why do we refrain from using a global variable? Is it related to avoiding any kind of exploit or a general practise.
There was a problem hiding this comment.
It is standard programming practice to refrain from global variables in modular and object-oriented projects to prevent hidden issues from creeping up down the line when they would be hard to diagnose (for example, recognising which subroutines reach the global state, which in itself is bad, and in what order).
| @@ -29,7 +30,7 @@ def decrjson(self, data): | |||
| return self.suit.decrypt(data.encode("utf8")).decode("utf8") | |||
|
|
|||
|
|
|||
| async def consumer_handler(cphrsuit, websocket, username, chatroom, servaddr): | |||
| async def consumer_handler(cphrsuit, username, chatroom, servaddr): | |||
There was a problem hiding this comment.
The websocket object must be passed here as a parameter and should not be used a global variable.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| @@ -46,7 +47,7 @@ async def consumer_handler(cphrsuit, websocket, username, chatroom, servaddr): | |||
| pass | |||
|
|
|||
|
|
|||
| async def producer_handler(cphrsuit, websocket, username, chatroom, servaddr): | |||
| async def producer_handler(cphrsuit, username, chatroom, servaddr): | |||
There was a problem hiding this comment.
The websocket object must be passed here as a parameter and should not be used a global variable.
| async with websockets.connect(servaddr) as websocket: | ||
| try: | ||
| chkUserPresence = await chk_username_presence(websocket,username, chatroom) | ||
| if chkUserPresence == "False": | ||
| cphrsuit = fernetst(password.encode("utf8")) | ||
| prod = asyncio.get_event_loop().create_task(producer_handler(cphrsuit, websocket, str(username), str(chatroom), str(servaddr))) | ||
| cons = asyncio.get_event_loop().create_task(consumer_handler(cphrsuit, websocket, str(username), str(chatroom), str(servaddr))) | ||
| await websocket.send(username+sepr+chatroom) | ||
| await prod | ||
| await cons | ||
| asyncio.get_event_loop().run_forever() | ||
| else: | ||
| print_formatted_text(HTML("[" + obtntime() + "] " + "SNCTRYZERO > <red>Username already exist in chatroom</red>")) | ||
| await websocket.close() | ||
| sys.exit() | ||
| except Exception as EXPT: | ||
| if websocket.closed: | ||
| print_formatted_text(HTML("[" + obtntime() + "] " + "SNCTRYZERO > <red>A connection to the server was lost</red>".format(EXPT))) | ||
| raise KeyboardInterrupt |
There was a problem hiding this comment.
Your commit undoes a lot of work previously done by the other fellow contributors. Please push only those changes which fall in line with the contributions made by others. Improvements in small increments are much better than total inventive improvements.
There was a problem hiding this comment.
Hi @t0xic0der, I have made sure that none of the past work is made undo. I have incorporated their code in this and this code does exactly what was done before, except the small syntactic changes. The overall pragma of the code still remains the same, I believe
| geee = int(chatroom,16) | ||
| except ValueError: | ||
| return "False" | ||
| global websocket |
There was a problem hiding this comment.
Please abstain from using a global variable here as far as possible.
|
|
||
| def formusnm(username): | ||
| if len(username) < 10: return username + " " * (10 - len(username)) | ||
| elif len(username) > 10: return username[0:10] | ||
| else: return username | ||
|
|
||
| async def askserver(username,servaddr): | ||
| global websocket |
There was a problem hiding this comment.
Please abstain from using a global variable here as far as possible.
| class fernetst(): | ||
| def __init__(self, pswd): | ||
| self.suit = Fernet(pswd) | ||
|
|
||
| def encrjson(self, data): | ||
| return self.suit.encrypt(data.encode("utf8")).decode("utf8") | ||
|
|
||
| def decrjson(self, data): | ||
| return self.suit.decrypt(data.encode("utf8")).decode("utf8") |
There was a problem hiding this comment.
This totally defeats the purpose of end-to-end encryption. The server is able to read and understand the messages conveyed among the users.
There was a problem hiding this comment.
Well, I wasn't sure of that part either. But without generating the credentials at the server side, we always have risk of inconsistency and doing this at server side also prevents from creating same named chatroom. Also, in the earlier method of checking credentials we were justing checking if the password are valid ,not if they are correct. There may happen that a person enters a chatroom 12345678 with a wrong password(unknowingly) and then send some text(which he thinks is received at the other end) but in reality the other side is just reading some gibberish. I wasn't sure how to handle this.
| if USERS[websocket] == "": | ||
| if mesgjson.split(sepr)[0] == 'NEW': | ||
| new_room = randgene() | ||
| current_rooms.append(new_room) | ||
| USERS[websocket] = [mesgjson.split(sepr)[1], new_room] | ||
| password = Fernet.generate_key().decode("utf-8") | ||
| CHATROOM[new_room] = password | ||
| await websocket.send(new_room+sepr+password) | ||
| print_formatted_text(HTML("[" + obtntime() + "] " + "<b>USERJOINED</b> > <green>" + mesgjson.split(sepr)[1] + "@" + new_room + "</green>")) | ||
| await notify_mesej("SNCTRYZERO" + sepr + "USERJOINED" + sepr + mesgjson.split(sepr)[1] + sepr + new_room + sepr + str(getallus(new_room)),new_room) | ||
| elif mesgjson.split(sepr)[0] == 'CHKUSR': | ||
| # [query username chatroom password] | ||
| if mesgjson.split(sepr)[2] in current_rooms and mesgjson.split(sepr)[3] == CHATROOM[mesgjson.split(sepr)[2]] and str(chk_username_presence(mesgjson)) == "False": | ||
| await websocket.send("True") | ||
| USERS[websocket] = [mesgjson.split(sepr)[1],mesgjson.split(sepr)[2]] | ||
| print_formatted_text(HTML("[" + obtntime() + "] " + "<b>USERJOINED</b> > <green>" + mesgjson.split(sepr)[1] + "@" + mesgjson.split(sepr)[2] + "</green>")) | ||
| await notify_mesej("SNCTRYZERO" + sepr + "USERJOINED" + sepr + mesgjson.split(sepr)[1] + sepr + mesgjson.split(sepr)[2] + sepr + str(getallus(mesgjson.split(sepr)[2])),mesgjson.split(sepr)[2]) | ||
| else: | ||
| await websocket.send("False") | ||
| isInvalid = True | ||
| elif str(mesgjson) == '/list': | ||
| await send_chatroommembers_list(websocket) |
There was a problem hiding this comment.
This is great but try to make an implementation where the message is protected and only the room name is exposed.
There was a problem hiding this comment.
I will work on that and push it.
| cphrsuit = fernetst(password.encode("utf8")) | ||
| prod = asyncio.get_event_loop().create_task(producer_handler(cphrsuit, websocket, str(username), str(chatroom), str(servaddr))) | ||
| cons = asyncio.get_event_loop().create_task(consumer_handler(cphrsuit, websocket, str(username), str(chatroom), str(servaddr))) | ||
| await websocket.send(username+sepr+chatroom) | ||
| await prod | ||
| await cons | ||
| asyncio.get_event_loop().run_forever() |
There was a problem hiding this comment.
Always implement exception handling when you write programs and especially when you are replacing huge chunks of code with your own implementations. We will be testing your commits thoroughly before even thinking of merging it and this, along with lack of clarity to the changes, is only going to amplify our work and frustrations.
Fix for issue #12.