redhatcz / Thesis-management-system

https://diplomky.redhat.com
15 stars 8 forks source link

#58 - Allow registration mail filter to operate over 2nd level domains #116

Closed pehala closed 8 years ago

pehala commented 8 years ago

Overview

Only thing I had to do was change function replaceAll() for replace(), and only difference between these functions that replaceAll() accepts REGEX patterns, which is not needed when replacing dots. Email filter should now accept all REGEX patterns except for dot, which is being filtered out. I also created test to prove that method now accepts patterns.

Changes in files

Util.groovy

test/unit/UtilSpecification.groovy

VaclavDedik commented 8 years ago

Looks good :) .. One thing though, I think we shouldn't allow patterns like "*vutbr.cz". It would basically make the whole point of allowing only certain domains quite ineffective as you could just register your own domain (e.g. something like "somethingsomethingvutbr.cz") and circumvent the email domain validation.

So I would just add a validation to the system configuration form that checks if the character after "*" is a dot ("."). If an administrator added an email domain pattern that doesn't contain a dot after the asterisk, I would let the form submission fail and inform the admin that this kind of pattern is not allowed.

pehala commented 8 years ago

I think that since it's up to system administrator, we should not limit them. Maybe add warning that it would be better to use pattern with "*." or even better add text that says that they can use REGEX patterns and link to REGEX documentation, same way as it's done with Markdown.

jcechace commented 8 years ago

We should certainly limit them. The administrators don't necessarily have to be technical people. For the same reason I think that directly suing regex field is not a good idea (as regexes are way more complex than markdown. )

I mostly agree with Vaclav. The check should however account for 2nd level domains as well. That means either "." or "@" should directly precede allowed domain main.

pehala commented 8 years ago

Overview

I did some changes as you requested:

Changes in files

ConfigurationCommand.groovy

ConfigurationController.groovy

configuration/_form.bsp

Util.groovy

UtilSpecification.groovy

VaclavDedik commented 8 years ago

Doesn't work for me, it says "Your allowed email configuration contains forbidden characters." when I try to update the site configuration, even though it clearly doesn't. I think it's also a good idea to mention what characters are allowed.

pehala commented 8 years ago

Ok, fixed. I had to switch method which I use to add parameters to validator.

VaclavDedik commented 8 years ago

Verified. Merged, thanks!