Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ICtrl logging integration for application/profile and general application files #43

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 64 additions & 12 deletions application/Profile/DBProfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import base64
import json
import logging
import os
import re
import uuid
Expand Down Expand Up @@ -55,6 +56,9 @@ class ActivationType(IntEnum):
NORMAL_USER = 1


logger = logging.getLogger(__name__)


class DBProfile(Profile):

def __init__(self, app):
Expand All @@ -67,13 +71,21 @@ def __init__(self, app):
self.salt = bcrypt.gensalt()

# key: user_id, value: activation code
self.activation_cache = TTLCache(maxsize=1024, ttl=ACTIVATION_TTL_SECOND)
size = 1024
li-ruihao marked this conversation as resolved.
Show resolved Hide resolved
self.activation_cache = TTLCache(maxsize=size, ttl=ACTIVATION_TTL_SECOND)
logger.debug("activation_cache set up with %d, expiration time = %d", size, ACTIVATION_TTL_SECOND)
li-ruihao marked this conversation as resolved.
Show resolved Hide resolved

# key: user_id, value: True (to indicate the entry exists; can be any dummy value)
self.resend_cooldown = TTLCache(maxsize=1024, ttl=RESEND_COOLDOWN_TTL_SECOND)
self.resend_cooldown = TTLCache(maxsize=size, ttl=RESEND_COOLDOWN_TTL_SECOND)
logger.debug("resend_cooldown cache set up with %d, expiration time = %d", size, RESEND_COOLDOWN_TTL_SECOND)
li-ruihao marked this conversation as resolved.
Show resolved Hide resolved

with open('/var/www/ictrl/application/resources/activation_email_template.html', 'r') as f:
self.activation_email_body_template = f.read()
activation_email_template = '/var/www/ictrl/application/resources/activation_email_template.html'
li-ruihao marked this conversation as resolved.
Show resolved Hide resolved
logger.debug("Opening %s in read-only mode", activation_email_template)
li-ruihao marked this conversation as resolved.
Show resolved Hide resolved
try:
with open(activation_email_template) as f:
self.activation_email_body_template = f.read()
except IOError as e:
logger.exception("Failed to open %s, does file exist? Error: %s", activation_email_template, e)
li-ruihao marked this conversation as resolved.
Show resolved Hide resolved
li-ruihao marked this conversation as resolved.
Show resolved Hide resolved
li-ruihao marked this conversation as resolved.
Show resolved Hide resolved

class User(db.Model):
__table_args__ = {"schema": "ictrl"}
Expand All @@ -85,8 +97,11 @@ class User(db.Model):

@validates('username')
def validate_username(self, key, username):
assert re.match("^[A-Za-z0-9_-]+$", username), \
'User name should contain only letters, numbers, underscores and dashes'
username_error = 'User name should contain only letters, numbers, underscores and dashes'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we want to use assert? If the purpose is to log the erroneous username, can we write this instead?

assert re.match("^[A-Za-z0-9_-]+$", username), \
    'User name should contain only letters, numbers, underscores and dashes: "%s"' % username

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The suggested line would not log the message, but the assertion error will be raised with the specified message instead. Is this the preferred behaviour?

if not re.match("^[A-Za-z0-9_-]+$", username):
logger.error(username_error)
raise AssertionError(username_error)

return username

# this field is for the hashed passwords
Expand All @@ -97,16 +112,23 @@ def validate_username(self, key, username):

@validates('email')
def validate_email(self, key, email):
assert re.match(r'\b[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Z|a-z]{2,}\b', email), \
f'Invalid email address: "{email}"'
invalid_email_error = f'Invalid email address: "{email}"'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto: why don't we want to use assert which is more concise?

if not re.match(r'\b[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Z|a-z]{2,}\b', email):
logger.error(invalid_email_error)
raise AssertionError(invalid_email_error)

# FIXME: remove this utoronto mail restriction in the future
assert email.endswith('utoronto.ca'), "Currently, only Uoft emails are supported"
not_uoft_email_error = "Currently, only UofT emails are supported, emails must end with utoronto.ca"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

if not email.endswith('utoronto.ca'):
logger.error(not_uoft_email_error)
raise AssertionError(not_uoft_email_error)

return email

activation_type = db.Column(db.Integer, nullable=False, default=ActivationType.NOT_ACTIVATED)

logger.info("Defined a database table: User")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this? Do we expect the class definition to fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially added to ensure normal program execution, i.e to give information that the table has been successfully defined.


class Session(db.Model):
__table_args__ = {"schema": "ictrl"}

Expand All @@ -118,27 +140,35 @@ class Session(db.Model):

@validates('nickname')
def validate_nickname(self, key, nickname):
assert len(nickname) <= 8, \
'Entered nickname is too long'
nickname_too_long = 'Entered nickname is too long'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

if len(nickname) > 8:
logger.error(nickname_too_long)
raise AssertionError(nickname_too_long)

return nickname

username = db.Column(db.String, nullable=False)
private_key = db.Column(db.Text, nullable=True)

logger.info("Defined a database table: Session")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto


class VNCCredentials(db.Model):
__table_args__ = {"schema": "ictrl"}

session_id = db.Column(UUID(as_uuid=True), db.ForeignKey('ictrl.session.id'), primary_key=True,
nullable=False)
credentials = db.Column(db.Text, nullable=False)

logger.info("Defined a database table: VNCCredentials")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto


self.User = User
self.Session = Session
self.VNCCredentials = VNCCredentials

# db.drop_all()
db.engine.execute("CREATE SCHEMA IF NOT EXISTS ictrl;")
db.create_all()
logger.info("Created database SCHEMA ictrl and created all databases defined")
li-ruihao marked this conversation as resolved.
Show resolved Hide resolved

def login(self, username, password):
username = username.lower()
Expand Down Expand Up @@ -169,7 +199,8 @@ def login(self, username, password):
@staticmethod
def logout():
# remove the username from the session if it's there
flask_session.pop('userid', None)
userid = flask_session.pop('userid', None)
logger.info("Removed session user: %s", userid)
li-ruihao marked this conversation as resolved.
Show resolved Hide resolved

return True

Expand All @@ -188,6 +219,8 @@ def query(self):
"username": session.username
}

logger.info("Query user sessions successful")
li-ruihao marked this conversation as resolved.
Show resolved Hide resolved

return _profile

def add_user(self, username, password, email):
Expand Down Expand Up @@ -236,6 +269,8 @@ def activate_user(self, userid, code):
user.activation_type = ActivationType.NORMAL_USER
self.save_profile()

logger.info("Successfully activated user with userid=%s", userid)

return True

return False
Expand All @@ -249,6 +284,7 @@ def get_user(self):
if user is None:
abort(403, 'Cannot find any matching record')

logger.info("Successfully retrieved user with userid=%s", userid)
return user

def add_session(self, host, username, conn=None):
Expand All @@ -273,6 +309,8 @@ def add_session(self, host, username, conn=None):
session.private_key = f.encrypt(clear_private_key).decode('ascii')

self.save_profile()

logger.info("Successfully added a new session: session_id = %s", session.id)
li-ruihao marked this conversation as resolved.
Show resolved Hide resolved
except AssertionError as e:
abort(403, e)
except sqlalchemy.exc.IntegrityError as e:
Expand All @@ -285,6 +323,7 @@ def _get_session(self, session_id):
abort(403, 'You are not logged in')
userid = flask_session['userid']

logger.info("Returning session, session_id = %s", session_id)
li-ruihao marked this conversation as resolved.
Show resolved Hide resolved
return self.Session.query.filter_by(id=session_id, user_id=userid).first()

def delete_session(self, session_id):
Expand All @@ -295,6 +334,8 @@ def delete_session(self, session_id):
self.db.session.delete(session)
self.save_profile()

logger.info("Successfully deleted session, session_id = %s", session_id)
li-ruihao marked this conversation as resolved.
Show resolved Hide resolved

return True, ''

def change_host(self, session_id, new_host):
Expand All @@ -305,14 +346,18 @@ def change_host(self, session_id, new_host):
session.host = new_host
self.save_profile()

logger.info("Successfully changed host for session, session_id = %s", session_id)
li-ruihao marked this conversation as resolved.
Show resolved Hide resolved

return True, ''

def save_profile(self):
self.db.session.commit()
logger.info("Profile saved")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention behind is so that the log viewer notices that the profile has been saved, in case if the log viewer wants to ensure that the db_profile was successfully saved. Unless this operation is guaranteed to be successful, where data modification and validation has been done else where, else I think it could be helpful to inform the log viewer that this operation was successful.


def get_session_info(self, session_id):
session = self._get_session(session_id)
if session is None:
logger.debug("Session %s does not exist, cannot retrieve session info", session_id)
li-ruihao marked this conversation as resolved.
Show resolved Hide resolved
li-ruihao marked this conversation as resolved.
Show resolved Hide resolved
return None, None, None, None, None

f = Fernet(flask_session['session_crypt_key'])
Expand All @@ -329,6 +374,8 @@ def set_session_nickname(self, session_id, nickname):
session.nickname = nickname
self.save_profile()

logger.info("Successfully set session nickname=%s for session %s", nickname, session_id)

return True, ''

def set_session_vnc_credentials(self, session_id, credentials):
Expand All @@ -340,6 +387,7 @@ def set_session_vnc_credentials(self, session_id, credentials):
# it is a delete request
vnc_credential = self.VNCCredentials.query.filter_by(session_id=session_id).first()
self.db.session.delete(vnc_credential)
logger.info("Successfully deleted vnc credentials for session %s", session_id)
else:
# it is an add / update request
json_str = json.dumps(credentials)
Expand All @@ -352,12 +400,14 @@ def set_session_vnc_credentials(self, session_id, credentials):
# add
vnc_credential = self.VNCCredentials(session_id=session_id, credentials=base64_str)
self.db.session.add(vnc_credential)
logger.info("Successfully added/updated vnc credentials for session %s", session_id)

self.save_profile()

return True, ''

def get_session_vnc_credentials(self, session_id):
logger.debug("Getting vnc credentials for session: %s", session_id)
li-ruihao marked this conversation as resolved.
Show resolved Hide resolved
session = self._get_session(session_id)
if session is None:
return False, f'failed: session {session_id} does not exist'
Expand Down Expand Up @@ -389,4 +439,6 @@ def send_activation_email(self, username):
expire_min=int(ACTIVATION_TTL_SECOND / 60))
send_email(user.email, 'Activate Your iCtrl Account', body)

logger.info("Successfully sent out activation email to email=%s", user.email)

Comment on lines +439 to +440
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid logging sensitive user information

Logging user email addresses can expose personal identifiable information (PII) in the logs. This could pose security and privacy risks. Consider removing the email address from the log message or anonymizing it.

Apply this diff to modify the log message:

-logger.info("Successfully sent out activation email to email=%s", user.email)
+logger.info("Successfully sent out activation email to user ID=%s", user.id)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
logger.info("Successfully sent out activation email to email=%s", user.email)
logger.info("Successfully sent out activation email to user ID=%s", user.id)

return True
Loading
Loading