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 all 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
73 changes: 61 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 All @@ -40,7 +41,9 @@
from .Profile import Profile
from ..utils import send_email, validate_password

ACTIVATION_EMAIL_TEMPLATE = '/var/www/ictrl/application/resources/activation_email_template.html'
ACTIVATION_TTL_SECOND = 60 * 30
MAX_PENDING_ACTIVATION_NUM = 1024
RESEND_COOLDOWN_TTL_SECOND = 30

SESSION_CRYPT_SALT = b'>@\x05[N%\xcf]+\x82\xc3\xcd\xde\xa6a\xeb'
Expand All @@ -55,6 +58,9 @@ class ActivationType(IntEnum):
NORMAL_USER = 1


logger = logging.getLogger(__name__)


class DBProfile(Profile):

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

# key: user_id, value: activation code
self.activation_cache = TTLCache(maxsize=1024, ttl=ACTIVATION_TTL_SECOND)
self.activation_cache = TTLCache(maxsize=MAX_PENDING_ACTIVATION_NUM, ttl=ACTIVATION_TTL_SECOND)

# 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=MAX_PENDING_ACTIVATION_NUM, ttl=RESEND_COOLDOWN_TTL_SECOND)

with open('/var/www/ictrl/application/resources/activation_email_template.html', 'r') as f:
self.activation_email_body_template = f.read()
try:
with open(ACTIVATION_EMAIL_TEMPLATE) as f:
self.activation_email_body_template = f.read()
except OSError as e:
logger.exception('Failed to open "%s"', ACTIVATION_EMAIL_TEMPLATE)
raise e
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Use raise instead of raise e to preserve traceback

When re-raising an exception, using raise without specifying the exception variable preserves the original traceback, which is helpful for debugging. Replace raise e with raise to maintain the full traceback and exception context.

Apply this diff to implement the change:

 except OSError as e:
     logger.exception('Failed to open "%s"', ACTIVATION_EMAIL_TEMPLATE)
-    raise e
+    raise
📝 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
raise e
raise
🧰 Tools
🪛 Ruff (0.8.2)

86-86: Use raise without specifying exception name

Remove exception name

(TRY201)


class User(db.Model):
__table_args__ = {"schema": "ictrl"}
Expand All @@ -85,8 +95,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 +110,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 +138,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("Database initialization is done.")

def login(self, username, password):
username = username.lower()
Expand Down Expand Up @@ -169,7 +197,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 user session: %s", userid)

return True

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

logger.info("Query user sessions successful, all user sessions:\n%s", json.dumps(_profile))

Comment on lines +220 to +221
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 data

Logging the entire user profile, including session information, may expose sensitive data in the logs. This could lead to security and privacy concerns. Consider removing this log statement or ensure that it does not include any sensitive information.

Apply this diff to remove the logging of sensitive data:

-logger.info("Query user sessions successful, all user sessions:\n%s", json.dumps(_profile))
+logger.info("User sessions queried successfully.")
📝 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("Query user sessions successful, all user sessions:\n%s", json.dumps(_profile))
logger.info("User sessions queried successfully.")

return _profile

def add_user(self, username, password, email):
Expand Down Expand Up @@ -236,6 +267,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 +282,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 +307,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)
except AssertionError as e:
abort(403, e)
except sqlalchemy.exc.IntegrityError as e:
Expand All @@ -295,6 +331,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)

return True, ''

def change_host(self, session_id, new_host):
Expand All @@ -305,14 +343,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)

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("Cannot retrieve session info: session_id=%s", session_id)
return None, None, None, None, None

f = Fernet(flask_session['session_crypt_key'])
Expand All @@ -329,6 +371,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 +384,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 +397,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 +436,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