[patch] HttpAuthStore authentication enhancement

[Dennis McRitchie <dmcr@…>]

I have been working on getting HttpAuthStore authentication working with Trac 0.12.1 and Apache under WSGI. The main difficulty was that the file identified by the authentication_url directive had to exist and be successfully fetched. And fetching static files under WSGI (but also under mod_python) can be difficult to configure.

So for example, it is possible to put a file called 'authFile' in a project's htdocs directory, but then one has to know that the trac.ini directive would need to be set to:

authentication_url = https://server.domain.com/trac/chrome/site/authFile

and the Apache <Location> directive would need to be configured as:

<Location /trac/chrome/site/authFile>

This is not intuitive. Also, the current requirement for being able to successfully fetch 'authFile' is not really necessary, since it does not guarantee true authentication. That is, if the <Location> directive is incorrectly configured, it will be possible to 'successfully' authenticate with an invalid password, as long as authentication_url points to an existing file.

And it is somewhat inconvenient that authentication_url values must be absolute URLs.

So in the attached patch to http.py, I test for a 404 error code, and allow this to signal a successful authentication attempt, which it will be as long as the <Location> directive is correctly set. This then allows one to more closely mimic the Trac's native login support, where the server-relative URL is '/login'. So, in combination with allowing server-relative URLs (as well as true relative URLs, just to be consistent with the [header_logo]'s 'src' directive), one can now simply set authentication_url to '/authFile' and similarly set the <Location> directive to '/authFile'.

In the case of supporting multiple Trac projects, these could become:

authentication_utl = /project1/authFile

and

<LocationMatch "^/[^/]+/authFile$">

One can of course use the Apache 'Alias' directive, or use the 'SetHandler' directive to reset the Apache content handler to 'None', but this seems needlessly complicated.

Ideally, one would have a handler for the resource identified by the authentication_url directive, but I don't know how to do that; and the only drawback to this approach, I believe, is that there will be HTTPNotFound warnings in the log files if the user points to a non-existent file.

Let me know what you think.

Dennis

[Dennis McRitchie <dmcr@…>]

Patch to http.py to enhance HttpAuthStore authentication

[Dennis McRitchie <dmcr@…>]

I see now that I created the patch against an older version of http.py. If you like the changes, I'll be happy to revise my patch since the 'patch' utility won't be able to handle some of the changes as is.

Dennis

[Steffen Hoffmann]

Revision is unnecessary, if not for reworking any code introducing incompatibility with Python down to 2.4, because I know this is still required by some of our users. Despite your patch suggests it, I didn't spot code specific to Python2.6, so this should be fine.

Are you prepared to test trunk, because this would be the place I'll put it in for acct_mgr-0.4 (yet unreleased)? I'll rely on your judgment, if/when this is/will be ready to release. Changes seem relatively straight forward. Just tell me, should we retain all the debug logging as essential, or is something just for development?

[Dennis McRitchie <dmcr@…>]

I was developing on 2.6, but I agree that I think my changes should work on 2.4. But I mentioned revising my patch because there appeared to be 2 lines that you had already changed that I also changed:

mgr.add_password(None, self.auth_url, user, password)

and

HTTPDigestAuthHandler(mgr)).open(self.auth_url)

So when you apply the patch you'll need to be careful to preserve both sets of changes.

I can test trunk once you tell me it's there. I have a test environment for that purpose.

Yes, I would suggest leaving in the debug log messages, because it can be very frustrating trying to figure out why authentication is failing when it should work, or working when it should fail.

Dennis

[Steffen Hoffmann]

Thanks for the pointers and your statement on debug messages in your patch.

I'm looking forward to your test results once I've pushed the changes out. Thank you for taking care, contributing ideas and your precious time to plugin development here.

[Steffen Hoffmann]

(In [11045]) AccountManagerPlugin: Allow relative URL for HttpAuthStore configuration, refs #8925 and #9618.

Add more sophisticated logic to deal with various authentication_url values other than full absolute URLs as suggested by Dennis McRitchie. Minor differences to the original patch include enforced 79 char/line limit including use of multi-line strings, modified comments and the rename of user -> username according to previous changes, starting with [10288].

[Dennis McRitchie <dmcr@…>]

Installed the trunk version (r11045), and it is working fine as far as I have tested it. I've carefully tested my changes to HttpAuthStore, and those are ok; but not sure what else has changed that might need some deeper testing.

Had some upgrade problems:

1. For some reason it required me to reverify my email address. This might imply some database change that does not recognize previous verifications. Have there been changes to this feature? This could be quite annoying if every user needs to reverify.
2. Several keywords have changed their name, making it seem as though they have been disabled by the upgrade:
   1. password_file -> htpasswd_file
   2. acct_mgr.admin.accountmanageradminpage -> acct_mgr.admin.accountmanageradminpages
   3. acct_mgr.notification.accountchangenotificationadminpanel -> acct_mgr.notification.accountchangenotificationadminpage

Perhaps there are other name changes? This is not a big deal for a single project, but when you manage dozens as I do, spotting these changes and fixing them can become a headache. It might be best to accept either name when a name has changed, and treat the old name as 'deprecated' in the documentation.

Note that I did not do a 'trac-admin upgrade'. Don't know if I was supposed to. I did not get a message on my trac project test website that I needed to. But it would be nice to have a easy way to transition to your new acct_mgr version, because many upgraders will be existing trac administrators with existing projects and users.

Let me know if there's anything else you'd specifically like me to test.

Thanks,

Dennis

[Steffen Hoffmann]

Thank you for the report. Great that it works for you, so more users will profit from greater flexibility soon.

Regarding the name changes:
I know there's a bunch of non-backwards-compatible changes for the upcoming release. Some are strictly required, like the password_file -> htpasswd_file (note: now there is htdigest_file too), because this was the only way to allow for concurrent, independent configuration of HtDigestStore, HtPasswdStore.

Others seem more cosmetic, like the other two you mentioned above. They help to align to common naming conventions for similar components. Fellow developers could understand the structure of the plugin more easily. I know how hard this is, because I invested evening after evening to read code on my own, before I was able to fix some of the pending issues.

Look at the changelog​. A lot of work, nothing done too lightly though - like only to break existing installations. But you're right, this needs to be carefully documented for the upgrade to the acct_mgr-0.4 release.

Btw, you could avoid a lot of the hassle for upgrading a multi-environment setup, if you create rather small per-environment trac.ini files and move the common settings to another file, that is shared between all of them. That's what the [inherit] section is for:

[inherit]
file = /path/to/global/trac.ini

or better: You could use the wildcard syntax for module/multi-module activation:

[components]
acct_mgr.* = enabled

Speaking of documentation, you could do me and all of us another favor, if you'd update HttpAuthStore to reflect the new functionality for acct_mgr-0.4.

[Steffen Hoffmann]

You sent me into another interesting journey back even beyond the acct_mgr-0.2.1 release, that I've been investigating before for the changelog file.

Turns out that at least "*adminpanel" -> "*adminpage" was a misconception of mine, because "panel" was/is the proper Trac name for all admin pages. I'll revert this causing even more hassle to early adopters, but leaving one point less to watch out for upgrades from current stable the next release. A new file Readme.upgrade is on the way too, review welcome once committed (will refer to it here).

[Dennis McRitchie <dmcr@…>]

Thanks for the tip about inherit, which I might be able to use for some settings.

Re the name changes, I appreciate the extra research that led you to revert one of them. And I understand that password_file had to be dropped. But where possible (e.g., acct_mgr.admin.accountmanageradminpage -> acct_mgr.admin.accountmanageradminpages) I would strongly advocate for backward compatibility whenever possible. It would seem in this type of case, both keywords could be accepted (i.e., check for the preferred one, and only if not present, check for the deprecated one). Then the documentation can reflect the change without causing pain.

Re the need for the re-verification of my email address that I reported earlier, were you expecting this? This is more serious, because this doesn't just give a headache to the admin, but potentially to all the users. If this is expected, is there anyway to mitigate the pain? Or does this need more investigation.

As for HttpAuthStore, I had already updated the main plugin documentation for it, as well as added a section to the Trac/WSGI documentation page, to reflect current behavior. I will change it further to reflect new 0.4 behavior, and report back when I've done so.

Dennis

[Steffen Hoffmann]

Replying to Dennis McRitchie <dmcr@princeton.edu>:

Thanks for the tip about inherit, which I might be able to use for some settings.

You're welcome.

Re the name changes, ...

The configuration is a direct match of module and class names provided, so retaining old names would require to keep the old names in place and sync (implicit as well as explicit enabled/disabled state) of both. Rather ugly and error prone.

Just consider there could be both names evaluating to different states, and the corresponding matrix of states and (automatically selected) solutions might again not satisfy everyone and every application - but without a chance to overwrite without source changes, or introduce a dedicated option to switch it off. Ugly at a power of two, minimum.

I guess, at least this issue sets Trac apart from trac-hacks for that bit of stability and reliability of module names.

Anyway I reconsider all solutions again and will follow-up later.

Re the need for the re-verification of my email address that I reported earlier, were you expecting this? This is more serious, because this doesn't just give a headache to the admin, but potentially to all the users. If this is expected, is there anyway to mitigate the pain? Or does this need more investigation.

More investigation. Be assured, that I take this serious too. I looked into it almost instantly after initially reading your complaint but didn't get a clue immediately. Btw, you're aware of any email address change causing a re-verification, aren't you?

As for HttpAuthStore, I had already updated the main plugin documentation for it, as well as added a section to the Trac/WSGI documentation page, to reflect current behavior. I will change it further to reflect new 0.4 behavior, and report back when I've done so.

Obviously I didn't look at the wiki page close enough, sorry. And thanks for taking care.

[Steffen Hoffmann]

(In [11054]) AccountManagerPlugin: Rename some components - again, refs #9618.

Let's adhere to Trac notation as the general rule of thumb. But First and foremost we introduce and will further accumulate update notes to remind of important steps on plugin update to all future releases.

[Dennis McRitchie <dmcr@…>]

Will double check the bahavior of the email verification module upon my return in the new year. Just FYI, I was logged in as an admin user with a verified email address, updated the account manager plugin, restarted apache, and then refreshed the browser that had my logged-in session. That is when it told me that the my email address needed verification. Logging our and back in did not fix it.

Dennis

[Dennis McRitchie <dmcr@…>]

I have now updated the HttpAuthStore and WSGI documentation to reflect the use of acct_mgr-0.4. Please check for accuracy.

I have also added a few more warnings to supplement your text regarding the problems that can result, when upgrading, because of the two keyword name changes we discussed earlier.

Dennis

[Steffen Hoffmann]

(In [12398]) AccountManagerPlugin: Releasing version 0.4, pushing development to acct_mgr-0.5dev.

Availability of that code as stable release closes #874, #3459, #4677, #5295, #5691, #6616, #7577, #8076, #8685, #8770, #8791, #8990, #9052, #9079, #9090, #9139, #9246, #9252, #9547, #9618, #9676, #9843, #9852, #9940, #10023, #10028, #10123, #10142, #10204, #10276, #10397, #10412, #10594, #10625 and #10644.

Some more issues have been worked-on, yet without confirmed resolution, refs #5464 (for JiraToTracIntegration), #8927 and #10134.

And finally there are some issues and enhancement requests showing progress, but known to require more work to resolve them satisfactorily, refs #843, #1600, #5964, #8217, #8933.

Thanks to all contributors and followers, that enabled and encouraged a good portion of this development work.