Remove ja_resource and canary reliance from UserController #882

snewcomer · 2017-08-31T13:28:24Z

What's in this PR?

[WIP]
Remove ja_resource from User Controller

References

Fixes #882

Progress on: #864

snewcomer · 2017-09-01T14:29:50Z

lib/code_corps/policy/policy.ex

    def can?(%User{}, _action, nil), do: true
-
    def can?(%User{} = current_user, :update, %User{} = user), do: Policy.User.update?(user, current_user)


NOTE: had to leave these in here b/c other tests not in the user_controller_test were depending on this

I understand why line 44 needs to stay, but are you sure about 45? Which tests fail if you remove it? Assuming you add the "fixes" i described below.

snewcomer · 2017-09-01T14:37:13Z

lib/code_corps_web/controllers/user_controller.ex

-  defp login(conn, _opts) do
-    Plug.Conn.register_before_send(conn, &do_login(&1))
-  end
+  # defp login(conn, _opts) do


@begedin any idea why commenting these lines (and the plug :login) above out allows for a passing test suite? Otherwise I get this line and this line failing

I'm not sure, but I would expect the plug to cause things to fail either way. It was added mainly to get around some ja_resource quirks.

What I would do is remove plug :login when action in [:create], as well as defp login and defp do_login completely.

Then, in create/2, I would add a |> Guardian.Plug.sign_in(user) between conn and |> put_status(:created), or really, anywhere in that chain.

That would be the most explicit and easiest to read approach for dealing with it anyway.

@begedin is this b/c the user is signed in automatically after creating a user?

begedin

This generally looks good. Ping me for another review when you try the advice I gave.

snewcomer · 2017-09-05T15:00:49Z

lib/code_corps_web/controllers/user_controller.ex

+  def create(%Conn{} = conn, %{} = params) do
+    with {:ok, %User{} = user} <- %User{} |> User.registration_changeset(params) |> Repo.insert
+    do
+      conn |> Plug.Conn.fetch_session |> Guardian.Plug.sign_in(user) |> put_status(:created) |> render("show.json-api", data: user)


@begedin so this worked.

Although a bit lost on why this is needed as compared to before these changes.

The login function was a plug. It automatically ran before performing the create action and told the conn that, after performing create, it needs to run the do_login function.

The do_login function then signed the user into the conn if the create action was successful.

Basically, from a user's perspective, "i want to be signed in when I register".

We had to do it via a plug, due to some quirks with canary.

Now that canary is gone, we can do the more direct route, so we just do it as part of the action itself.

If you mean the fetch_session part, that should not be needed. What happens if you remove it?

@snewcomer Strange, but it makes sense.

I think you actually found a bug in the old login function. We were just calling Conn.assign instead of actually formally signing the user in.

Additionally, it looks like our client is not expecting to sign the user in automatically upon registration and instead does it manually.

Considering these two, I think the simplest thing we can do is outright remove both fetch_session and sign_in.

Simply put, the code should read conn |> put_status(:created) |> render("show.json-api", data: user)and we can leave it that that.

Sorry to make go through all of this, but hey, we discovered a bug and a piece of code that does nothing along the way!

begedin · 2017-09-06T05:44:21Z

lib/code_corps_web/controllers/user_controller.ex

+  def create(%Conn{} = conn, %{} = params) do
+    with {:ok, %User{} = user} <- %User{} |> User.registration_changeset(params) |> Repo.insert
+    do
+      conn |> Plug.Conn.fetch_session |> Guardian.Plug.sign_in(user) |> put_status(:created) |> render("show.json-api", data: user)


@snewcomer Strange, but it makes sense.

I think you actually found a bug in the old login function. We were just calling Conn.assign instead of actually formally signing the user in.

Additionally, it looks like our client is not expecting to sign the user in automatically upon registration and instead does it manually.

Considering these two, I think the simplest thing we can do is outright remove both fetch_session and sign_in.

Simply put, the code should read conn |> put_status(:created) |> render("show.json-api", data: user)and we can leave it that that.

Sorry to make go through all of this, but hey, we discovered a bug and a piece of code that does nothing along the way!

begedin · 2017-09-07T07:18:51Z

lib/code_corps/policy/policy.ex

@@ -30,6 +30,8 @@ defmodule CodeCorps.Policy do
  defp can?(%User{} = user, :update, %Comment{} = comment, %{}), do: Policy.Comment.update?(user, comment)
  defp can?(%User{} = user, :create, %Organization{}, %{}), do: Policy.Organization.create?(user)
  defp can?(%User{} = user, :update, %Organization{} = organization, %{}), do: Policy.Organization.update?(user, organization)  
+  defp can?(%User{} = current_user, :update, %User{} = user, %{}), do: Policy.User.update?(user, current_user)
+  defp can?(%User{}, _action, %User{} = _user, %{}), do: true


Remind me again, why is this one needed? It seems unsafe, as it would technically allow any user to perform any action on any other user.

If you meant to copy-paste the one from line 46 below, then the third argument would be nil, but that means you'd also have to update the spec to allow nil, not just User.t as the third argument.

However, I don't think the catch all from below is strictly necessary when not using canary, so could you double check?

Yep my bad. Good call!

begedin · 2017-09-07T15:15:33Z

Hey @snewcomer. I merged! Excellent work.

joshsmith · 2017-09-07T16:34:53Z

🙌 I love seeing things like this happening when I'm not even looking. Great work @snewcomer!

…s#882) * Removed ja_resource and canary reliance from UserController * Fixed bug

snewcomer added the in progress label Aug 31, 2017

snewcomer self-assigned this Aug 31, 2017

snewcomer added Difficulty: Easy Priority: Medium Size: S labels Aug 31, 2017

begedin mentioned this pull request Sep 1, 2017

Remove ja_resource #864

Closed

snewcomer commented Sep 1, 2017

View reviewed changes

snewcomer force-pushed the user-controller-no-jaresource branch from 8ab2ada to 2db3676 Compare September 1, 2017 14:51

joshsmith added needs code review question labels Sep 4, 2017

joshsmith requested a review from begedin September 4, 2017 01:56

begedin requested changes Sep 4, 2017

View reviewed changes

begedin mentioned this pull request Sep 4, 2017

Switch UserController away from ja_resource and canary #913

Closed

snewcomer added 8 commits September 5, 2017 07:50

[USER CONTROLLER]: remove ja_resource

69867ed

move all user controller actions away from ja_resource

4b83aad

almost passing user tests

50fdc3f

passing user controller tests

774f14f

add back user policy to can defimpl

3c38037

passing tests by removing login controller action

c57c061

remove uneeded User policy check

af94989

add Guardian sign_in

1b56985

snewcomer force-pushed the user-controller-no-jaresource branch from 7187697 to 1b56985 Compare September 5, 2017 14:54

snewcomer commented Sep 5, 2017

View reviewed changes

begedin requested changes Sep 6, 2017

View reviewed changes

remove sign_in function

6fa41b4

snewcomer changed the title ~~[WIP] [USER CONTROLLER]: remove ja_resource~~ [WIP] [USER CONTROLLER]: remove ja_resource close #913 Sep 6, 2017

begedin reviewed Sep 7, 2017

View reviewed changes

update policy by removing uneeded default true User policy

4f4d0e7

begedin approved these changes Sep 7, 2017

View reviewed changes

begedin changed the title ~~[WIP] [USER CONTROLLER]: remove ja_resource close #913~~ Remove ja_resource and canary reliance from UserController Sep 7, 2017

begedin merged commit c579fb7 into develop Sep 7, 2017

begedin deleted the user-controller-no-jaresource branch September 7, 2017 15:15

untra pushed a commit to untra/code-corps-api that referenced this pull request Sep 16, 2017

Remove ja_resource and canary reliance from UserController (code-corp…

17a250a

…s#882) * Removed ja_resource and canary reliance from UserController * Fixed bug

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove ja_resource and canary reliance from UserController #882

Remove ja_resource and canary reliance from UserController #882

snewcomer commented Aug 31, 2017 •

edited by begedin

snewcomer Sep 1, 2017

begedin Sep 4, 2017

snewcomer Sep 1, 2017

begedin Sep 4, 2017

snewcomer Sep 5, 2017

begedin left a comment

snewcomer Sep 5, 2017

snewcomer Sep 5, 2017

begedin Sep 5, 2017

begedin Sep 5, 2017

snewcomer Sep 5, 2017

begedin Sep 6, 2017

begedin Sep 6, 2017

begedin Sep 7, 2017

snewcomer Sep 7, 2017

begedin commented Sep 7, 2017

joshsmith commented Sep 7, 2017

		def can?(%User{}, _action, nil), do: true

		def can?(%User{} = current_user, :update, %User{} = user), do: Policy.User.update?(user, current_user)

Remove ja_resource and canary reliance from UserController #882

Remove ja_resource and canary reliance from UserController #882

Conversation

snewcomer commented Aug 31, 2017 • edited by begedin

What's in this PR?

References

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

begedin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

begedin commented Sep 7, 2017

joshsmith commented Sep 7, 2017

snewcomer commented Aug 31, 2017 •

edited by begedin