Add proper validation that user_id is presentin the create and update requests

Author: VeljkoMaksimovic

notifications/lib/notifications/api/internal_api/create.ex

@@ -11,7 +11,7 @@ defmodule Notifications.Api.InternalApi.Create do
     org_id = req.metadata.org_id
     creator_id = req.metadata.user_id
 
-    with {:ok, :valid} <- Validator.validate(req.notification),
+    with {:ok, :valid} <- Validator.validate(req.notification, creator_id),
          {:ok, n} <- create_notification(org_id, creator_id, req.notification) do
       %CreateResponse{notification: Serialization.serialize(n)}
     else

notifications/lib/notifications/api/internal_api/update.ex

@@ -9,10 +9,11 @@ defmodule Notifications.Api.InternalApi.Update do
 
   def run(req) do
     org_id = req.metadata.org_id
+    updater_id = req.metadata.user_id
 
-    with {:ok, :valid} <- Validator.validate(req.notification),
+    with {:ok, :valid} <- Validator.validate(req.notification, updater_id),
          {:ok, n} <- find_by_id_or_name(org_id, req.id, req.name),
-         {:ok, n} <- update_notification(n, req.notification) do
+         {:ok, n} <- update_notification(n, updater_id, req.notification) do
       %UpdateResponse{notification: Serialization.serialize(n)}
     else
       {:error, :invalid_argument, message} ->
@@ -37,11 +38,12 @@ defmodule Notifications.Api.InternalApi.Update do
   # notification: existing row from database
   # apiresource: new notification from the API call that needs to be applied
   #
-  defp update_notification(notification, apiresource) do
+  defp update_notification(notification, updater_id, apiresource) do
     Repo.transaction(fn ->
       changes =
         Model.changeset(notification, %{
           name: apiresource.name,
+          creator_id: updater_id,
           spec: Notifications.Util.Transforms.encode_spec(%{rules: apiresource.rules})
         })
 

notifications/lib/notifications/api/public_api/create.ex

@@ -12,7 +12,7 @@ defmodule Notifications.Api.PublicApi.Create do
     Logger.info("#{inspect(org_id)} #{inspect(user_id)} #{name}")
 
     with {:ok, :authorized} <- Auth.can_manage?(user_id, org_id),
-         {:ok, :valid} <- Validator.validate(notification),
+         {:ok, :valid} <- Validator.validate(notification, user_id),
          {:ok, n} <- create_notification(org_id, user_id, notification) do
       Serialization.serialize(n)
     else

notifications/lib/notifications/api/public_api/update.ex

@@ -12,9 +12,9 @@ defmodule Notifications.Api.PublicApi.Update do
     Logger.info("#{inspect(org_id)} #{inspect(user_id)} #{name_or_id}")
 
     with {:ok, :authorized} <- Auth.can_manage?(user_id, org_id),
-         {:ok, :valid} <- Validator.validate(req.notification),
+         {:ok, :valid} <- Validator.validate(req.notification, user_id),
          {:ok, n} <- Model.find_by_id_or_name(org_id, name_or_id),
-         {:ok, n} <- update_notification(n, req.notification) do
+         {:ok, n} <- update_notification(n, user_id, req.notification) do
       Serialization.serialize(n)
     else
       {:error, :permission_denied} ->
@@ -44,11 +44,12 @@ defmodule Notifications.Api.PublicApi.Update do
   # notification: existing row from database
   # apiresource: new notification from the API call that needs to be applied
   #
-  def update_notification(notification, apiresource) do
+  def update_notification(notification, creator_id, apiresource) do
     Repo.transaction(fn ->
       changes =
         Model.changeset(notification, %{
           name: apiresource.metadata.name,
+          creator_id: creator_id,
           spec: Notifications.Util.Transforms.encode_spec(apiresource.spec)
         })
 

notifications/lib/notifications/util/validator.ex

@@ -1,16 +1,24 @@
 defmodule Notifications.Util.Validator do
   alias Notifications.Models.Pattern
 
-  def validate(notification) do
+  def validate(notification, user_id) do
     with {:ok, :valid} <- validate_regex_patterns(notification),
          {:ok, :valid} <- validate_result_patterns(notification),
-         {:ok, :valid} <- validate_notification_targets(notification) do
+         {:ok, :valid} <- validate_notification_targets(notification),
+         {:ok, :valid} <- validate_user_id(user_id) do
       {:ok, :valid}
     else
       e -> e
     end
   end
 
+  defp validate_user_id(user_id) do
+    case Ecto.UUID.cast(user_id) do
+      {:ok, _} -> {:ok, :valid}
+      :error -> {:error, :invalid_argument, "Invalid user_id: expected a valid UUID"}
+    end
+  end
+
   def validate_notification_targets(notification = %InternalApi.Notifications.Notification{}),
     do: validate_notification_targets_(notification.rules)
 

notifications/test/notifications/api/internal_api/update_test.exs

@@ -14,7 +14,7 @@ defmodule Notifications.Api.InternalApi.UpdateTest do
       {:ok, channel} = GRPC.Stub.connect("localhost:50051")
 
       req = %UpdateRequest{
-        metadata: %RequestMeta{org_id: @org_id},
+        metadata: %RequestMeta{org_id: @org_id, user_id: @creator_id},
         name: "non-exist",
         notification: Support.Factories.Notification.internal_api_model("first")
       }
@@ -41,7 +41,7 @@ defmodule Notifications.Api.InternalApi.UpdateTest do
         })
 
       req = %UpdateRequest{
-        metadata: %RequestMeta{org_id: @org_id},
+        metadata: %RequestMeta{org_id: @org_id, user_id: @creator_id},
         name: "first",
         notification: Support.Factories.Notification.internal_api_model("first")
       }
@@ -126,7 +126,7 @@ defmodule Notifications.Api.InternalApi.UpdateTest do
              end)
     end
 
-    test "it returns serialized notification" do
+    test "returs error when user_id not present in metadata" do
       alias Notifications.Models.Notification
 
       {:ok, channel} = GRPC.Stub.connect("localhost:50051")
@@ -145,6 +145,30 @@ defmodule Notifications.Api.InternalApi.UpdateTest do
         notification: Support.Factories.Notification.internal_api_model("first")
       }
 
+      {:error, result} = Stub.update(channel, req)
+
+      assert match?(%GRPC.RPCError{message: "Invalid user_id: expected a valid UUID"}, result)
+    end
+
+    test "it returns serialized notification" do
+      alias Notifications.Models.Notification
+
+      {:ok, channel} = GRPC.Stub.connect("localhost:50051")
+
+      notification = Support.Factories.Notification.internal_api_model("first")
+
+      {:ok, _} =
+        Stub.create(channel, %CreateRequest{
+          metadata: %RequestMeta{org_id: @org_id, user_id: @creator_id},
+          notification: notification
+        })
+
+      req = %UpdateRequest{
+        metadata: %RequestMeta{org_id: @org_id, user_id: @creator_id},
+        name: "first",
+        notification: Support.Factories.Notification.internal_api_model("first")
+      }
+
       {:ok, %{notification: notification}} = Stub.update(channel, req)
 
       assert notification.name == "first"
@@ -196,7 +220,7 @@ defmodule Notifications.Api.InternalApi.UpdateTest do
 
       req =
         UpdateRequest.new(
-          metadata: %RequestMeta{org_id: @org_id},
+          metadata: %RequestMeta{org_id: @org_id, user_id: @creator_id},
           name: "first",
           notification: notif
         )
@@ -239,7 +263,7 @@ defmodule Notifications.Api.InternalApi.UpdateTest do
 
       req =
         UpdateRequest.new(
-          metadata: %RequestMeta{org_id: @org_id},
+          metadata: %RequestMeta{org_id: @org_id, user_id: @creator_id},
           name: "second-notification",
           notification: notif
         )

notifications/test/notifications/util/validator_test.exs

@@ -6,6 +6,8 @@ defmodule Notifications.Util.ValidatorTest do
   alias Notification.{Metadata, Spec}
   alias Spec.Rule
 
+  @user_id Ecto.UUID.generate()
+
   describe ".validate" do
     test "everything is valid" do
       n =
@@ -35,7 +37,39 @@ defmodule Notifications.Util.ValidatorTest do
             )
         )
 
-      assert Validator.validate(n) == {:ok, :valid}
+      assert Validator.validate(n, @user_id) == {:ok, :valid}
+    end
+
+    test "user_id is empty" do
+      n =
+        Notification.new(
+          metadata: Metadata.new(name: "A"),
+          spec:
+            Spec.new(
+              rules: [
+                Rule.new(
+                  name: "Example Rule",
+                  filter:
+                    Rule.Filter.new(
+                      projects: [
+                        "/.*/"
+                      ]
+                    ),
+                  notify:
+                    Rule.Notify.new(
+                      slack:
+                        Rule.Notify.Slack.new(
+                          endpoint: "https://whatever.com",
+                          channels: ["#testing-hq"]
+                        )
+                    )
+                )
+              ]
+            )
+        )
+
+      assert Validator.validate(n, "") ==
+               {:error, :invalid_argument, "Invalid user_id: expected a valid UUID"}
     end
 
     test "reports broken regexes" do
@@ -66,7 +100,7 @@ defmodule Notifications.Util.ValidatorTest do
             )
         )
 
-      assert Validator.validate(n) ==
+      assert Validator.validate(n, @user_id) ==
                {:error, :invalid_argument, "Pattern /*/ is not a valid regex statement"}
     end
 
@@ -101,7 +135,7 @@ defmodule Notifications.Util.ValidatorTest do
             )
         )
 
-      assert Validator.validate(n) ==
+      assert Validator.validate(n, @user_id) ==
                {:error, :invalid_argument,
                 "Patterns [/*/, /+dasdas/] are not valid regex statements"}
     end
@@ -134,7 +168,7 @@ defmodule Notifications.Util.ValidatorTest do
             )
         )
 
-      assert Validator.validate(n) ==
+      assert Validator.validate(n, @user_id) ==
                {:error, :invalid_argument,
                 "Value pass is not a valid result entry. Valid values are: passed, failed, canceled, stopped."}
     end
@@ -170,7 +204,7 @@ defmodule Notifications.Util.ValidatorTest do
             )
         )
 
-      assert Validator.validate(n) == {:ok, :valid}
+      assert Validator.validate(n, @user_id) == {:ok, :valid}
     end
 
     test "regex is valid result entry" do
@@ -201,7 +235,7 @@ defmodule Notifications.Util.ValidatorTest do
             )
         )
 
-      assert Validator.validate(n) == {:ok, :valid}
+      assert Validator.validate(n, @user_id) == {:ok, :valid}
     end
 
     test "no notify target specified => not valid" do
@@ -225,7 +259,7 @@ defmodule Notifications.Util.ValidatorTest do
             )
         )
 
-      assert Validator.validate(n) ==
+      assert Validator.validate(n, @user_id) ==
                {:error, :invalid_argument,
                 "A notification rule must have at least one notification target configured."}
     end
@@ -250,7 +284,7 @@ defmodule Notifications.Util.ValidatorTest do
             )
         )
 
-      assert Validator.validate(n) ==
+      assert Validator.validate(n, @user_id) ==
                {:error, :invalid_argument, "A notification rule must have a notify field."}
     end
 
@@ -282,7 +316,7 @@ defmodule Notifications.Util.ValidatorTest do
             )
         )
 
-      assert Validator.validate(n) == {:ok, :valid}
+      assert Validator.validate(n, @user_id) == {:ok, :valid}
     end
 
     test "only valid webhook target => valid" do
@@ -309,7 +343,7 @@ defmodule Notifications.Util.ValidatorTest do
             )
         )
 
-      assert Validator.validate(n) == {:ok, :valid}
+      assert Validator.validate(n, @user_id) == {:ok, :valid}
     end
 
     test "only valid email target => valid" do
@@ -334,7 +368,7 @@ defmodule Notifications.Util.ValidatorTest do
             )
         )
 
-      assert Validator.validate(n) == {:ok, :valid}
+      assert Validator.validate(n, @user_id) == {:ok, :valid}
     end
 
     test "valid + invalid rule => fails" do
@@ -374,7 +408,7 @@ defmodule Notifications.Util.ValidatorTest do
             )
         )
 
-      assert Validator.validate(n) ==
+      assert Validator.validate(n, @user_id) ==
                {:error, :invalid_argument, "A notification rule must have a notify field."}
     end
   end