avoid removing subscriber destinations secrets on update (superdesk#2583

)

SDESK-7271

features/subscribers.feature

@@ -448,3 +448,41 @@ Feature: Subscribers
     """
     {"expiry": "__any_value__"}
     """
+
+  @auth
+  Scenario: Adding destination should not remove secret key
+    When we post to "subscribers"
+    """
+    {
+      "name": "Foo",
+      "email": "admin@localhost",
+      "subscriber_type": "all",
+      "sequence_num_settings": {"min": 1, "max": 2},
+      "destinations": [{"name":"newsroom","format": "ninjs", "delivery_type":"http_push","config":{"secret_token":"foo", "resource_url": "http://example.com"}}]
+    }
+    """
+    Then we get OK response
+
+    When we get "subscribers"
+    Then we get existing resource
+    """
+    {"_items": [{"name": "Foo", "destinations": [{"name":"newsroom","format": "ninjs", "delivery_type":"http_push","config":{"secret_token":"__no_value__"}, "_id": "8f4ced9c036772413b61fc1ed547cd3cc3a0ffba"}]}]}
+    """
+
+    When we get "subscribers/#subscribers._id#"
+    Then we get existing resource
+    """
+    {"name": "Foo", "destinations": [{"name":"newsroom","format": "ninjs", "delivery_type":"http_push","config":{"secret_token":"foo"}}]}
+    """
+
+    When we patch "/subscribers/#subscribers._id#"
+    """
+    {"name": "Foo", "is_active": true, "destinations": [{"name":"newsroom","format": "ninjs", "delivery_type":"http_push","config":{"resource_url": "example.com"}, "_id": "8f4ced9c036772413b61fc1ed547cd3cc3a0ffba"}]}
+    """
+    Then we get OK response
+
+    When we get "subscribers/#subscribers._id#"
+    Then we get existing resource
+    """
+    {"name": "Foo", "destinations": [{"name":"newsroom","format": "ninjs", "delivery_type":"http_push","config":{"secret_token":"foo"}, "_id": "8f4ced9c036772413b61fc1ed547cd3cc3a0ffba"}]}
+    """

superdesk/publish/subscribers.py

@@ -16,7 +16,7 @@
 from flask import current_app as app
 from superdesk import get_resource_service
 from eve.utils import ParsedRequest, config
-from superdesk.utils import ListCursor
+from superdesk.utils import ListCursor, get_dict_hash
 from superdesk.resource import Resource, build_custom_hateoas
 from superdesk.services import CacheableService
 from superdesk.errors import SuperdeskApiError
@@ -28,6 +28,12 @@
 logger = logging.getLogger(__name__)
 
 
+def get_destination_id(destination) -> str:
+    if destination.get("_id"):
+        return destination["_id"]
+    return get_dict_hash(destination)
+
+
 class SubscribersResource(Resource):
     schema = {
         "name": {"type": "string", "iunique": True, "required": True, "nullable": False, "empty": False},
@@ -60,6 +66,7 @@ class SubscribersResource(Resource):
                     "preview_endpoint_url": {"type": "string"},
                     "delivery_type": {"type": "string", "required": True},
                     "config": {"type": "dict"},
+                    "_id": {"type": "string"},
                 },
             },
         },
@@ -125,6 +132,20 @@ def on_update(self, updates, original):
         subscriber = deepcopy(original)
         subscriber.update(updates)
         self._validate_products_destinations(subscriber)
+        self.keep_destinations_secrets(updates, original)
+
+    def keep_destinations_secrets(self, updates, original):
+        """Populate the secrets removed on fetch so those won't be overriden on save."""
+        original_destinations = original.get("destinations") or []
+        updates_destinations = updates.get("destinations") or []
+        for destination in original_destinations:
+            if not destination.get("config"):
+                continue
+            dest_id = get_destination_id(destination)
+            for update_destination in updates_destinations:
+                if dest_id == update_destination.get("_id"):
+                    for field, value in destination["config"].items():
+                        update_destination["config"].setdefault(field, value)
 
     def on_updated(self, updates, original):
         push_notification("subscriber:update", _id=[original.get(config.ID_FIELD)])
@@ -303,6 +324,7 @@ def hideConfigField(self, docs, fields):
                         {
                             **destination,
                             "config": {key: value for key, value in destination["config"].items() if key not in fields},
+                            "_id": get_destination_id(destination),
                         }
                         for destination in doc.get("destinations", [])
                     ],

superdesk/utils.py

@@ -26,8 +26,8 @@
 from bson import ObjectId
 from enum import Enum
 from importlib import import_module
-from eve.utils import config
-from typing import Any, Dict, Iterator, Optional
+from eve.utils import config, document_etag
+from typing import Any, Dict, Iterator, Mapping, Optional
 from superdesk.default_settings import ELASTIC_DATE_FORMAT, ELASTIC_DATETIME_FORMAT
 from superdesk.text_utils import get_text
 from flask import current_app as app
@@ -367,3 +367,7 @@ def abort(status: int, message: str) -> None:
 
 def get_list_chunks(items, chunk_size=100):
     return [items[i : i + chunk_size] for i in range(0, len(items), chunk_size)]
+
+
+def get_dict_hash(data: Mapping) -> str:
+    return document_etag(data)