From 7a6a013d93c685559ad5aebea9b2870ffeeb81ad Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Fri, 6 May 2022 19:30:52 +0200 Subject: [PATCH] Make sure users can't create profiles or groups with non-valid patterns Closes #1068 Signed-off-by: Thomas Citharel --- lib/mobilizon/actors/actor.ex | 20 ++ test/graphql/resolvers/group_test.exs | 98 ++++++---- test/graphql/resolvers/person_test.exs | 247 +++++++++++++++---------- test/mobilizon/actors/actors_test.exs | 12 +- 4 files changed, 236 insertions(+), 141 deletions(-) diff --git a/lib/mobilizon/actors/actor.ex b/lib/mobilizon/actors/actor.ex index 4320d3562..21ee6eaec 100644 --- a/lib/mobilizon/actors/actor.ex +++ b/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) diff --git a/test/graphql/resolvers/group_test.exs b/test/graphql/resolvers/group_test.exs index 6fa9944ba..225b8f842 100644 --- a/test/graphql/resolvers/group_test.exs +++ b/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 - test "create_group/3 creates a group and check a group with this name does not already exist", + describe "create_group/3" do + @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 = """ - mutation { - createGroup( - preferred_username: "#{@new_group_params.groupname}" - ) { - preferred_username, - type - } - } - """ + res = + conn + |> auth_conn(user) + |> AbsintheHelpers.graphql_query( + query: @create_group_mutation, + variables: @new_group_params + ) + + 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"] == - @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"] == + assert hd(res["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 diff --git a/test/graphql/resolvers/person_test.exs b/test/graphql/resolvers/person_test.exs index 173e45b1a..dabdf3dfb 100644 --- a/test/graphql/resolvers/person_test.exs +++ b/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 """ - query Person($id: ID!) { - person(id: $id) { - preferredUsername, - } + @get_person_query """ + query Person($id: ID!) { + person(id: $id) { + preferredUsername, } - """ + } + """ - @fetch_person_query """ - query FetchPerson($preferredUsername: String!) { - fetchPerson(preferredUsername: $preferredUsername) { - preferredUsername, - } + @fetch_person_query """ + query FetchPerson($preferredUsername: String!) { + fetchPerson(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 - |> post("/api", AbsintheHelpers.mutation_skeleton(mutation)) + conn + |> 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 - |> get("/api", AbsintheHelpers.query_skeleton(query, "identities")) + conn + |> 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 = """ - mutation { - createPerson( - preferredUsername: "new_identity", - name: "secret person", - summary: "no-one will know who I am", - banner: { - media: { - file: "landscape.jpg", - name: "irish landscape", - alt: "The beautiful atlantic way" - } - } - ) { - id, - preferredUsername - avatar { - id, - url - }, - banner { - id, - name, - url - } - } + variables = %{ + preferredUsername: "new_identity", + name: "secret person", + summary: "no-one will know who I am", + banner: %{ + media: %{ + file: "landscape.jpg", + name: "irish landscape", + alt: "The beautiful atlantic way" } - """ + } + } 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") diff --git a/test/mobilizon/actors/actors_test.exs b/test/mobilizon/actors/actors_test.exs index 423b5c758..4ddfc333a 100644 --- a/test/mobilizon/actors/actors_test.exs +++ b/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