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

[Dave Abrahams <dave@…>]

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.

[Dave Abrahams <dave@…>]

Local Trac env configuration

[Dave Abrahams <dave@…>]

"Global" (inherited) config file

[Dave Abrahams <dave@…>]

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.

[Emmanuel Blot]

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.

[Emmanuel Blot]

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.

[Emmanuel Blot]

Proposal for raising errors on missing files

[Emmanuel Blot]

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.

[Christian Boos]

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.

[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.

[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.

[anonymous]

Where does one update the api documentation?

[Emmanuel Blot]

#6218 marked as a duplicate.

[Christian Boos]

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.

[Christian Boos]

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.

[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).

[Christian Boos]

Point taken. A real error page is actually needed.

[osimons]

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

[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.

• ​http://trac-hacks.org/ticket/8575#comment:19

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

@@ -255 +255 @@
     def parse_if_needed(self, force=False):
         if not self.filename or not os.path.isfile(self.filename):
             return False
+        try:
+            with open(self.filename) as f:
+                pass
+        except Exception,e :
+            raise TracError(_('Unable to open config file %s')%self.filename)
 
+
         changed = False
         modtime = os.path.getmtime(self.filename)
         if force or modtime > self._lastmtime:

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

[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.