[patch] Add optional account email regexp to registration checks

#5295 already proposed regexp to check for a valid username, so this is related.

I've a medium problem and some minor issues with this proposal:

problem

While we can and do check for (existence of a valid) email, if configured that way, we have absolutely no control over later changes. I hate to say it, but the email checks could be bypassed today even within AccountManagerPlugin itself. I've done some auditing regarding such issues before. A check that could be bypassed easily is no great value, so this is not a major issue to me, at least not, before more common and pressing issues like #7437 are resolved. And the bypass needs to be fixed too. Beware, that users can change (own) email address within Trac itself without any checking enforced, so the fix is non-trivial and will need more than one place in existing code to act upon.

minor issues

• Your options proposed name email_regexp would have to be more self-explaining to be acceptable right away, you see?
• Your options default value is an empty string. Why don't you give a hint on required format? Most people, even admins, are not regexp guru's. Meaningful use cases/examples are badly needed here. If I would just take the code, this would turn into a defect due to missing documentation. At least my judgement is that strict, sorry. Providing information to build that documentation would be the way to go here.
• Not sure, if you did recognize, that we already have some checking on email addresses.? Even if a non-issue performance-wise, this shouldn't be done multiple times to prevent config/code bloat. We may resort to just making the existing check configurabel and even fix the complaint about missing default value raise before.
• Please provide unified diff, if possible at all. This would enable Trac's own diff file preview as well. Your patches need to be downloaded to look at them.

That said, any proposal is appreciated, even more with patch attached. You have my attention for working towards an acceptable solution.

BTW, what's your use case? Enforcing domain restrictions in corporate application (*@mycompany.com)? Could be interesting to know and maybe I understand your requirement leading to this request even better.

The use case for this is that we have a web-accessible trac instance, and would prefer to allow users in the organization to register their own accounts. The permissions for unauthenticated users are set to empty. We have email account verification enabled, so the main use of this code would be to restrict registration and verification to users within the organization (*@mycompany.com as you mentioned). Because of this I'm not too worried about email changes after account registration. We don't have ldap or AD set up or accessible by the trac instance, otherwise we'd be using a unified login system. We would also potentially have external users signing up, so this could be used to enforce a multi-domain restriction.

Minor Issues:

• What would be a better descriptive name? Something like email_validation_regexp?
• What is the correct location to put an example regexp? In the doc portion of the option?
• I do see that there's a regexp for properly-formed emails and a non-empty field, which have value in themselves, but there isn't any mechanism for enforcing a specific format or domain of any kind.

I was thinking about doing a simple string comparison, extracting the domain from the email and checking against a config value, but a regular expression allows for some more flexibility

Replying to tchrist@nationalfield.org:

[...], so the main use of this code would be to restrict registration and verification to users within the organization (*@mycompany.com as you mentioned).

I see, but wouldn't it be more effective to block /register for users from outside then? The email restriction would i.e. not hold from producing a lot of noise, of a malicious being would like to register arbitrary xyz@mycompany.com addresses, just to cause some mischief and disturbance. But I see you point.

[...]
We would also potentially have external users signing up, so this could be used to enforce a multi-domain restriction.

Certainly regexp could do that.

Minor Issues: * What would be a better descriptive name? Something like email_validation_regexp?

Better.

* What is the correct location to put an example regexp? In the doc portion of the option?

To be honest, I don't know a guaranteed «correct» location. Wiki? At this is what I'll be working on within the next weeks. And I invite you heartily to join in.

If you come up with some suggestions regarding wiki content, you could just drop me a note directly.

* I do see that there's a regexp for properly-formed emails and a non-empty field, which have value in themselves, but there isn't any mechanism for enforcing a specific format or domain of any kind.

Correct, so I meant to extend this a little bit by making it configurable. Combine with a small cookbook of examples as documentation, that would do. Different opinion? I'm open to further discussion, not acting in a hurry, as I told you before.

I was thinking about doing a simple string comparison, extracting the domain from the email and checking against a config value, but a regular expression allows for some more flexibility

Good. I try to do it as generally usable as possible too, so we are already going same direction.

Replying to hasienda:

Replying to tchrist@nationalfield.org:

[...], so the main use of this code would be to restrict registration and verification to users within the organization (*@mycompany.com as you mentioned).

I see, but wouldn't it be more effective to block /register for users from outside then? The email restriction would i.e. not hold from producing a lot of noise, of a malicious being would like to register arbitrary xyz@mycompany.com addresses, just to cause some mischief and disturbance. But I see you point.

We have a dedicated email inbox, so it doesn't spam individuals that much. Also we have a number of distributed employees, which we could manage via a VPN and and internally hosted instance but we're not currently set up for that and we're trying to avoid additional internal infrastructure.

Minor Issues: * What would be a better descriptive name? Something like email_validation_regexp?

Better.

* What is the correct location to put an example regexp? In the doc portion of the option?

To be honest, I don't know a guaranteed «correct» location. Wiki? At this is what I'll be working on within the next weeks. And I invite you heartily to join in.

I could add an example regexp or two, but after a certain point there would only be so much you can do without knowledge of regular expressions. It seems like having a couple examples in the wiki and a general one in the doc section should be pretty good.

If you come up with some suggestions regarding wiki content, you could just drop me a note directly.

* I do see that there's a regexp for properly-formed emails and a non-empty field, which have value in themselves, but there isn't any mechanism for enforcing a specific format or domain of any kind.

Correct, so I meant to extend this a little bit by making it configurable. Combine with a small cookbook of examples as documentation, that would do. Different opinion? I'm open to further discussion, not acting in a hurry, as I told you before.

Well the generally valid and non-empty email checks are pretty general, enforcing a format check is an additional use case which probably needs its own handling. Would creating a configurable error message add value for this feature?

Replying to anonymous:

Replying to hasienda:

Replying to tchrist@nationalfield.org:

[...] * I do see that there's a regexp for properly-formed emails and a non-empty field, which have value in themselves, but there isn't any mechanism for enforcing a specific format or domain of any kind.

Correct, so I meant to extend this a little bit by making it configurable. Combine with a small cookbook of examples as documentation, that would do. Different opinion? I'm open to further discussion, not acting in a hurry, as I told you before.

[...] Would creating a configurable error message add value for this feature?

IMHO currently this is exactly the critical part, while the test itself shouldn't be hard to implement. Sorry, that has not been worked on for many months, but as part of current t-h.o migration plan it's getting back into development focus.

(In [11917]) AccountManagerPlugin: Add interface for configurable user registration procedure, refs #874, #2707, #2897, #4651, #5295, #7577, #8076.

The current user registration process lacks flexibility, as can be witnessed by the history of one of the oldest still pending tickets for this plugin: #874.

(In [11923]) AccountManagerPlugin: Finish registration check rewrite, refs #874, #2707, #2897, #4651, #5295, #7577, #8076.

Now moving check execution into AccountManager, and _create_user() is gone, replaced by more clearly structured and modularized code.

Registration checking for requests from the admin panel is re-activated and minor improvements related to RegistrationError? processing done too.

Note: The test, that detected admin requests to skip additional checking of existing permissions for each new username, has been done differently. In the future each check has to decide on its own, like this is done in the UsernamePermCheck now.

Replying to hasienda: ...

problem While we can and do check for (existence of a valid) email, if configured that way, we have absolutely no control over later changes. I hate to say it, but the email checks could be bypassed today even within AccountManagerPlugin itself.

...

Beware, that users can change (own) email address within Trac itself without any checking enforced, so the fix is non-trivial and will need more than one place in existing code to act upon.

Glad to know, there is #10204 for better visibility of the issue, and even more that it has been addressed by [11929] recently.

(In [11960]) AccountManagerPlugin: Add configurable regular expression registration checks, refs #5295 and #8076.

Credits

• for the email check part: suggested and drafted by T. Christ
• for the username check part:
  • suggested and darfted by Sebastian Krysmanski
  • further developed by M. Mitar

Thank you all for inspiring contributions.

I release this in the hope, that it's gonna get better than just the sum of the single patches. Surely it will, because we're using flexibility provided by the IAccountRegistrationInspector interface now. And while case-insensitive matching was deemed preferable this is no longer hard-coded but added to Python regexp by prepending (?i), so you've really full control over your new configurable regexp-driven checks.

And now README.update has caught up with recent changes and new features too.

I just noticed that the changelog reads acct_mgr-0.4 (not yet released) - branch 0.11, however it looks like it should be trunk rather than branch 0.11.

Read: I'm still developing trunk for the 0.11 branch, with as many features as possible from later versions - that's it.

Trust me, please, that the label is for good reason: On release day I'll just copy trunk code to 0.11 branch, tag it and than it's 100 % correct.

Not much value in keeping a separate change-log file for trunk itself. IMHO it's just the future of the most recent branch. But development models diverge in detail, so you don't need to agree. Note, that the feature was requested for Trac 0.12 - but you won't stop me from bringing it to Trac 0.11 too, right? Lets stop here, because all this is OT.

Ultimately I prefer information on the ticket subject. Yet no offense intended, thanks for taking care anyway.

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