Skip to content

Remove "p_" from mailadresses, change subject on lists #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jolly-jump
Copy link

  • when the project account is created, remove the leading "p_" from LMN-name.
  • when the maillist account is created, the name of the maillist is now "Verteiler name". ok this is opionated and german...
  • when the mail goes to a maillist, change the subject from "oldsubject" to "[name] oldsubject"
  • add possibility to set the ldap_filter for account creation and the sogo filter as environmental variables
  • add possibility to hardcode an not-verifying SSL-Keys

@PLanB2008
Copy link

Overall, it looks good. However, I’m not in favor of disabling the SSL certificate check. While changing “Mailinglist” to “Verteiler” makes it less technical, I agree, it also conflicts with our principle of designing everything in a more international way. Hardcoding it in German isn’t the right approach. The better solution would be to add an option that allows users to pick their preferred language and then reference this in a lookup table.

I really like the idea of removing the “p_” prefix, but how can we ensure this doesn’t interfere with other mailing lists or users? Currently, sophomorix handles conflict checking.

Just my two cents. Thanks for contributing!

@jolly-jump
Copy link
Author

Hi.
Thanks for the feedback.
I'll revert the german bit, because config option and lookup table is overkill for now.

The "p_" is such a pain in the neck, that I'd rather run in a conflict on the mailcow syncer.

When thinking about conflicts: An admin can always add a proxy address to any user and provoke a conflict on the mailcow syncer. So I am not sure, if sophomorix should be responsible for conflicts regarding an email adress. sophomorix could check a conflict of the local part, for sure, even with all proxy addresses. But then there might be someone using "[email protected]" while "[email protected]" is for a student and resolving the local part conflict is not enough 🙄

Thinking about all this, I am not sure, if the better solution would be, sophomorix would allow a change of the main email-adress of a project and a class.

I will try to change the "p_" replacement part making it optional, on a "if you know what you are doing" warning basis.

Copy link
Member

@dorianim dorianim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution. I didn't get a notification about this and I only saw it now, I'm sorry!

I added some comments. One more thing to keep in mind: mailcow changed the authentication system in their March 2025 update, which probably breaks this entire package. So it may not be worth putting in more time before someone commits to maintaining this, see #3

@@ -9,6 +9,8 @@ def __init__(self, ldapUri, ldapBindDn, ldapBindPassword, ldapBaseDn):

def bind(self):
try:
# uncomment to disable CERT-Check on LDAP-Server
#ldap.set_option(ldap.OPT_X_TLS_REQUIRE_CERT, ldap.OPT_X_TLS_NEVER)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how a normal user would be able to use this. They would have to build their own docker image. If you want to add this, please make it configurable using an env variable.

@@ -150,13 +153,13 @@ def _sync(self):
"mail": mail,
"sophomorixStatus": "U",
"sophomorixMailQuotaCalculated": 1,
"displayName": mailingList["sAMAccountName"] + " (list)"
"displayName": "Verteiler " + desc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this or make it configurable (see @PLanB2008 's comment).

scriptData += "require \"editheader\";\r\n"
scriptData += "require \"copy\";\r\n"
scriptData += "require \"variables\";\r\n"
scriptData += "set \"addendum\" \""+description+"\";\r\n"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you using description here instead of cn? Is there a way to alter the description from the webui?

@@ -133,6 +133,9 @@ def _sync(self):
continue

mail = mailingList["mail"]
if mail.startswith("p_"):
mail = mail[2:]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this. As @PLanB2008 mentioned previously, there can be conflicts by blindly doing this.
Also, it will change currently existing lists! This is unacceptable because it will break the setup for everyone who is using the lists with p_ prefix.

I could imagine adding an option to automatically create aliases without the p_ prefix, as this would not break existing setups and would have to be enabled manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants