Added documentation FS Session

lodel/plugins/filesystem_session/main.py

@@ -8,22 +8,45 @@ import os
 import pickle
 import re
 import time
-
+from .filesystem_session import FileSystemSession
 from lodel.context import LodelContext
+
+
 LodelContext.expose_modules(globals(), {
     'lodel.logger': 'logger',
     'lodel.auth.exceptions': ['ClientAuthenticationFailure'],
     'lodel.settings': ['Settings']})
 
-from .filesystem_session import FileSystemSession
-
 __sessions = dict()
 
 SESSION_TOKENSIZE = 150
 
 
-## @brief generates a new session token
+##
+# @brief generates a new session token
+#
 # @return str
+#
+# @warning The tokensize should absolutely be used as set! os.urandom function
+#            takes a number of bytes as a parameter, dividing it by 2 is an
+#            extremely dangerous idea as it drastically decrease the token expected 
+#            entropy expected from the value set in configs.
+# @remarks There is no valid reason for checking the generated token uniqueness:
+#        - checking for uniqueness is slow ;
+#        - keeping a dict with a few thousand keys of hundred bytes also is
+#            memory expensive ;
+#        - should the system get distributed while sharing session storage, there
+#            would be no reasonable way to efficiently check for uniqueness ;
+#        - sessions do have a really short life span, drastically reducing
+#            even more an already close to inexistent risk of collision. A 64 bits
+#            id would perfectly do the job, or to be really cautious, a 128 bits
+#            one (actual size of UUIDs) ;
+#        - if we are still willing to ensure uniqueness, then simply salt it
+#            with a counter, or a timestamp, and hash the whole thing with a 
+#            cryptographically secured method such as sha-2 if we are paranoids
+#            and trying to avoid what will never happen, ever ;
+#        - sure, two hexadecimal characters is one byte long. Simply go for 
+#            bit length, not chars length.
 def generate_token():
     token = binascii.hexlify(os.urandom(SESSION_TOKENSIZE//2))
     if token in __sessions.keys():
@@ -31,9 +54,18 @@ def generate_token():
     return token.decode('utf-8')
 
 
-## @brief checks the validity of a given session token
+##
+# @brief checks the validity of a given session token
+#
 # @param token str
-# @throw ClientAuthenticationFailure for invalid or not found session token
+# @raise ClientAuthenticationFailure for invalid or not found session token
+#
+# @remarks It is useless to check the token size, unless urandom you don't
+#            trust in PRNG such as urandom.
+# @remarks Linear key search...
+# @remarks Consider renaming. The "validity of a session token" usually means
+#            that it is a active session token and/or that it was actually
+#            produced by the application (signed for exemple).
 def check_token(token):
     if len(token) != SESSION_TOKENSIZE:
         raise ClientAuthenticationFailure("Invalid token string")
@@ -49,8 +81,13 @@ def generate_file_path(token):
 
 
 ##
+# @brief Retrieve the token from the file system
+#
 # @param filepath str
 # @return str|None : returns the token or None if no token was found
+#
+# @remarks What is the purpose of the regex right here? There should be a way
+#            to avoid slow operations.
 def get_token_from_filepath(filepath):
     token_regex = re.compile(os.path.abspath(os.path.join(Settings.sessions.directory, Settings.sessions.file_template % '(?P<token>.*)')))
     token_search_result = token_regex.match(filepath)
@@ -59,10 +96,15 @@ def get_token_from_filepath(filepath):
     return None
 
 
-## @brief returns the session's last modification timestamp
+##
+# @brief Returns the session's last modification timestamp
+#
 # @param token str
 # @return float
-# @throw ValueError if the given token doesn't match with an existing session
+# @raise ValueError if the given token doesn't match with an existing session
+#
+# @remarks Consider renaming
+# @warning Linear search in array, again. See @ref generate_token().
 def get_session_last_modified(token):
     if token in __sessions[token]:
         return os.stat(__sessions[token]).st_mtime
@@ -70,19 +112,26 @@ def get_session_last_modified(token):
         raise ValueError("The given token %s doesn't match with an existing session")
 
 
-## @brief returns the token of a new session
-# @return str
+##
+# @brief Starts a new session and returns a new token
+#
+# @return str : the new token
 def start_session():
     session = FileSystemSession(generate_token())
     session.path = generate_file_path(session.token)
+    
     with open(session.path, 'wb') as session_file:
         pickle.dump(session, session_file)
+
     __sessions[session.token] = session.path
     logger.debug("New session created")
+
     return session.token
 
 
-## @brief destroys a session given its token
+##
+# @brief destroys a session given its token
+#
 # @param token str
 def destroy_session(token):
     check_token(token)
@@ -93,7 +142,9 @@ def destroy_session(token):
     logger.debug("Session %s unregistered" % token)
 
 
-## @brief restores a session's content
+##
+# @brief Restores a session's content
+#
 # @param token str
 # @return FileSystemSession|None
 def restore_session(token):
@@ -128,6 +179,8 @@ def save_session(token, datas):
 
 
 ## @brief session store's garbage collector
+#
+# @remarks 
 def gc():
     # Unregistered files in the session directory
     session_files_directory = os.path.abspath(Settings.sessions.directory)
@@ -164,8 +217,12 @@ def get_session_value(token, key):
     return session[key]
 
 ##
+# @brief deletes a session value
+#
 # @param token str
 # @param key str
+#
+# @todo Should we add a save_session at the end of this method?
 def del_session_value(token, key):
     session = restore_session(token)
     if key in session: