Added missing logout route

[pmyjavec]

Hey,

I'm just learning Phoenix + Elixir so sorry this is not the most DRY code.

Fixes trenpixster/addict/#82

[jaimeiniesta]

I think that it's better to not implement the logout via GET, as it's a destructive action, think of link prefetchers that might log you out. It should go through DELETE (POST under the hood).

So, to fix #82 I would change the routes, and remove the definition of the logout via GET, instead of implementing it.

[pmyjavec]

I did think about this later and agree. I'll amend the PR and get it to you.

Thanks :)

[trenpixster]

Thanks @pmyjavec! I'm with @jaimeiniesta on this one for security reasons :)

[pmyjavec]

Hey guys, wondering what you think of this solution?

I made /logout respond to DELETE requests , I kept POST requests for backwards compatibility. Maybe /logout should only respond to DELETE requests eventually ?

What do you think?

Cheers

[trenpixster]

Great! Regarding the DELETE, maybe if we had the endpoint as /session it would make more sense in my head, given the REST approach. That said, probably addict should also create a session via a POST /session and rethink the other paths? What do you guys think?

[pmyjavec]

Sounds good; however, I think we could do that work on a seperate feature so as not to overload this one?

Having /logout as a DELETE still makes good sense to me, it's much more reflective of what it actually does.

[jaimeiniesta]

@pmyjavec if you keep the POST route, then keep the test for it as well. I tried but now it fails because there is no matching clause in the controller.

Said that, I would not keep the POST for delete. Reason being is, when you write a link with method: :delete, like this:

<%= link "Delete", to: post_path(@conn, :delete, post), method: :delete %>

Phoenix will generate a form that will go via POST, and it will also include a hidden field _method with the value delete. Later, this will be converted internally to a DELETE request. That's because not every browser implements DELETE:

<form action="/posts/1" method="post">
  <input name="_method" type="hidden" value="delete">
  <input name="_csrf_token" type="hidden" value="...">
  <a data-confirm="Are you sure?" href="#" rel="nofollow">Delete</a>
</form>

So, I don't know, maybe supporting POST for logging out is OK for a while, but I would deprecate this as soon as possible.

[jaimeiniesta]

@trenpixster totally, login and logout can be seen as creating a session (POST), and destroying a session (DELETE).

[pmyjavec]

@jaimeiniesta,

Said that, I would not keep the POST for delete. Reason being is, when you write a link with method: :delete, like this:

Sorry, did you mean to say, just remove the POST method for the /logout?

If I understand correctly you mean that even though we specify a DELETE method, Phoenix converts it to a POST for us anyway ?

[pmyjavec]

I've removed the POST for now, let me know if you guys want it back in with tests and the updated controller and I will do that.

Pretty keen to get this merged so I can use it ;)

[jaimeiniesta]

@pmyjavec yes, I suggest to remove the POST and only keep DELETE for the logout - Phoenix, like Rails, will convert links with method: :delete to forms via POST and a hidden attribute for the method, which will in its turn be converted back to DELETE.

[pmyjavec]

Ok, cool :)

That's now been done, the PR has been updated.

[pmyjavec]

@jaimeiniesta, My bad, I forgot to add the change to my amendment, I also changed the order of methods so they're all the same, :get, :post

[jaimeiniesta]

:+1: LGTM.

[trenpixster]

Awesome, thanks guys! I'll create an issue in the mean time to be more REST'ish 😄 👍