Revert "use Protocol.create_for for all protocols, not just HAS_COPIES ones"

This reverts commit 9f9ecce187.

Not 100% sure but I suspect this caused #1929. Reverting while I debug more.

activitypub.py

@@ -3,12 +3,10 @@ from base64 import b64encode
 from hashlib import sha256
 import itertools
 import logging
-import random
 import re
 from urllib.parse import quote_plus, urljoin, urlparse
 from unittest.mock import MagicMock
 
-from Crypto.PublicKey import RSA
 from flask import abort, g, redirect, request
 from google.cloud import ndb
 from google.cloud.ndb.query import FilterNode, OR, Query
@@ -36,7 +34,6 @@ from common import (
     error,
     host_url,
     LOCAL_DOMAINS,
-    long_to_base64,
     PRIMARY_DOMAIN,
     PROTOCOL_DOMAINS,
     redirect_wrap,
@@ -46,7 +43,7 @@ from common import (
 )
 from ids import BOT_ACTOR_AP_IDS
 import memcache
-from models import fetch_objects, Follower, KEY_BITS, Object, PROTOCOLS, User
+from models import fetch_objects, Follower, Object, PROTOCOLS, User
 from protocol import activity_id_memcache_key, DELETE_TASK_DELAY, Protocol
 import webfinger
 
@@ -88,7 +85,7 @@ def instance_actor():
     global _INSTANCE_ACTOR
     if _INSTANCE_ACTOR is None:
         import web
-        _INSTANCE_ACTOR = web.Web.get_or_create(PRIMARY_DOMAIN, propagate=True)
+        _INSTANCE_ACTOR = web.Web.get_or_create(PRIMARY_DOMAIN)
     return _INSTANCE_ACTOR
 
 
@@ -230,28 +227,6 @@ class ActivityPub(User, Protocol):
         kwargs['prefer_id'] = False
         return super().user_page_path(rest=rest, **kwargs)
 
-    @classmethod
-    def create_for(cls, user):
-        """Creates an AP keypair for a non-APProto user.
-
-        Can use urandom() and do nontrivial math, so this can take time
-        depending on the amount of randomness available and compute needed.
-
-        Args:
-          user (models.User)
-        """
-        assert not isinstance(user, ActivityPub)
-
-        if not user.public_exponent or not user.private_exponent or not user.mod:
-            assert (not user.public_exponent and not user.private_exponent
-                    and not user.mod), id
-            key = RSA.generate(KEY_BITS, randfunc=random.randbytes
-                               if appengine_info.DEBUG else None)
-            user.mod = long_to_base64(key.n)
-            user.public_exponent = long_to_base64(key.e)
-            user.private_exponent = long_to_base64(key.d)
-            user.put()
-
     @classmethod
     def target_for(cls, obj, shared=False):
         """Returns ``obj``'s or its author's/actor's inbox, if available."""
@@ -1076,8 +1051,7 @@ def _load_user(handle_or_id, create=False):
 
     assert id
     try:
-        user = (proto.get_or_create(id, propagate=True) if create
+        user = proto.get_or_create(id) if create else proto.get_by_id(id)
-                else proto.get_by_id(id))
     except ValueError as e:
         logging.warning(e)
         user = None

models.py

@@ -5,6 +5,7 @@ from functools import lru_cache
 import itertools
 import json
 import logging
+import random
 import re
 from threading import Lock
 from urllib.parse import quote, urlparse
@@ -29,6 +30,7 @@ import common
 from common import (
     base64_to_long,
     DOMAIN_RE,
+    long_to_base64,
     OLD_ACCOUNT_AGE,
     PROTOCOL_DOMAINS,
     report_error,
@@ -375,13 +377,30 @@ class User(StringIdModel, metaclass=ProtocolUserMeta):
                 proto = PROTOCOLS[label]
                 if proto == cls:
                     continue
-                elif user.is_enabled(proto):
+                elif proto.HAS_COPIES:
-                    try:
+                    if not user.get_copy(proto) and user.is_enabled(proto):
-                        proto.create_for(user)
+                        try:
-                    except (ValueError, AssertionError):
+                            proto.create_for(user)
-                        logger.info(f'failed creating {proto.LABEL} copy',
+                        except (ValueError, AssertionError):
-                                    exc_info=True)
+                            logger.info(f'failed creating {proto.LABEL} copy',
-                        util.remove(user.enabled_protocols, proto.LABEL)
+                                        exc_info=True)
+                            util.remove(user.enabled_protocols, proto.LABEL)
+                    else:
+                        logger.debug(f'{proto.LABEL} not enabled or user copy already exists, skipping propagate')
+
+        # generate keys for all protocols _except_ our own
+        #
+        # these can use urandom() and do nontrivial math, so they can take time
+        # depending on the amount of randomness available and compute needed.
+        if cls.LABEL != 'activitypub':
+            if (not user.public_exponent or not user.private_exponent or not user.mod):
+                assert (not user.public_exponent and not user.private_exponent
+                        and not user.mod), id
+                key = RSA.generate(KEY_BITS,
+                                   randfunc=random.randbytes if DEBUG else None)
+                user.mod = long_to_base64(key.n)
+                user.public_exponent = long_to_base64(key.e)
+                user.private_exponent = long_to_base64(key.d)
 
         try:
             user.put()
@@ -586,9 +605,10 @@ class User(StringIdModel, metaclass=ProtocolUserMeta):
 
         added = False
 
-        # do this even if there's an existing copy since we might need to
+        if to_proto.LABEL in ids.COPIES_PROTOCOLS:
-        # reactivate it, which create_for should do
+            # do this even if there's an existing copy since we might need to
-        to_proto.create_for(self)
+            # reactivate it, which create_for should do
+            to_proto.create_for(self)
 
         @ndb.transactional()
         def enable():

protocol.py

@@ -497,22 +497,20 @@ class Protocol:
 
     @classmethod
     def create_for(cls, user):
-        """Creates or re-activate a user in this protocol.
+        """Creates or re-activate a copy user in this protocol.
 
-        If this protocol has copies, adds the new copy user to :attr:`copies`.
+        Should add the copy user to :attr:`copies`.
-        If the copy user already exists and active, does nothing.
 
-        By default, does nothing.
+        If the copy user already exists and active, should do nothing.
 
         Args:
           user (models.User): original source user. Shouldn't already have a
             copy user for this protocol in :attr:`copies`.
 
         Raises:
-          ValueError: if we can't create the given user in this protocol
+          ValueError: if we can't create a copy of the given user in this protocol
         """
-        if cls.HAS_COPIES:
+        raise NotImplementedError()
-            raise NotImplementedError()
 
     @classmethod
     def send(to_cls, obj, url, from_user=None, orig_obj_id=None):

tests/test_models.py

@@ -72,8 +72,7 @@ class UserTest(TestCase):
         self.assertIsNone(Web.get_by_id('y.za'))
 
     def test_get_or_create(self):
-        user = Fake.get_or_create('fake:user', enabled_protocols=['activitypub'],
+        user = Fake.get_or_create('fake:user')
-                                  propagate=True)
 
         assert not user.existing
         assert user.mod
@@ -188,17 +187,11 @@ class UserTest(TestCase):
         self.assertIsNone(Fake.get_or_create('fake:user', manual_opt_out=True))
 
     def test_public_pem(self):
-        bot = self.make_user(id='ap.brid.gy', cls=Web)
-        self.user.enable_protocol(ActivityPub)
-
         pem = self.user.public_pem()
         self.assertTrue(pem.decode().startswith('-----BEGIN PUBLIC KEY-----\n'), pem)
         self.assertTrue(pem.decode().endswith('-----END PUBLIC KEY-----'), pem)
 
     def test_private_pem(self):
-        bot = self.make_user(id='ap.brid.gy', cls=Web)
-        self.user.enable_protocol(ActivityPub)
-
         pem = self.user.private_pem()
         self.assertTrue(pem.decode().startswith('-----BEGIN RSA PRIVATE KEY-----\n'), pem)
         self.assertTrue(pem.decode().endswith('-----END RSA PRIVATE KEY-----'), pem)
@@ -452,19 +445,6 @@ class UserTest(TestCase):
 
         self.assertIsNone(OtherFake().get_copy(Fake))
 
-    def test_enable_protocol_calls_create_for(self):
-        bot = self.make_user(id='ap.brid.gy', cls=Web)
-        user = Fake(id='fake:user')
-        user.put()
-
-        self.assertIsNone(user.public_exponent)
-        self.assertIsNone(user.private_exponent)
-
-        user.enable_protocol(ActivityPub)
-
-        self.assertIsNotNone(user.public_exponent)
-        self.assertIsNotNone(user.private_exponent)
-
     def test_count_followers(self):
         self.assertEqual((0, 0), self.user.count_followers())
 

tests/testutil.py

@@ -295,8 +295,7 @@ Web.DEFAULT_ENABLED_PROTOCOLS += ('fake', 'other')
 # expensive to generate.
 requests.post(f'http://{ndb_client.host}/reset')
 with ndb_client.context():
-    global_user = activitypub._INSTANCE_ACTOR = Fake.get_or_create(
+    global_user = activitypub._INSTANCE_ACTOR = Fake.get_or_create('fake:user')
-        'fake:user', propagate=True, enabled_protocols=['activitypub'])
 
 
 class TestCase(unittest.TestCase, testutil.Asserts):

web.py

@@ -192,7 +192,7 @@ class Web(User, Protocol):
             return None
 
         if verify or (verify is None and not user.existing):
-            user = user.verify(**kwargs)
+            user = user.verify()
 
         if not allow_opt_out and user.status:
             return None
@@ -289,13 +289,9 @@ class Web(User, Protocol):
 
         return super().status
 
-    def verify(self, **kwargs):
+    def verify(self):
         """Fetches site a couple ways to check for redirects and h-card.
 
-        Args:
-          **kwargs: passed through to :meth:`Web.get_or_create` if this user is a www
-            domain and we need to call it to create a new root domain user.
-
         Returns:
           web.Web: user that was verified. May be different than self! eg if
           self's domain started with www and we switch to the root domain.
@@ -314,7 +310,7 @@ class Web(User, Protocol):
                     logger.info(f'{root_site} serves ok ; using {root} instead')
                     root_user = Web.get_or_create(
                         root, enabled_protocols=self.enabled_protocols,
-                        allow_opt_out=True, **kwargs)
+                        allow_opt_out=True)
                     self.use_instead = root_user.key
                     self.put()
                     return root_user.verify()