-
Notifications
You must be signed in to change notification settings - Fork 16
ICtrl logging integration for application/profile and general application files #43
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
base: main
Are you sure you want to change the base?
Changes from 13 commits
fa6f811
b692a39
4a48a1f
fea6f8f
5636256
bcb03e0
5e0b6d2
b6c0756
55c57ca
edccf85
a86f91f
49d84fc
72097f0
c0e884f
cef2743
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' | ||
|
Owner
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
Contributor
Author
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}"' | ||
|
Owner
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
Contributor
Author
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. Same discussion as above |
||
| 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" | ||
|
Owner
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
Contributor
Author
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. same discussion as above |
||
| 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") | ||
|
Owner
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?
Contributor
Author
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. |
||
|
|
||
|
Comment on lines
+128
to
+129
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. 🧹 Nitpick (assertive) Info logs on table definitions. These logs help confirm that ORM mappings are set, though consider downgrading to debug if it’s too verbose for production. |
||
| 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' | ||
|
Owner
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
Contributor
Author
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. same discussion as above |
||
| 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") | ||
|
Owner
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
Contributor
Author
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. added to ensure normal program execution, can delete |
||
|
|
||
|
Comment on lines
+151
to
+152
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. 🧹 Nitpick (assertive) Consistency in table definition logs. Same note as with the |
||
| 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") | ||
|
Owner
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
Contributor
Author
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. added to ensure normal program execution, can delete |
||
|
|
||
|
Comment on lines
+160
to
+161
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. 🧹 Nitpick (assertive) Consistent logging for VNCCredentials table. Again, consider using debug level for table creation logs to reduce noise in production. |
||
| 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.") | ||
|
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. 🧹 Nitpick (assertive) Database initialization log. Clear message. Keep or downgrade to debug if repeated many times. |
||
|
|
||
| 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)) | ||
|
|
||
li-ruihao marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| 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") | ||
|
Owner
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?
Contributor
Author
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. |
||
|
|
||
|
Comment on lines
+352
to
353
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. 🧹 Nitpick (assertive) Profile save confirmation. This is helpful. If you prefer to avoid log bloat, consider |
||
| 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) | ||
|
|
||
li-ruihao marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| return True | ||
Uh oh!
There was an error while loading. Please reload this page.