[_499,_688] collapsed review changes

whitespace, documentation, comments

review changes nr. 1 2/17/25 2:07am

return early if calling into 4.2 version of func.

mv attr change

attr_changed usage replaced with older version

1 level indir ->callable

info->debug

auth modules get local loggers

more doc/msg chg

explanation/underscore

address other comments

del attr_changed

Author: d-w-moore

irods/__init__.py

@@ -30,8 +30,10 @@ def derived_auth_filename(env_filename):
     if not env_filename:
         return ""
     default_irods_authentication_file = (
-        ##->https://github.com/irods/python-irodsclient/issues/686
-        #os.path.join(os.path.dirname(env_filename), ".irodsA")
+        # TODO(#686): move into separate commit linked to this issue.
+        # This is a revision in how we determine the default value of the authentication file (.irodsA) path.
+        # Previously this was calculated as:
+        #     os.path.join(os.path.dirname(env_filename), ".irodsA")
         os.path.expanduser("~/.irods/.irodsA")
     )
     return os.environ.get(

irods/auth/__init__.py

@@ -13,10 +13,27 @@
 AUTH_PLUGIN_PACKAGE = "irods.auth"
 
 
-NoneType = type(None)
+# Python3 does not have types.NoneType
+_NoneType = type(None)
 
 
 class AuthStorage:
+    """A class that facilitates flexible means password storage.
+
+    Using an instance of this class, passwords may either be
+
+        - directly placed in a member attribute (pw), or 
+
+        - they may be written to / read from a specified file path in encoded
+          form, usually in an .irodsA file intended for iRODS client authentication.
+
+    Most typical of this class's utility is the transfer of password information from
+    the pam_password to the native authentication flow.  In this usage, whether the
+    password is stored in RAM or in the filesystem depends on whether it was read 
+    originally as a function parameter or from an authentication file, respectively,
+    when the session was created.
+    
+    """
 
     @staticmethod
     def get_env_password(filename = None):
@@ -38,12 +55,20 @@ def set_env_password(unencoded_pw, filename = None):
 
     @staticmethod
     def get_temp_pw_storage(conn):
+        """Fetch the AuthStorage instance associated with this connection object.
+        """
         return getattr(conn,'auth_storage',lambda:None)()
 
     @staticmethod
     def create_temp_pw_storage(conn):
-        """A reference to the value returned by this call should be stored for the duration of the
-           authentication exchange.
+        """Creates an AuthStorage instance to be cached and associated with this connection object.
+
+           Called multiple times for the same connection, it will return the cached instance.
+
+           The value returned by this call should be stored by the caller into an appropriately scoped
+           variable to ensure the AuthStorage instance endures for the desired lifetime -- that is,
+           for however long we wish to keep the password information around.  This is because the
+           connection object only maintains a weak reference to said instance.
         """
         store = getattr(conn,'auth_storage',None)
         if store is None:
@@ -64,7 +89,9 @@ def auth_file(self):
         return self._auth_file or self.conn.account.derived_auth_file
 
     def use_client_auth_file(self, auth_file):
-        if isinstance(auth_file, (str, NoneType)):
+        """Set to None to completely suppress use of an .irodsA auth file.
+        """
+        if isinstance(auth_file, (str, _NoneType)):
             self._auth_file = auth_file
         else:
             msg = f"Invalid object in {self.__class__}._auth_file"
@@ -132,17 +159,17 @@ def __init__(self, connection, scheme):
     def call(self, next_operation, request):
         logging.info('next operation = %r', next_operation)
         old_func = func = next_operation
-        while isinstance(func, str):
+        # One level of indirection should be sufficient to get a callable method.
+        if not callable(func):
             old_func, func = (func, getattr(self, func, None))
         func = (func or old_func)
-        if not func:
+        if not callable(func):
             raise RuntimeError("client request contains no callable 'next_operation'")
         resp = func(request)
         logging.info('resp = %r',resp)
         return resp
 
     def authenticate_client(self, next_operation = "auth_client_start", initial_request = {}):
-
         to_send = initial_request.copy()
         to_send["scheme"] = self.scheme
 

irods/auth/native.py

@@ -12,37 +12,42 @@
 
 
 def login(conn, **extra_opt):
+    """When the Python iRODS client loads this(or any plugin) for authenticating a connection object,
+    login is the hook function that gets called.
+    """
     opt = {'user_name': conn.account.proxy_user,
            'zone_name': conn.account.proxy_zone}
     opt.update(extra_opt)
-    authenticate_native(conn, req = opt)
+    _authenticate_native(conn, req = opt)
 
 
 _scheme = 'native'
 
 
-def authenticate_native(conn, req):
+_logger = logging.getLogger(__name__)
 
-    logging.info('----------- %s (begin)', _scheme)
 
-    native_ClientAuthState(
+def _authenticate_native(conn, req):
+    """The implementation for the client side of a native scheme authentication flow.
+       It is called by login(), the external hook.
+       Other client auth plugins should at least roughly follow this pattern.
+    """
+    _logger.debug('----------- %s (begin)', _scheme)
+
+    _native_ClientAuthState(
         conn,
         scheme = _scheme
     ).authenticate_client(
         # initial_request is called context (or ctx for short) in iRODS core library code.
         initial_request = req
     )
 
-    logging.info('----------- %s (end)', _scheme)
-
+    _logger.debug('----------- %s (end)', _scheme)
 
-class native_ClientAuthState(authentication_base):
 
-    def auth_client_start(self, request):
-        resp = request.copy()
-        # user_name and zone_name keys injected by authenticate_client() method
-        resp[__NEXT_OPERATION__] = self.AUTH_CLIENT_AUTH_REQUEST # native_auth_client_request
-        return resp
+class _native_ClientAuthState(authentication_base):
+    """A class containing the specific methods needed to implement a native scheme authentication flow.
+    """
 
     # Client defines. These strings should match instance method names within the class namespace.
     AUTH_AGENT_START = 'native_auth_agent_start'
@@ -54,6 +59,12 @@ def auth_client_start(self, request):
     AUTH_AGENT_AUTH_REQUEST = "auth_agent_auth_request"
     AUTH_AGENT_AUTH_RESPONSE = "auth_agent_auth_response"
 
+    def auth_client_start(self, request):
+        resp = request.copy()
+        # user_name and zone_name keys injected by authenticate_client() method
+        resp[__NEXT_OPERATION__] = self.AUTH_CLIENT_AUTH_REQUEST # native_auth_client_request
+        return resp
+
     def native_auth_client_request(self, request):
         server_req = request.copy()
         server_req[__NEXT_OPERATION__] = self.AUTH_AGENT_AUTH_REQUEST

irods/auth/pam_password.py

@@ -23,8 +23,11 @@ def login(conn, **extra_opt):
 _scheme = 'pam_password'
 
 
+_logger = logging.getLogger(__name__)
+
+
 def authenticate_pam_password(conn, req):
-    logging.info('----------- %s (begin)', _scheme)
+    _logger.debug('----------- %s (begin)', _scheme)
 
     # By design, we persist this "depot" object over the whole of the authentication
     # exchange with the iRODS server as a means of sending password information to the
@@ -33,14 +36,14 @@ def authenticate_pam_password(conn, req):
     # are authenticating without the help of iCommand-type client env/auth files.
     _ = AuthStorage.create_temp_pw_storage(conn)
 
-    pam_password_ClientAuthState(
+    _pam_password_ClientAuthState(
         conn,
         scheme = _scheme
     ).authenticate_client(
         initial_request = req
     )
 
-    logging.info('----------- %s (end)', _scheme)
+    _logger.debug('----------- %s (end)', _scheme)
 
 
 def get_pam_password_from_stdin(file_like_object = None):
@@ -61,30 +64,39 @@ def get_pam_password_from_stdin(file_like_object = None):
 AUTH_PASSWORD_KEY = "a_pw"
 
 
-class pam_password_ClientAuthState(authentication_base):
+class _pam_password_ClientAuthState(authentication_base):
+
+    # Client define
+    AUTH_CLIENT_AUTH_REQUEST = "pam_password_auth_client_request"
+
+    # Server define
+    AUTH_AGENT_AUTH_REQUEST = "auth_agent_auth_request"
 
     def __init__(self,*_,check_ssl=True,**_kw):
         super().__init__(*_,**_kw)
         self.check_ssl = check_ssl
-        self._l = None
+        self._list_for_request_result_return = None
 
     def auth_client_start(self, request):
 
-        self._l = request.pop(CLIENT_GET_REQUEST_RESULT, False)
+        # This list reference is popped and cached for the purpose of returning the request_result value
+        # to the caller upon request.
+        self._list_for_request_result_return = request.pop(CLIENT_GET_REQUEST_RESULT, False)
 
         if self.check_ssl:
             if not isinstance(self.conn.socket, ssl.SSLSocket):
-                msg = 'Need to be connected via SSL.'
+                msg = "pam_password auth scheme requires secure communications (TLS/SSL) with the server."
                 raise RuntimeError(msg)
 
         resp = request.copy()
 
-        obj = resp.pop(FORCE_PASSWORD_PROMPT, None)
+        password_input_obj = resp.pop(FORCE_PASSWORD_PROMPT, None)
 
-        if obj:
-            obj = None if isinstance(obj,(int,bool)) else obj
-            # Like with the C++ plugin, we offer the user a chance 
-            resp[AUTH_PASSWORD_KEY] = get_pam_password_from_stdin(file_like_object = obj)
+        if password_input_obj:
+            if isinstance(password_input_obj,(int,bool)):
+                password_input_obj = None
+            # Like with the C++ plugin, we offer the user a chance to enter a password.
+            resp[AUTH_PASSWORD_KEY] = get_pam_password_from_stdin(file_like_object = password_input_obj)
         else:
             # Password from .irodsA in environment.
             if self.conn.account._auth_file:
@@ -97,12 +109,6 @@ def auth_client_start(self, request):
         resp[__NEXT_OPERATION__] = self.AUTH_CLIENT_AUTH_REQUEST
         return resp
 
-    # Client define
-    AUTH_CLIENT_AUTH_REQUEST = "pam_password_auth_client_request"
-
-    # Server define
-    AUTH_AGENT_AUTH_REQUEST = "auth_agent_auth_request"
-
     def pam_password_auth_client_request(self, request):
         server_req = request.copy()
         server_req[__NEXT_OPERATION__] = self.AUTH_AGENT_AUTH_REQUEST
@@ -113,14 +119,15 @@ def pam_password_auth_client_request(self, request):
         depot = AuthStorage.get_temp_pw_storage(self.conn)
         if depot:
             if resp.get(STORE_PASSWORD_IN_MEMORY, None):
+                # Prevent use of an .irodsA to store an encoded password.
                 depot.use_client_auth_file(None)
             depot.store_pw(resp["request_result"])
         else:
             msg = "auth storage object was either not set, or allowed to expire prematurely."
             raise RuntimeError(msg)
 
-        if isinstance(self._l,list):
-            self._l[:] = (resp["request_result"],)
+        if isinstance(self._list_for_request_result_return,list):
+            self._list_for_request_result_return[:] = (resp["request_result"],)
 
         resp[__NEXT_OPERATION__] = self.perform_native_auth
         return resp

irods/client_init.py

@@ -65,15 +65,15 @@ def write_pam_irodsA_file(password, overwrite=True, ttl = '', **kw):
     import io
     ses = kw.pop('_session', None) or h.make_session(**kw)
     if ses._server_version(iRODSSession.RAW_SERVER_VERSION) < (4,3):
-        write_pam_credentials_to_secrets_file(
+        return write_pam_credentials_to_secrets_file(
             password,
             overwrite = overwrite,
             ttl = ttl,
             _session = ses)
 
     auth_file = ses.pool.account.derived_auth_file
     if not auth_file:
-        msg = "Need an active client environment"
+        msg = "Auth file could not be written because no iRODS client environment was found."
         raise RuntimeError(msg)
     if ttl:
         ses.set_auth_option_for_scheme('pam_password', irods.auth.pam_password.AUTH_TTL_KEY, ttl)

irods/helpers/__init__.py

@@ -38,11 +38,20 @@ def xml_mode(s):
         ET(None)
 
 
+class _unlikely_value:
+    pass
+
 @contextlib.contextmanager
-def attr_changed(obj, attrname, value):
-   old = getattr(obj, attrname, None)
-   try:
-       setattr(obj, attrname, value)
-       yield
-   finally:
-       setattr(obj, attrname, old)
+def temporarily_assign_attribute(
+    target, attr, value, not_set_indicator=_unlikely_value()
+):
+    save = not_set_indicator
+    try:
+        save = getattr(target, attr, not_set_indicator)
+        setattr(target, attr, value)
+        yield
+    finally:
+        if save != not_set_indicator:
+            setattr(target, attr, save)
+        else:
+            delattr(target, attr)

irods/message/__init__.py

@@ -772,6 +772,7 @@ class MetadataRequest(Message):
     def __init__(self, *args, **metadata_opts):
         super(MetadataRequest, self).__init__()
 
+        # Python3 does not have types.NoneType
         NoneType = type(None)
 
         def field_name(i):

irods/pool.py

@@ -7,7 +7,6 @@
 
 from irods import DEFAULT_CONNECTION_TIMEOUT
 from irods.connection import Connection
-#import irods.helpers
 from irods.ticket import Ticket
 
 logger = logging.getLogger(__name__)
@@ -71,7 +70,7 @@ def __init__(
     @contextlib.contextmanager
     def no_auto_authenticate(self):
         import irods.helpers
-        with irods.helpers.attr_changed(self, '_need_auth', False):
+        with irods.helpers.temporarily_assign_attribute(self, '_need_auth', False):
             yield self
 
     def set_session_ref(self, session):

irods/session.py

@@ -9,7 +9,7 @@
 import os
 import threading
 import weakref
-import irods.auth 
+import irods.auth
 from irods.query import Query
 from irods.genquery2 import GenQuery2
 from irods.pool import Pool
@@ -71,16 +71,6 @@ class NonAnonymousLoginWithoutPassword(RuntimeError):
     pass
 
 
-@contextlib.contextmanager
-def attr_changed(obj, attrname, value):
-   old = getattr(obj, attrname, None)
-   try:
-       setattr(obj, attrname, value)
-       yield
-   finally:
-       setattr(obj, attrname, old)
-
-
 class iRODSSession:
 
     def library_features(self):

irods/test/connection_test.py

@@ -12,11 +12,8 @@
 )
 import irods.session
 import irods.test.helpers as helpers
-from irods.test.helpers import (
-    server_side_sleep,
-    temporarily_assign_attribute as temp_setter,
-)
-
+from irods.test.helpers import server_side_sleep
+from irods.helpers import temporarily_assign_attribute as temp_setter
 
 class TestConnections(unittest.TestCase):