LP #281156: 'AccessControl.SecurityInfo.secureModule' dropped ModuleS…

…ecurity

for failed imports, obscuring later attempts to import the same broken module.

'AccessControl.ZopeGuards.guarded_import' mapped some Unauthorized exceptions
onto ImportErrors:  don't do that!  Also, removed mutable defaults from
argument list, improved tests.

doc/CHANGES.txt

@@ -8,6 +8,14 @@ Zope Changes
 
    Bugs fixed
 
+      - 'AccessControl.ZopeGuards.guarded_import' mapped some Unauthorized
+        exceptions onto ImportErrors:  don't do that!  Also, removed
+        mutable defaults from argument list, improved tests.
+
+      - LP #281156:  'AccessControl.SecurityInfo.secureModule' dropped
+        ModuleSecurity for failed imports, obscuring later attempts to
+        import the same broken module.
+
       - LP #142667:  Updated to ZODB-3.6.4 to fix problem with product
         auto-refresh.
 

lib/python/AccessControl/SecurityInfo.py

@@ -207,10 +207,10 @@ def secureModule(mname, *imp):
     modsec = _moduleSecurity.get(mname, None)
     if modsec is None:
         return
-    del _moduleSecurity[mname]
 
     if imp:
         __import__(mname, *imp)
+    del _moduleSecurity[mname]
     module = sys.modules[mname]
     modsec.apply(module.__dict__)
     _appliedModuleSecurity[mname] = modsec

lib/python/AccessControl/ZopeGuards.py

@@ -259,31 +259,32 @@ def guarded_map(f, *seqs):
     return map(f, *safe_seqs)
 safe_builtins['map'] = guarded_map
 
-def guarded_import(mname, globals={}, locals={}, fromlist=None):
+def guarded_import(mname, globals=None, locals=None, fromlist=None):
+    if fromlist is None:
+        fromlist = ()
+    if '*' in fromlist:
+        raise Unauthorized, "'from %s import *' is not allowed"
+    if globals is None:
+        globals = {}
+    if locals is None:
+        locals = {}
     mnameparts = mname.split('.')
     firstmname = mnameparts[0]
     validate = getSecurityManager().validate
     module = load_module(None, None, mnameparts, validate, globals, locals)
-    if module is not None:
-        if fromlist is None:
-            fromlist = ()
-        try:
-            for name in fromlist:
-                if name == '*':
-                    raise ImportError, ('"from %s import *" is not allowed'
-                                        % mname)
-                v = getattr(module, name, None)
-                if v is None:
-                    v = load_module(module, mname, [name], validate,
-                                    globals, locals)
-                if not validate(module, module, name, v):
-                    raise Unauthorized
-            else:
-                return __import__(mname, globals, locals, fromlist)
-        except Unauthorized, why:
-            raise ImportError, ('import of "%s" from "%s" is unauthorized. %s'
-                                % (name, mname, why))
-    raise ImportError, 'import of "%s" is unauthorized' % mname
+    if module is None:
+        raise Unauthorized, "import of '%s' is unauthorized" % mname
+    if fromlist is None:
+        fromlist = ()
+    for name in fromlist:
+        v = getattr(module, name, None)
+        if v is None:
+            v = load_module(module, mname, [name], validate,
+                            globals, locals)
+        if not validate(module, module, name, v):
+            raise Unauthorized
+    else:
+        return __import__(mname, globals, locals, fromlist)
 safe_builtins['__import__'] = guarded_import
 
 class GuardedListType:

lib/python/AccessControl/tests/testModuleSecurity.py

@@ -1,6 +1,6 @@
 ##############################################################################
 #
-# Copyright (c) 2002 Zope Corporation and Contributors. All Rights Reserved.
+# Copyright (c) 2008 Zope Corporation and Contributors. All Rights Reserved.
 #
 # This software is subject to the provisions of the Zope Public License,
 # Version 2.1 (ZPL).  A copy of the ZPL should accompany this distribution.
@@ -10,51 +10,44 @@
 # FOR A PARTICULAR PURPOSE
 #
 ##############################################################################
-"""Module Import Tests
-"""
+import unittest
 
-__rcs_id__='$Id$'
-__version__='$Revision: 1.4 $'[11:-2]
+class ModuleSecurityTests(unittest.TestCase):
 
-import os, sys, unittest
+    def setUp(self):
+        from AccessControl import ModuleSecurityInfo as MSI
+        MSI('AccessControl.tests.mixed_module').declarePublic('pub')
+        MSI('AccessControl.tests.public_module').declarePublic('pub')
+        MSI('AccessControl.tests.public_module.submodule').declarePublic('pub')
 
-import Testing
-import ZODB
-from AccessControl import User
-from AccessControl import Unauthorized, ModuleSecurityInfo
-from AccessControl.ZopeGuards import guarded_import
-
-ModuleSecurityInfo('AccessControl.tests.mixed_module').declarePublic('pub')
-
-ModuleSecurityInfo('AccessControl.tests.public_module').declarePublic('pub')
-ModuleSecurityInfo('AccessControl.tests.public_module.submodule'
-                   ).declarePublic('pub')
-
-class SecurityTests(unittest.TestCase):
+    def tearDown(self):
+        import sys
+        for module in ('AccessControl.tests.public_module',
+                       'AccessControl.tests.public_module.submodule',
+                       'AccessControl.tests.mixed_module',
+                       'AccessControl.tests.mixed_module.submodule',
+                       'AccessControl.tests.private_module',
+                       'AccessControl.tests.private_module.submodule',
+                      ):
+            if module in sys.modules:
+                del sys.modules[module]
 
     def assertUnauth(self, module, fromlist):
-        try:
-            guarded_import(module, fromlist=fromlist)
-        except (Unauthorized, ImportError):
-            # Passed the test.
-            pass
-        else:
-            assert 0, ('Did not protect module instance %s, %s' %
-                       (`module`, `fromlist`))
+        from zExceptions import Unauthorized
+        from AccessControl.ZopeGuards import guarded_import
+        self.assertRaises(Unauthorized,
+                          guarded_import, module, fromlist=fromlist)
 
     def assertAuth(self, module, fromlist):
-        try:
-            guarded_import(module, fromlist=fromlist)
-        except (Unauthorized, ImportError):
-            assert 0, ('Did not expose module instance %s, %s' %
-                       (`module`, `fromlist`))
+        from AccessControl.ZopeGuards import guarded_import
+        guarded_import(module, fromlist=fromlist)
 
     def testPrivateModule(self):
-        for name in '', '.submodule':
-            for fromlist in (), ('priv',):
-                self.assertUnauth(
-                    'AccessControl.tests.private_module%s' % name,
-                    fromlist)
+        self.assertUnauth('AccessControl.tests.private_module', ())
+        self.assertUnauth('AccessControl.tests.private_module', ('priv',))
+        self.assertUnauth('AccessControl.tests.private_module.submodule', ())
+        self.assertUnauth('AccessControl.tests.private_module.submodule',
+                          ('priv',))
 
     def testMixedModule(self):
         self.assertAuth('AccessControl.tests.mixed_module', ())
@@ -63,19 +56,25 @@ def testMixedModule(self):
         self.assertUnauth('AccessControl.tests.mixed_module.submodule', ())
 
     def testPublicModule(self):
-        for name in '', '.submodule':
-            for fromlist in (), ('pub',):
-                self.assertAuth(
-                    'AccessControl.tests.public_module%s' % name,
-                    fromlist)
+        self.assertAuth('AccessControl.tests.public_module', ())
+        self.assertAuth('AccessControl.tests.public_module', ('pub',))
+        self.assertAuth('AccessControl.tests.public_module.submodule', ())
+        self.assertAuth('AccessControl.tests.public_module.submodule',
+                        ('pub',))
 
-def test_suite():
-    suite = unittest.TestSuite()
-    suite.addTest( unittest.makeSuite( SecurityTests ) )
-    return suite
+    def test_public_module_asterisk_not_allowed(self):
+        self.assertUnauth('AccessControl.tests.public_module', ('*',))
 
-def main():
-    unittest.TextTestRunner().run(test_suite())
+    def test_failed_import_keeps_MSI(self):
+        # LP #281156
+        from AccessControl import ModuleSecurityInfo as MSI
+        from AccessControl.SecurityInfo import _moduleSecurity as MS
+        from AccessControl.ZopeGuards import guarded_import
+        MSI('AccessControl.tests.nonesuch').declarePublic('pub')
+        self.failUnless('AccessControl.tests.nonesuch' in MS)
+        self.assertRaises(ImportError,
+                      guarded_import, 'AccessControl.tests.nonesuch', ())
+        self.failUnless('AccessControl.tests.nonesuch' in MS)
 
-if __name__ == '__main__':
-    main()
+def test_suite():
+    return unittest.makeSuite(ModuleSecurityTests)

lib/python/Products/PythonScripts/tests/testPythonScript.py

@@ -236,8 +236,9 @@ def testSubversiveExcept(self):
         self.assertPSRaises(SyntaxError, path='subversive_except')
 
     def testBadImports(self):
-        self.assertPSRaises(ImportError, body="from string import *")
-        self.assertPSRaises(ImportError, body="import mmap")
+        from zExceptions import Unauthorized
+        self.assertPSRaises(Unauthorized, body="from string import *")
+        self.assertPSRaises(Unauthorized, body="import mmap")
 
     def testAttributeAssignment(self):
         # It's illegal to assign to attributes of anything that