Make sure users can't create profiles or groups with non-valid patterns

Closes #1068

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

lib/mobilizon/actors/actor.ex

@@ -19,6 +19,7 @@ defmodule Mobilizon.Actors.Actor do
   alias Mobilizon.Web.Endpoint
   alias Mobilizon.Web.Router.Helpers, as: Routes
   import Mobilizon.Web.Gettext, only: [dgettext: 2]
+  import Mobilizon.Service.Guards, only: [is_valid_string: 1]
 
   require Logger
 
@@ -313,6 +314,7 @@ defmodule Mobilizon.Actors.Actor do
     |> build_urls()
     |> common_changeset(attrs)
     |> unique_username_validator()
+    |> username_validator()
     |> validate_required(@registration_required_attrs)
   end
 
@@ -356,6 +358,7 @@ defmodule Mobilizon.Actors.Actor do
     |> put_change(:keys, Crypto.generate_rsa_2048_private_key())
     |> put_change(:type, :Group)
     |> unique_username_validator()
+    |> username_validator()
     |> validate_required(@group_creation_required_attrs)
     |> validate_length(:summary, max: 5000)
     |> validate_length(:preferred_username, max: 100)
@@ -381,6 +384,23 @@ defmodule Mobilizon.Actors.Actor do
   # When we don't even have any preferred_username, don't even try validating preferred_username
   defp unique_username_validator(changeset), do: changeset
 
+  defp username_validator(%Ecto.Changeset{} = changeset) do
+    username = Ecto.Changeset.fetch_field!(changeset, :preferred_username)
+
+    if is_valid_string(username) and Regex.match?(~r/^[a-z0-9_]+$/, username) do
+      changeset
+    else
+      add_error(
+        changeset,
+        :preferred_username,
+        dgettext(
+          "errors",
+          "Username must only contain alphanumeric lowercased characters and underscores."
+        )
+      )
+    end
+  end
+
   @spec build_urls(Ecto.Changeset.t(), atom()) :: Ecto.Changeset.t()
   defp build_urls(changeset, type \\ :Person)
 

test/graphql/resolvers/group_test.exs

@@ -8,7 +8,7 @@ defmodule Mobilizon.Web.Resolvers.GroupTest do
   alias Mobilizon.GraphQL.AbsintheHelpers
 
   @non_existent_username "nonexistent"
-  @new_group_params %{groupname: "new group"}
+  @new_group_params %{name: "new group", preferredUsername: "new_group"}
 
   setup %{conn: conn} do
     user = insert(:user)
@@ -17,49 +17,75 @@ defmodule Mobilizon.Web.Resolvers.GroupTest do
     {:ok, conn: conn, actor: actor, user: user}
   end
 
-  describe "create a group" do
+  describe "create_group/3" do
-    test "create_group/3 creates a group and check a group with this name does not already exist",
+    @create_group_mutation """
+    mutation CreateGroup(
+    $preferredUsername: String!
+    $name: String!
+    $summary: String
+    $avatar: MediaInput
+    $banner: MediaInput
+    ) {
+      createGroup(
+        preferredUsername: $preferredUsername
+        name: $name
+        summary: $summary
+        banner: $banner
+        avatar: $avatar
+      ) {
+        preferredUsername
+        type
+        banner {
+          id
+          url
+        }
+      }
+    }
+    """
+
+    test "creates a group and check a group with this name does not already exist",
          %{conn: conn, user: user} do
-      mutation = """
+      res =
-          mutation {
+        conn
-            createGroup(
+        |> auth_conn(user)
-              preferred_username: "#{@new_group_params.groupname}"
+        |> AbsintheHelpers.graphql_query(
-            ) {
+          query: @create_group_mutation,
-                preferred_username,
+          variables: @new_group_params
-                type
+        )
-              }
+
-            }
+      assert res["errors"] == nil
-      """
+
+      assert res["data"]["createGroup"]["preferredUsername"] ==
+               @new_group_params.preferredUsername
+
+      assert res["data"]["createGroup"]["type"] == "GROUP"
 
       res =
         conn
         |> auth_conn(user)
-        |> post("/api", AbsintheHelpers.mutation_skeleton(mutation))
+        |> AbsintheHelpers.graphql_query(
+          query: @create_group_mutation,
+          variables: @new_group_params
+        )
 
-      assert json_response(res, 200)["data"]["createGroup"]["preferred_username"] ==
+      assert hd(res["errors"])["message"] ==
-               @new_group_params.groupname
-
-      assert json_response(res, 200)["data"]["createGroup"]["type"] == "GROUP"
-
-      mutation = """
-          mutation {
-            createGroup(
-              preferred_username: "#{@new_group_params.groupname}"
-            ) {
-                preferred_username,
-                type
-              }
-            }
-      """
-
-      res =
-        conn
-        |> auth_conn(user)
-        |> post("/api", AbsintheHelpers.mutation_skeleton(mutation))
-
-      assert hd(json_response(res, 200)["errors"])["message"] ==
                "A profile or group with that name already exists"
     end
+
+    test "doesn't creates a group if the username doesn't match the requirements",
+         %{conn: conn, user: user} do
+      res =
+        conn
+        |> auth_conn(user)
+        |> AbsintheHelpers.graphql_query(
+          query: @create_group_mutation,
+          variables: Map.put(@new_group_params, :preferredUsername, "no@way")
+        )
+
+      assert hd(res["errors"])["message"] == [
+               "Username must only contain alphanumeric lowercased characters and underscores."
+             ]
+    end
   end
 
   describe "list groups" do

test/graphql/resolvers/person_test.exs

@@ -13,23 +13,34 @@ defmodule Mobilizon.GraphQL.Resolvers.PersonTest do
 
   @non_existent_username "nonexistent"
 
-  describe "Person Resolver" do
+  @get_person_query """
-    @get_person_query """
+  query Person($id: ID!) {
-    query Person($id: ID!) {
+      person(id: $id) {
-        person(id: $id) {
+          preferredUsername,
-            preferredUsername,
-        }
       }
-    """
+    }
+  """
 
-    @fetch_person_query """
+  @fetch_person_query """
-    query FetchPerson($preferredUsername: String!) {
+  query FetchPerson($preferredUsername: String!) {
-        fetchPerson(preferredUsername: $preferredUsername) {
+      fetchPerson(preferredUsername: $preferredUsername) {
-            preferredUsername,
+          preferredUsername,
-        }
       }
-    """
+    }
+  """
 
+  @fetch_identities_query """
+  {
+    identities {
+      avatar {
+        url
+      },
+      preferredUsername,
+    }
+  }
+  """
+
+  describe "Getting a person" do
     test "get_person/3 returns a person by its username", %{conn: conn} do
       user = insert(:user)
       actor = insert(:actor, user: user)
@@ -128,107 +139,114 @@ defmodule Mobilizon.GraphQL.Resolvers.PersonTest do
 
       assert json_response(res, 200)["data"]["loggedPerson"]["avatar"]["url"] =~ Endpoint.url()
     end
+  end
 
-    test "create_person/3 creates a new identity", context do
+  describe "create_person/3" do
+    @create_person_mutation """
+    mutation CreatePerson(
+      $preferredUsername: String!
+      $name: String!
+      $summary: String
+      $avatar: MediaInput
+      $banner: MediaInput
+      ) {
+      createPerson(
+        preferredUsername: $preferredUsername
+        name: $name
+        summary: $summary
+        avatar: $avatar
+        banner: $banner
+      ) {
+        id
+        preferredUsername
+        avatar {
+          id,
+          url
+        },
+        banner {
+          id,
+          name,
+          url
+        }
+      }
+    }
+    """
+
+    test "creates a new identity", %{conn: conn} do
       user = insert(:user)
       actor = insert(:actor, user: user)
 
-      mutation = """
-          mutation {
-            createPerson(
-              preferredUsername: "new_identity",
-              name: "secret person",
-              summary: "no-one will know who I am"
-            ) {
-              id,
-              preferredUsername
-            }
-          }
-      """
-
       res =
-        context.conn
+        conn
-        |> post("/api", AbsintheHelpers.mutation_skeleton(mutation))
+        |> AbsintheHelpers.graphql_query(
+          query: @create_person_mutation,
+          variables: %{
+            preferredUsername: "new_identity",
+            name: "secret person",
+            summary: "no-one will know who I am"
+          }
+        )
 
-      assert json_response(res, 200)["data"]["createPerson"] == nil
+      assert res["data"]["createPerson"] == nil
 
-      assert hd(json_response(res, 200)["errors"])["message"] ==
+      assert hd(res["errors"])["message"] ==
                "You need to be logged in"
 
       res =
-        context.conn
+        conn
         |> auth_conn(user)
-        |> post("/api", AbsintheHelpers.mutation_skeleton(mutation))
+        |> AbsintheHelpers.graphql_query(
+          query: @create_person_mutation,
+          variables: %{
+            preferredUsername: "new_identity",
+            name: "secret person",
+            summary: "no-one will know who I am"
+          }
+        )
 
-      assert json_response(res, 200)["data"]["createPerson"]["preferredUsername"] ==
+      assert res["data"]["createPerson"]["preferredUsername"] ==
                "new_identity"
 
-      query = """
-      {
-          identities {
-            avatar {
-              url
-            },
-            preferredUsername,
-          }
-        }
-      """
-
       res =
-        context.conn
+        conn
-        |> get("/api", AbsintheHelpers.query_skeleton(query, "identities"))
+        |> AbsintheHelpers.graphql_query(query: @fetch_identities_query)
 
-      assert json_response(res, 200)["data"]["identities"] == nil
+      assert res["data"]["identities"] == nil
 
-      assert hd(json_response(res, 200)["errors"])["message"] ==
+      assert hd(res["errors"])["message"] ==
                "You need to be logged in"
 
       res =
-        context.conn
+        conn
         |> auth_conn(user)
-        |> get("/api", AbsintheHelpers.query_skeleton(query, "identities"))
+        |> AbsintheHelpers.graphql_query(query: @fetch_identities_query)
 
-      assert json_response(res, 200)["data"]["identities"]
+      assert res["data"]["identities"]
              |> Enum.map(fn identity -> Map.get(identity, "preferredUsername") end)
              |> MapSet.new() ==
                MapSet.new([actor.preferred_username, "new_identity"])
     end
 
-    test "create_person/3 with an avatar and an banner creates a new identity", context do
+    test "with an avatar and an banner creates a new identity", %{conn: conn} do
       user = insert(:user)
       insert(:actor, user: user)
 
-      mutation = """
+      variables = %{
-          mutation {
+        preferredUsername: "new_identity",
-            createPerson(
+        name: "secret person",
-              preferredUsername: "new_identity",
+        summary: "no-one will know who I am",
-              name: "secret person",
+        banner: %{
-              summary: "no-one will know who I am",
+          media: %{
-              banner: {
+            file: "landscape.jpg",
-                media: {
+            name: "irish landscape",
-                  file: "landscape.jpg",
+            alt: "The beautiful atlantic way"
-                  name: "irish landscape",
-                  alt: "The beautiful atlantic way"
-                }
-              }
-            ) {
-              id,
-              preferredUsername
-              avatar {
-                id,
-                url
-              },
-              banner {
-                id,
-                name,
-                url
-              }
-            }
           }
-      """
+        }
+      }
 
       map = %{
-        "query" => mutation,
+        "query" => @create_person_mutation,
+        "variables" => variables,
         "landscape.jpg" => %Plug.Upload{
           path: "test/fixtures/picture.png",
           filename: "landscape.jpg"
@@ -236,34 +254,63 @@ defmodule Mobilizon.GraphQL.Resolvers.PersonTest do
       }
 
       res =
-        context.conn
+        conn
         |> put_req_header("content-type", "multipart/form-data")
-        |> post("/api", map)
+        |> post(
+          "/api",
+          map
+        )
+        |> json_response(200)
 
-      assert json_response(res, 200)["data"]["createPerson"] == nil
+      assert res["data"]["createPerson"] == nil
 
-      assert hd(json_response(res, 200)["errors"])["message"] ==
+      assert hd(res["errors"])["message"] ==
                "You need to be logged in"
 
       res =
-        context.conn
+        conn
         |> auth_conn(user)
         |> put_req_header("content-type", "multipart/form-data")
-        |> post("/api", map)
+        |> post(
+          "/api",
+          map
+        )
+        |> json_response(200)
 
-      assert json_response(res, 200)["data"]["createPerson"]["preferredUsername"] ==
+      assert res["data"]["createPerson"]["preferredUsername"] ==
                "new_identity"
 
-      assert json_response(res, 200)["data"]["createPerson"]["banner"]["id"]
+      assert res["data"]["createPerson"]["banner"]["id"]
 
-      assert json_response(res, 200)["data"]["createPerson"]["banner"]["name"] ==
+      assert res["data"]["createPerson"]["banner"]["name"] ==
                "The beautiful atlantic way"
 
-      assert json_response(res, 200)["data"]["createPerson"]["banner"]["url"] =~
+      assert res["data"]["createPerson"]["banner"]["url"] =~
                Endpoint.url() <> "/media/"
     end
 
-    test "update_person/3 updates an existing identity", context do
+    test "with an username that is not acceptable", %{conn: conn} do
+      user = insert(:user)
+      _actor = insert(:actor, user: user)
+
+      res =
+        conn
+        |> auth_conn(user)
+        |> AbsintheHelpers.graphql_query(
+          query: @create_person_mutation,
+          variables: %{
+            preferredUsername: "me@no",
+            name: "wrong person"
+          }
+        )
+
+      assert hd(res["errors"])["message"] ==
+               ["Username must only contain alphanumeric lowercased characters and underscores."]
+    end
+  end
+
+  describe "update_person/3" do
+    test "updates an existing identity", context do
       user = insert(:user)
       %Actor{id: person_id} = insert(:actor, user: user, preferred_username: "riri")
 
@@ -333,7 +380,7 @@ defmodule Mobilizon.GraphQL.Resolvers.PersonTest do
       assert res_person["banner"]["url"] =~ Endpoint.url() <> "/media/"
     end
 
-    test "update_person/3 should fail to update a not owned identity", context do
+    test "should fail to update a not owned identity", context do
       user1 = insert(:user)
       user2 = insert(:user)
       %Actor{id: person_id} = insert(:actor, user: user2, preferred_username: "riri")
@@ -360,7 +407,7 @@ defmodule Mobilizon.GraphQL.Resolvers.PersonTest do
                "Profile is not owned by authenticated user"
     end
 
-    test "update_person/3 should fail to update a not existing identity", context do
+    test "should fail to update a not existing identity", context do
       user = insert(:user)
       insert(:actor, user: user, preferred_username: "riri")
 
@@ -385,8 +432,10 @@ defmodule Mobilizon.GraphQL.Resolvers.PersonTest do
       assert hd(json_response(res, 200)["errors"])["message"] ==
                "Profile not found"
     end
+  end
 
-    test "delete_person/3 should fail to update a not owned identity", context do
+  describe "delete_person/3" do
+    test "should fail to update a not owned identity", context do
       user1 = insert(:user)
       user2 = insert(:user)
       %Actor{id: person_id} = insert(:actor, user: user2, preferred_username: "riri")
@@ -410,7 +459,7 @@ defmodule Mobilizon.GraphQL.Resolvers.PersonTest do
                "Profile is not owned by authenticated user"
     end
 
-    test "delete_person/3 should fail to delete a not existing identity", context do
+    test "should fail to delete a not existing identity", context do
       user = insert(:user)
       insert(:actor, user: user, preferred_username: "riri")
 
@@ -433,7 +482,7 @@ defmodule Mobilizon.GraphQL.Resolvers.PersonTest do
                "Profile not found"
     end
 
-    test "delete_person/3 should fail to delete the last user identity", context do
+    test "should fail to delete the last user identity", context do
       user = insert(:user)
       %Actor{id: person_id} = insert(:actor, user: user, preferred_username: "riri")
 
@@ -456,7 +505,7 @@ defmodule Mobilizon.GraphQL.Resolvers.PersonTest do
                "Cannot remove the last identity of a user"
     end
 
-    test "delete_person/3 should fail to delete an identity that is the last admin of a group",
+    test "should fail to delete an identity that is the last admin of a group",
          context do
       group = insert(:group)
       classic_user = insert(:user)
@@ -488,7 +537,7 @@ defmodule Mobilizon.GraphQL.Resolvers.PersonTest do
                "Cannot remove the last administrator of a group"
     end
 
-    test "delete_person/3 should delete an actor identity", context do
+    test "should delete an actor identity", context do
       user = insert(:user)
       %Actor{id: person_id} = insert(:actor, user: user, preferred_username: "riri")
       insert(:actor, user: user, preferred_username: "fifi")

test/mobilizon/actors/actors_test.exs

@@ -25,7 +25,7 @@ defmodule Mobilizon.ActorsTest do
       suspended: true,
       uri: "some uri",
       url: "some url",
-      preferred_username: "some username"
+      preferred_username: "some_username"
     }
     @update_attrs %{
       summary: "some updated description",
@@ -35,7 +35,7 @@ defmodule Mobilizon.ActorsTest do
       suspended: false,
       uri: "some updated uri",
       url: "some updated url",
-      preferred_username: "some updated username"
+      preferred_username: "some_updated_username"
     }
     @invalid_attrs %{
       summary: nil,
@@ -234,7 +234,7 @@ defmodule Mobilizon.ActorsTest do
       assert actor.domain == "some domain"
       assert actor.keys == "some keypair"
       assert actor.suspended
-      assert actor.preferred_username == "some username"
+      assert actor.preferred_username == "some_username"
     end
 
     test "create_actor/1 with empty data returns error changeset" do
@@ -381,13 +381,13 @@ defmodule Mobilizon.ActorsTest do
     @valid_attrs %{
       summary: "some description",
       suspended: true,
-      preferred_username: "some-title",
+      preferred_username: "some_title",
       name: "Some Title"
     }
     @update_attrs %{
       summary: "some updated description",
       suspended: false,
-      preferred_username: "some-updated-title",
+      preferred_username: "some_updated_title",
       name: "Some Updated Title"
     }
     @invalid_attrs %{summary: nil, suspended: nil, preferred_username: nil, name: nil}
@@ -400,7 +400,7 @@ defmodule Mobilizon.ActorsTest do
 
       assert group.summary == "some description"
       refute group.suspended
-      assert group.preferred_username == "some-title"
+      assert group.preferred_username == "some_title"
     end
 
     test "create_group/1 with an existing profile username fails" do