global: new domain list feature #468

lnielsen · 2023-12-14T07:07:20Z

New domain list feature that is used to:

disallow user registration from specific email domains
automatically verify users from specific email domains.
gather statistics about number of users (and their state) in each domain to help identify high-risk domains
connect domains with known organisations to allow for reporting and identifying low-risk domains.

Checklist

Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:

invenio_accounts/alembic/6ec5ce377ca3_create_domains.py

lnielsen · 2023-12-14T07:53:58Z

invenio_accounts/models.py

@@ -103,6 +103,9 @@ class User(db.Model, Timestamp, UserMixin):
    _email = db.Column("email", db.String(255), unique=True)
    """User email."""

+    domain = db.Column(db.String(255), nullable=True)


New domain property is added on the user to allow for making a join against the domain table (needed for statistics).

lnielsen · 2023-12-14T07:55:21Z

invenio_accounts/models.py

@@ -463,3 +468,153 @@ def delete_by_user(cls, method, user):
        """Unlink a user from an external id."""
        with db.session.begin_nested():
            cls.query.filter_by(id_user=user.id, method=method).delete()
+
+
+class DomainOrg(db.Model):


Needed for being able to do statistics on different domains. E.g. a typical case is email domains like physics.uni.org and bio.uni.org all linking to the same organisation.

lnielsen · 2023-12-14T07:56:35Z

invenio_accounts/models.py

+    name = db.Column(db.String(255), nullable=False)
+    """Name of organisation."""
+
+    json = db.Column(


will be used on zenodo to store e.g. country_code, in_eu, in_cern_ms, in_cern_ams which can be used for statistics purposes.

lnielsen · 2023-12-14T07:56:43Z

invenio_accounts/models.py

+
+    id = db.Column(db.Integer(), primary_key=True, autoincrement=True)
+
+    pid = db.Column(db.String(255), unique=True, nullable=True)


on zenodo a ROR

lnielsen · 2023-12-14T07:58:48Z

invenio_accounts/models.py

+    )
+    """Link to parent organisation."""
+
+    parent = db.relationship("DomainOrg", remote_side=[id])


Allows for creating hierarchy of organisations - useful again for statistics (on Zenodo the ROR hierarchy)

lnielsen · 2023-12-14T07:59:18Z

invenio_accounts/models.py

+        return obj
+
+
+class DomainCategory(db.Model):


Label a domain with a category, on Zenodo will be used to classify domains in e.g. spammer, mail-provider, org, company.

lnielsen · 2023-12-14T08:01:12Z

invenio_accounts/models.py

+    Use to control possibility and capability of users registering with this domain.
+    """
+
+    flagged = db.Column(db.Boolean(), default=False, nullable=False)


A boolean to flag a domain automatically - e.g. if we download domains from https://github.com/disposable/disposable then domains can be flagged for further inspection by an admin. This is different from status which is used directly to control user registration from the domain.

lnielsen · 2023-12-14T08:02:18Z

invenio_accounts/models.py

+    category = db.Column(db.Integer(), db.ForeignKey(DomainCategory.id), nullable=True)
+    """Category of domain."""
+
+    num_users = db.Column(db.Integer(), default=0, nullable=False)


Will be computed on say daily/weekly basis (not part of PR). Needed for easy analysis of domains and detecting domains with high number of blocked users.

lnielsen · 2023-12-14T08:03:13Z

invenio_accounts/utils.py

@@ -216,3 +239,33 @@ def validate_username(username):
        # text explaining the validation rules.
        message = current_app.config["ACCOUNTS_USERNAME_RULES_TEXT"]
        raise ValueError(message)
+
+
+def validate_domain_form(form, field):


Will be needed in user-profiles to check the domain when a user want to change their email address.

lnielsen · 2023-12-14T09:49:51Z

invenio_accounts/forms.py

@@ -44,6 +45,16 @@ def __init__(self, *args, **kwargs):
            if not self.next.data:
                self.next.data = request.args.get("next", "")

+        def validate(self, extra_validators=None):


During registration we check the domain. We only check when the registration forms require email confirmation.

ntarocco

LGTM! Added a few comments as food for thoughts, no need to answer, feel free to merge anytime.

ntarocco · 2023-12-14T12:21:20Z

invenio_accounts/models.py

+
+    __tablename__ = "accounts_domain_org"
+
+    id = db.Column(db.Integer(), primary_key=True, autoincrement=True)


Are you ok with autoincrement ids, or better going for UUIDs by default? :)

ntarocco · 2023-12-14T12:24:30Z

invenio_accounts/models.py

+
+    __tablename__ = "accounts_domains"
+
+    id = db.Column(db.Integer(), primary_key=True, autoincrement=True)


Same here about the autoincrement

ntarocco · 2023-12-14T12:26:23Z

invenio_accounts/models.py

+    flagged = db.Column(db.Boolean(), default=False, nullable=False)
+    """Flag domain - used by automatic processes to flag domain."""
+
+    flagged_source = db.Column(db.String(255), default="", nullable=False)


Given that you have String(255), this field is more a keyword rather than a free text right?

ntarocco · 2023-12-14T12:28:52Z

invenio_accounts/utils.py

+    prefix, domain = email.split("@", 1)
+    prefix = prefix.lower().strip()
+    domain = domain.lower().strip()
+    if domain[-1] == ".":


Why this check?

I guess because [email protected]. is a valid email address, but we want to "normalize" the domain.

ntarocco · 2023-12-14T12:31:29Z

invenio_accounts/domains.py

+    # Domain is inserted on domain list when a user confirms their email.
+    domain = datastore.find_domain(user.domain)
+    if domain is None:
+        domain = datastore.create_domain(user.domain)


Given that we are doing a session.add() here, is a db.session.commit always happening after this?

Yep, checked. user_confirmed signal is sent from the confirm_user() function that's called here, which will do the commit:

https://github.com/inveniosoftware/flask-security-fork/blob/b99556579f4bbc9d1425e79d1f9f0d115e5baf6a/flask_security/views.py#L146-L147

ntarocco · 2023-12-14T12:36:42Z

invenio_accounts/models.py

+        return cls.query.filter_by(label=label).one_or_none()
+
+
+class Domain(db.Model, Timestamp):


Does it make sense to add __versioned__ = {"versioning": False} to this one given the various num_* columns updated daily by the cron?

This is just a plain model, so it doesnt' have any versioning on by default.

slint

LGTM! 2 minor comments

slint · 2024-01-08T13:02:19Z

invenio_accounts/datastore.py

+    def create_domain(self, domain, **kwargs):
+        """Create a new domain."""
+        domain = Domain.create(domain, **kwargs)
+        self.put(domain)


minor: this eventually calls self.mark_changed(..., uid=domain.id), probably just callingself.db.session.add(domain) is enough.

Actually, Domain.create() already does db.session.add() so line can be removed.

slint · 2024-01-08T13:05:40Z

invenio_accounts/utils.py

+    prefix, domain = email.split("@", 1)
+    prefix = prefix.lower().strip()
+    domain = domain.lower().strip()
+    if domain[-1] == ".":


I guess because [email protected]. is a valid email address, but we want to "normalize" the domain.

slint · 2024-01-08T16:13:37Z

invenio_accounts/utils.py

+
+def split_emailaddr(email):
+    """Split email address in prefix and domain."""
+    prefix, domain = email.split("@", 1)


Suggested change

prefix, domain = email.split("@", 1)

prefix, domain = email.rsplit("@", 1)

To avoid bypassing with something like "user@foo"@gmail.com, which would return foo"@gmail.com as the domain (see this SO answer)

* New domain list feature that can be used to block email domains from registering, and automatically verifying users from other domains.

lnielsen commented Dec 14, 2023

View reviewed changes

invenio_accounts/alembic/6ec5ce377ca3_create_domains.py Outdated Show resolved Hide resolved

lnielsen force-pushed the domainlist branch from 21aeb12 to 304b9c0 Compare December 14, 2023 07:51

lnielsen commented Dec 14, 2023

View reviewed changes

ntarocco approved these changes Dec 14, 2023

View reviewed changes

slint approved these changes Jan 8, 2024

View reviewed changes

global: new domain list feature

36a05be

* New domain list feature that can be used to block email domains from registering, and automatically verifying users from other domains.

lnielsen force-pushed the domainlist branch from 304b9c0 to 36a05be Compare January 9, 2024 20:20

lnielsen merged commit 2ad7fdc into inveniosoftware:master Jan 9, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

global: new domain list feature #468

global: new domain list feature #468

lnielsen commented Dec 14, 2023 •

edited

Loading

lnielsen Dec 14, 2023

lnielsen Dec 14, 2023

lnielsen Dec 14, 2023 •

edited

Loading

lnielsen Dec 14, 2023

lnielsen Dec 14, 2023

lnielsen Dec 14, 2023

lnielsen Dec 14, 2023

lnielsen Dec 14, 2023

lnielsen Dec 14, 2023

lnielsen Dec 14, 2023

ntarocco left a comment

ntarocco Dec 14, 2023

ntarocco Dec 14, 2023

ntarocco Dec 14, 2023

lnielsen Jan 9, 2024

ntarocco Dec 14, 2023

slint Jan 8, 2024

ntarocco Dec 14, 2023

lnielsen Jan 9, 2024

ntarocco Dec 14, 2023

lnielsen Jan 9, 2024

slint left a comment

slint Jan 8, 2024

lnielsen Jan 9, 2024

slint Jan 8, 2024

slint Jan 8, 2024


		id = db.Column(db.Integer(), primary_key=True, autoincrement=True)

		pid = db.Column(db.String(255), unique=True, nullable=True)


		__tablename__ = "accounts_domain_org"

		id = db.Column(db.Integer(), primary_key=True, autoincrement=True)


		__tablename__ = "accounts_domains"

		id = db.Column(db.Integer(), primary_key=True, autoincrement=True)

		return cls.query.filter_by(label=label).one_or_none()


		class Domain(db.Model, Timestamp):

	prefix, domain = email.split("@", 1)
	prefix, domain = email.rsplit("@", 1)

global: new domain list feature #468

global: new domain list feature #468

Conversation

lnielsen commented Dec 14, 2023 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lnielsen Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntarocco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slint left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lnielsen commented Dec 14, 2023 •

edited

Loading

lnielsen Dec 14, 2023 •

edited

Loading