Users without last names cannot execute triggers!

[christophermaier]

Validation is too strict; executing a trigger as a user without a last name silently fails, even though the same user may execute commands in chat with no problem.

Replication Steps

1. Create a user without a last name.

cogctl users create \
       --username=chris \
       --first-name=Chris \
       [email protected] \
       --password=password 

cogctl chat-handles create \
       --user=chris \
       --chat-provider=slack \
       --handle=chris

2. Create a trigger to execute as that user

cogctl triggers create \
       --name test-trigger \
       --pipeline "echo 'Hello World'" \
       --as-user=chris

3. Execute the trigger

curl --request POST
     --verbose
     --header "Content-Type: application/json"
     http://localhost:4001/v1/triggers/dad68d88-37a0-46c4-89bf-e041404a375c
*   Trying 127.0.0.1...
* Connected to localhost (127.0.0.1) port 4001 (#0)
> POST /v1/triggers/dad68d88-37a0-46c4-89bf-e041404a375c HTTP/1.1
> Host: localhost:4001
> User-Agent: curl/7.43.0
> Accept: */*
> Content-Type: application/json
>
< HTTP/1.1 202 Accepted
< server: Cowboy
< date: Wed, 14 Dec 2016 01:36:02 GMT
< content-length: 107
< content-type: application/json; charset=utf-8
< cache-control: max-age=0, private, must-revalidate
< x-request-id: siasugtdh097fi05pjsaj892ishkuv8n
<
* Connection #0 to host localhost left intact
{"status":"Request accepted and still processing after 30 seconds","id":"2162123ec59842d7b6047a0bd61e6618"}%

The request processing times out, returning a 202 to the requestor. However, the server logs reveal the following:

2016-12-14T01:35:35.756 module=Cog.Chat.Adapter line=251 [error] Error decoding chat message: {:error, %Conduit.ValidationError{errors: [%Conduit.FieldTypeError{errors: {:error, %Conduit.ValidationError{errors: [%Conduit.FieldPropertyError{field: :last_name, property: :required, value: nil}], message: nil, value: %Cog.Chat.User{email: nil, first_name: "Chris", handle: "chris", id: "chris", last_name: nil, mention_name: nil, provider: "http"}}}, field: :user, type: Cog.Chat.User, value: %Cog.Chat.User{email: nil, first_name: "Chris", handle: "chris", id: "chris", last_name: nil, mention_name: nil, provider: "http"}}], message: nil, value: %Cog.Chat.Message{bot_name: nil, edited: nil, id: "2162123ec59842d7b6047a0bd61e6618", initial_context: %{"body" => %{}, "headers" => %{"accept" => "*/*", "content-type" => "application/json", "host" => "localhost:4001", "user-agent" => "curl/7.43.0"}, "query_params" => %{}, "raw_body" => "", "trigger_id" => "dad68d88-37a0-46c4-89bf-e041404a375c"}, provider: "http", room: %Cog.Chat.Room{id: "2162123ec59842d7b6047a0bd61e6618", is_dm: true, name: "direct", provider: "http", secondary_id: nil}, text: "echo 'Hello World'", user: %Cog.Chat.User{email: nil, first_name: "Chris", handle: "chris", id: "chris", last_name: nil, mention_name: nil, provider: "http"}}}}   %{"bot_name" => nil, "edited" => nil,
  "id" => "2162123ec59842d7b6047a0bd61e6618",
  "initial_context" => %{"body" => %{},
    "headers" => %{"accept" => "*/*", "content-type" => "application/json",
      "host" => "localhost:4001", "user-agent" => "curl/7.43.0"},
    "query_params" => %{}, "raw_body" => "",
    "trigger_id" => "dad68d88-37a0-46c4-89bf-e041404a375c"},
  "provider" => "http",
  "room" => %{"id" => "2162123ec59842d7b6047a0bd61e6618", "is_dm" => true,
    "name" => "direct", "provider" => "http", "secondary_id" => nil},
  "text" => "echo 'Hello World'",
  "user" => %{"email" => nil, "first_name" => "Chris", "handle" => "chris",
    "id" => "chris", "last_name" => nil, "mention_name" => nil,
    "provider" => "http"}}

Here, we see that the last name is required. Updating the user to add a last name an re-executing the trigger shows it working as expected.

cogctl users update chris --last-name=Maier

curl --request POST
     --verbose
     --header "Content-Type: application/json"
     http://localhost:4001/v1/triggers/dad68d88-37a0-46c4-89bf-e041404a375c
*   Trying 127.0.0.1...
* Connected to localhost (127.0.0.1) port 4001 (#0)
> POST /v1/triggers/dad68d88-37a0-46c4-89bf-e041404a375c HTTP/1.1
> Host: localhost:4001
> User-Agent: curl/7.43.0
> Accept: */*
> Content-Type: application/json
>
< HTTP/1.1 200 OK
< server: Cowboy
< date: Wed, 14 Dec 2016 01:38:18 GMT
< content-length: 26
< content-type: application/json; charset=utf-8
< cache-control: max-age=0, private, must-revalidate
< x-request-id: ss13j5lu48bv6sjph6suoqhs4v2cukk3
<
* Connection #0 to host localhost left intact
[{"body":["Hello World"]}]%

Cause

The error is encountered in Cog.Chat.Adapter. Because the message does not validate, we treat it as invalid and do no further processing on it. However, since it has come in via a trigger, it's already in the system; if it was valid enough at the outside of the system, it should be valid deeper inside.

The fact that a user without a last name can still execute commands via chat indicates that there are two different paths that requests are taking. If a user can execute a command, they should be able to execute it regardless of how it is called.

Thoughts

1. Why is a last name required to execute a command via trigger? Is there some place where we are assuming the presence of a last name, perhaps for messaging or logging purposes? Can that be replaced with something else?
2. How many places are relying on validation by the Cog.Chat.User struct? Do any of those require a last name? Can these uses be reconciled?
3. Which other validations are in excess of what is strictly required? This ticket is in terms of "last name", but only because that happens to be how it was found; there could be others.
4. Is there any circumstance in which a trigger execution can be submitted to the system and legitimately fail at this point? If so, how can that be conveyed back to the requestor as a failure? This should never return an HTTP 202 to the user.
5. If we do in fact require a last name (or any other data, for that matter), then this should be reflected in the creation and update validation logic on our users. I should not be able to create a user in Cog that cannot actually use Cog.