Edgewall Software
Modify

Opened 7 years ago

Last modified 3 years ago

#5535 new defect

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

Reported by: Dave Abrahams <dave@…> Owned by: cboos
Priority: normal Milestone: next-major-releases
Component: general Version: devel
Severity: major Keywords: config
Cc: osimons, leho@…
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@…> 7 years ago.
Local Trac env configuration
trac.2.ini (3.7 KB) - added by Dave Abrahams <dave@…> 7 years ago.
"Global" (inherited) config file
5535.patch (4.1 KB) - added by eblot 7 years ago.
Proposal for raising errors on missing files

Download all attachments as: .zip

Change History (21)

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

Local Trac env configuration

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

"Global" (inherited) config file

comment:1 follow-up: Changed 7 years ago by Dave Abrahams <dave@…>

  • Resolution set to invalid
  • Status changed from new to closed

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 7 years ago by eblot

  • Milestone 0.11 deleted
  • Priority changed from normal to low
  • Resolution invalid deleted
  • Severity changed from critical to minor
  • Status changed from closed to reopened

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 7 years ago by eblot

  • Severity changed from minor to major

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 7 years ago by eblot

Proposal for raising errors on missing files

comment:4 follow-ups: Changed 7 years ago by eblot

  • 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 7 years ago by cboos

  • Milestone set to 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 7 years ago by eblot

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 7 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 7 years ago by anonymous

Where does one update the api documentation?

comment:9 Changed 7 years ago by eblot

#6218 marked as a duplicate.

comment:10 follow-up: Changed 7 years ago by cboos

  • Keywords review removed
  • Owner changed from jonas to eblot
  • Status changed from reopened to new

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 7 years ago by cboos

  • Milestone changed from 0.11 to 0.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 7 years ago by nkantrowitz

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 7 years ago by cboos

Point taken. A real error page is actually needed.

comment:14 Changed 6 years ago by osimons

  • Cc osimons added
  • Summary changed from [inherit] not working for me to [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 4 years ago by cboos

  • Keywords config added
  • Milestone changed from next-minor-0.12.x to next-major-0.1X
  • Owner changed from eblot to cboos
  • Priority changed from low to normal

comment:16 follow-up: Changed 4 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 4 years ago by cboos

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 3 years ago by lkraav <leho@…>

  • Cc leho@… added

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The owner will remain cboos.
as The resolution will be set. Next status will be 'closed'.
The owner will be changed from cboos 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.