Edgewall Software
Modify

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#11293 closed defect (fixed)

AuthzPolicy will fail silently if ConfigObj is not available

Reported by: Dirk Stöcker Owned by: Ryan J Ollos
Priority: highest Milestone: 1.0.2
Component: general Version: 1.0-stable
Severity: normal Keywords: authzpolicy, permissions, exception
Cc: Jun Omae Branch:
Release Notes:

A ConfigurationError is raised if tracopt.perm.authz_policy is enabled and AuthzPolicy is in the list of active permission_policies but one of the following is true:

  • ConfigObj is not installed
  • the [authz_policy] authz_file option is missing
  • the [authz_policy] authz_file option is empty

The ConfigObj dependency for tracopt.perm.authz_policy is enforced in setup.py.

API Changes:
Internal Changes:

Description (last modified by Ryan J Ollos)

When python-configobj is not available, the AuthzPolicy fails without any notice (except a log entry). In the default config that means, that all pages are accessible and any restrictions are void. This is VERY dangerous.

Immediate Fix:

  • /usr/lib/python2.7/site-packages/tracopt/perm/authz_policy.py

    old new  
    139139
    140140    def check_permission(self, action, username, resource, perm):
    141141        if ConfigObj is None:
    142             self.log.error('configobj package not found')
    143             return None
     142            self.log.error('AuthzPolicy: configobj package not found')
     143            return False # never silently fail!
    144144
    145145        if self.authz_file and not self.authz_mtime or \
    146146                os.path.getmtime(self.get_authz_file()) > self.authz_mtime:

Also the setup.py should show clearly, that AuthzPolicy requires python-configobj to make the problem obvious.

Attachments (0)

Change History (12)

comment:1 by Dirk Stöcker, 7 years ago

Description: modified (diff)

comment:2 by Ryan J Ollos, 7 years ago

Description: modified (diff)
Keywords: authzpolicy permissions added
Milestone: 1.0.2
Owner: set to Ryan J Ollos
Status: newassigned
Summary: AuthzPolicy fails SILENTLY!AuthzPolicy will fail silently if ConfigObj is not available

Related:

  • After #11272: if the authz file can't be found or can't be parsed, a ConfigurationError is raised.
  • After #10285: If AuthzPolicy is added to [trac] permission_policies but the component is not enabled or fails to load, a ConfigurationError will be raised.

If [authz_policy] authz_file is not specified in trac.ini but AuthzPolicy is active, there is currently no error. We should probably raise a ConfigurationError in this case.

Last edited 7 years ago by Ryan J Ollos (previous) (diff)

comment:3 by Ryan J Ollos, 7 years ago

Proposed changes can be found in rjollos.git:t11293.

comment:4 by Ryan J Ollos, 7 years ago

Release Notes: modified (diff)

rjollos.git:1c203a85 results in a ConfigurationError being raised immediately when tracopt.perm.authz_policy is enabled, and before any other steps are taken to configure the installation for authz permissions. It seems better to only raise the ConfigurationError when AuthzPolicy is in the list of active permission_policies. So the behavior I propose is, allow tracopt.perm.authz_policy to be enabled but don't perform any error checks unless AuthzPolicy is in the list of active permission_policies. Once AuthzPolicy is active, we are very strict about failing with a ConfigurationError if the authz policy won't be enforced due to a configuration error or dependency not being loaded. Furthermore, after #10285, if AuthzPolicy is active but tracopt.perm.authz_policy is not enabled, a ConfigurationError will be raised, and this also extends to the case of raising a ConfigurationError if a permission policy in permission_policies is misspelled.

In rjollos.git:t11293.2 another case is covered by raising a ConfigurationError if the authz_file is empty.

comment:5 by Ryan J Ollos, 7 years ago

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed to 1.0-stable in [12037:12039]. Merged to trunk in [12040].

comment:6 by Ryan J Ollos, 7 years ago

Release Notes: modified (diff)

comment:7 by Ryan J Ollos, 7 years ago

Release Notes: modified (diff)

comment:8 by Peter Suter, 7 years ago

Keywords: exception added

comment:9 by Jun Omae, 7 years ago

Cc: Jun Omae added

I got ConfigurationError and ERROR: The authz file is empty. in trac.log when I temporarily comment out (or remove) all settings in authzpolicy file like the following. I feel the behavior is strange and confused me.

# [admin:ticket/versions]
# foobar = TRAC_ADMIN
#
# [repository:babel@*]
# foobar = !BROWSER_VIEW, !FILE_VIEW

I think we should allow authzpolicy file to be empty.

Last edited 7 years ago by Jun Omae (previous) (diff)

comment:10 by Ryan J Ollos, 7 years ago

Allowing an empty file sounds fine to me.

comment:11 by Jun Omae, 7 years ago

I've pushed changes which allows an empty authz policy in [12812].

I mentioned authzpolicy doesn't work for default repository in comment:10:ticket:10961. I think the behavior is a defect. Proposed fix can be found in jomae.git@t11293.4.

comment:12 by Jun Omae, 7 years ago

Release Notes: modified (diff)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.