-
Notifications
You must be signed in to change notification settings - Fork 14
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
base: main
Are you sure you want to change the base?
Changes from all commits
fa6f811
b692a39
4a48a1f
fea6f8f
5636256
bcb03e0
5e0b6d2
b6c0756
55c57ca
edccf85
a86f91f
49d84fc
72097f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -20,6 +20,7 @@ | |||||||
|
||||||||
import base64 | ||||||||
import json | ||||||||
import logging | ||||||||
import os | ||||||||
import re | ||||||||
import uuid | ||||||||
|
@@ -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' | ||||||||
|
@@ -55,6 +58,9 @@ class ActivationType(IntEnum): | |||||||
NORMAL_USER = 1 | ||||||||
|
||||||||
|
||||||||
logger = logging.getLogger(__name__) | ||||||||
|
||||||||
|
||||||||
class DBProfile(Profile): | ||||||||
|
||||||||
def __init__(self, app): | ||||||||
|
@@ -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 | ||||||||
|
||||||||
class User(db.Model): | ||||||||
__table_args__ = {"schema": "ictrl"} | ||||||||
|
@@ -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' | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why don't we want to use
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||||
|
@@ -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}"' | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto: why don't we want to use |
||||||||
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" | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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") | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"} | ||||||||
|
||||||||
|
@@ -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' | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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") | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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") | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||||||||
|
@@ -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 | ||||||||
|
||||||||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
|
||||||||
return _profile | ||||||||
|
||||||||
def add_user(self, username, password, email): | ||||||||
|
@@ -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 | ||||||||
|
@@ -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): | ||||||||
|
@@ -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: | ||||||||
|
@@ -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): | ||||||||
|
@@ -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") | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need this? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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']) | ||||||||
|
@@ -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): | ||||||||
|
@@ -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) | ||||||||
|
@@ -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' | ||||||||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
|
||||||||
return True |
There was a problem hiding this comment.
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 ofraise e
to preserve tracebackWhen re-raising an exception, using
raise
without specifying the exception variable preserves the original traceback, which is helpful for debugging. Replaceraise e
withraise
to maintain the full traceback and exception context.Apply this diff to implement the change:
📝 Committable suggestion
🧰 Tools
🪛 Ruff (0.8.2)
86-86: Use
raise
without specifying exception nameRemove exception name
(TRY201)