Edgewall Software
Modify

Opened 9 years ago

Last modified 14 months ago

#5535 new defect

[inherit] not working for me (error on missing config)

Reported by: Dave Abrahams <dave@…> Owned by:
Priority: normal Milestone: next-major-releases
Component: general Version: devel
Severity: major Keywords: config
Cc: osimons, leho@…, Ryan J Ollos
Release Notes:
API Changes:

Description

plugins in the inherited plugin directory don't show up in my administration, etc. They work fine if I move them into the local trac env's plugins directory.

local and global confs attached.

Attachments (3)

trac.ini (1.2 KB) - added by Dave Abrahams <dave@…> 9 years ago.
Local Trac env configuration
trac.2.ini (3.7 KB) - added by Dave Abrahams <dave@…> 9 years ago.
"Global" (inherited) config file
5535.patch (4.1 KB) - added by Emmanuel Blot 9 years ago.
Proposal for raising errors on missing files

Download all attachments as: .zip

Change History (23)

Changed 9 years ago by Dave Abrahams <dave@…>

Attachment: trac.ini added

Local Trac env configuration

Changed 9 years ago by Dave Abrahams <dave@…>

Attachment: trac.2.ini added

"Global" (inherited) config file

comment:1 Changed 9 years ago by Dave Abrahams <dave@…>

Resolution: invalid
Status: newclosed

I should always assume I'm overlooking something and go to bed when I find myself thinking something is broken after 1AM.

My paths were pointing at a nonexistent global conf file. It would be good to warn about that in the log at least.

comment:2 in reply to:  1 Changed 9 years ago by Emmanuel Blot

Milestone: 0.11
Priority: normallow
Resolution: invalid
Severity: criticalminor
Status: closedreopened

Replying to Dave Abrahams <dave@boost-consulting.com>:

My paths were pointing at a nonexistent global conf file. It would be good to warn about that in the log at least.

Agreed. Emit a warning when a path is not found.

comment:3 Changed 9 years ago by Emmanuel Blot

Severity: minormajor

It is not possible to emit a warning to the log file, as if a configuration file is missing the log file is not known by the time the error is detected.

However, a missing trac.ini file can be considered as a fatal error, as it may contain values that override default values. Leaving default values where the administrator expects custom values may be a security issue.

The environment may also be put at risks if the plugins_dir is found missing: some plugins might for example add extra access conditions to the project, for example.

Increasing the severity value as this issue might lead to a security breach.

Changed 9 years ago by Emmanuel Blot

Attachment: 5535.patch added

Proposal for raising errors on missing files

comment:4 Changed 9 years ago by Emmanuel Blot

Keywords: review added

I've attached a patch to implement this feature:

  • Trac raises TracError on missing files (either a missing trac.ini file or a missing plugins dir)
  • rework the way the configuration file are loaded at start up. I'm not sure about this point, as the current implementation does not make the difference between the creation of a new environment (trac.ini file does not exist yet) and loading an existing environment. The proposed patch extends the existing behaviour (Configuration instance w/ no file name) and defers file name definition till the save() call.

comment:5 in reply to:  4 Changed 9 years ago by Christian Boos

Milestone: 0.11

Replying to eblot:

I've attached a patch to implement this feature:

… The proposed patch extends the existing behaviour (Configuration instance w/ no file name) and defers file name definition till the save() call.

I haven't tested it, but have read it carefully and the changes seem OK. An API documentation update is in order however, as for creating a new config file, the Configuration constructor must now be given None for the filename.

comment:6 Changed 9 years ago by Emmanuel Blot

Ok.

I just need to perform some other tests as the test suite runs ok and I've been able to create a new environment but I've discovered after the patch has been attached to this ticket that my trac.ini test file looked like a pristine one. I'm not sure if it has been reset during the debugging of the patch or if it's a bug within the patch.

I'll update the API doc.

comment:7 in reply to:  4 Changed 9 years ago by osimons <simon-code@…>

Replying to eblot:

  • Trac raises TracError on missing files (either a missing trac.ini file or a missing plugins dir)

  • rework the way the configuration file are loaded at start up. I'm not sure about this point, as the current implementation does not make the difference between the creation of a new environment (trac.ini file does not exist yet) and loading an existing environment. The proposed patch extends the existing behaviour (Configuration instance w/ no file name) and defers file name definition till the save() call.

The problem regarding using [inherit] on environment creation is well documented (with patch) in ticket #5651, and has some further consequences such as passing config file via trac-admin and such (not done yet).

The discussion about warnings and rules for handling with missing files is different matter though, and it might simplify things if the issues where kept separate? The solution to this should likely not care if it is on 'create' or 'use' as it happens when config is loaded or reparsed - same entry and behavior for both use cases, I think.

comment:8 Changed 9 years ago by anonymous

Where does one update the api documentation?

comment:9 Changed 9 years ago by Emmanuel Blot

#6218 marked as a duplicate.

comment:10 Changed 9 years ago by Christian Boos

Keywords: review removed
Owner: changed from Jonas Borgström to Emmanuel Blot
Status: reopenednew

I updated the patch for current trunk and it worksforme.

But the errors are a bit too "violent" I think, as you'll get only a raw text message without any further hint about what to do… Not a problem while you're installing your Trac for the first time, but imagine what will happen when e.g. an admin removes an .ini file (maybe that one was empty?) which was inherited by some Trac environment: now that environment will stop working and the users will possibly have no clue about who they have to contact for fixing the problem.

Ideally, those non critical errors (missing/unreadable [inherited] files) should be presented as a normal Trac error.

I reassign the ticket to manu as he proposed the initial patch. I think this can eventually wait for 0.11.x until the right solution is found.

comment:11 Changed 9 years ago by Christian Boos

Milestone: 0.110.11.1

Also I noticed that with that approach, we'll complain loudly for an invalid [inherited] plugins_dir folder, but we'll silently ignore a missing <env>/plugins folder!

I think a somehow different approach is needed: have an extra env.verify_config method which will do a few integrity checks and return a list of problems found, and have that called once and reported as warnings.

comment:12 in reply to:  10 Changed 9 years ago by Noah Kantrowitz

Replying to cboos:

Ideally, those non critical errors (missing/unreadable [inherited] files) should be presented as a normal Trac error.

This is very much a critical error. What if the now-missing file contained a permission policy setting that hid things from most users (think PrivateTickets)? Even if it is a nicer error page, we need to be very careful to not disclose anything about the env that could otherwise have been hidden (which is almost everything).

comment:13 Changed 9 years ago by Christian Boos

Point taken. A real error page is actually needed.

comment:14 Changed 7 years ago by osimons

Cc: osimons added
Summary: [inherit] not working for me[inherit] not working for me (error on missing config)

#7204 closed as a duplicate + minor addition to summary to focus the actual issue.

comment:15 Changed 5 years ago by Christian Boos

Keywords: config added
Milestone: next-minor-0.12.xnext-major-0.1X
Owner: changed from Emmanuel Blot to Christian Boos
Priority: lownormal

comment:16 Changed 5 years ago by bobbysmith007@…

Just wanted to note that I (and my plugin's users) ran afoul of this, and a better error message would have been greatly appreciated.

I would like to suggest a better error message to alert users that the specified config file is unreadable. I'm not sure that my patch below is meeting all trac coding criteria, but something similar to below would have allowed me to identify the actual cause of the error much more quickly.

  • trac/config.py

     
    255255    def parse_if_needed(self, force=False):
    256256        if not self.filename or not os.path.isfile(self.filename):
    257257            return False
     258        try:
     259            with open(self.filename) as f:
     260                pass
     261        except Exception,e :
     262            raise TracError(_('Unable to open config file %s')%self.filename)
    258263
     264
    259265        changed = False
    260266        modtime = os.path.getmtime(self.filename)
    261267        if force or modtime > self._lastmtime:

Thanks so much for trac, and for all your efforts on our behalf, Russ

comment:17 in reply to:  16 Changed 5 years ago by Christian Boos

Replying to bobbysmith007@…:

I would like to suggest a better error message to alert users that the specified config file is unreadable.

Thanks! Actually this specific part of the problem is tracked in #10044 and we noted there that the work in progress on #7378 already fixes this. So the situation is going to improve in 0.13.

That ticket was more intended to be general, also checking for missing plugins_dir folder, etc., so I think we can leave it open for now.

comment:18 Changed 5 years ago by lkraav <leho@…>

Cc: leho@… added

comment:19 Changed 16 months ago by Ryan J Ollos

Cc: Ryan J Ollos added

comment:20 Changed 14 months ago by Ryan J Ollos

Owner: Christian Boos deleted

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The ticket will remain with no owner.
The ticket will be disowned. Next status will be 'new'.
as The resolution will be set. Next status will be 'closed'.
The owner will be changed from (none) to anonymous. Next status will be 'assigned'.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.