Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unable to get value from conn.assigns using Guardian.Plug.claims #55

Closed
Lbatson opened this issue Oct 29, 2015 · 6 comments · Fixed by #58
Closed

Unable to get value from conn.assigns using Guardian.Plug.claims #55

Lbatson opened this issue Oct 29, 2015 · 6 comments · Fixed by #58

Comments

@Lbatson
Copy link

Lbatson commented Oct 29, 2015

I don't think checking the value in conn.assigns dict returns a tuple. all the documentation i can find points now to returning just the value or nil. Since Guardian.Plug.claims is checking for this tuple, https://github.com/hassox/guardian/blob/master/lib/guardian/plug.ex#L161, it's always failing for me with the :no_session error case. This breaks me checking claims and the various plugs like EnsureAuthenticated that use it. I'm using Plug v1.0.2 and I noticed the test suite is set up for v1.0.0 but i can't see the difference in return values for it between the versions. Is there something I'm missing?

@doomspork
Copy link
Member

@Lbatson I'm using Plug 1.0.2 and Guardian 0.6.2 together without issue.

As for the tuple, I'm pretty sure Guardian assigns that value. See plug.ex#L139 & plug_test.exs#L13

@Lbatson
Copy link
Author

Lbatson commented Oct 29, 2015

@doomspork it still fails for me. are you using Guardian.Plug.claims anywhere? I always end up getting the :no_session error even though i can see the claims in then conn itself. I did fix the issue with it failing on EnsureAuthenticated which had an issue with my on_verify hook failing, however I've noticed that Guardian.Plug.VerifyHeader also always fails with :no_session as well but then verifies the token and adds it. https://github.com/hassox/guardian/blob/master/lib/guardian/plug/verify_header.ex#L49

Also, not sure if it's relevant or not but i'm using Erlang 18 and Elixir 1.1.1

@Lbatson
Copy link
Author

Lbatson commented Oct 30, 2015

Ok, think i've found the issue. The way VerifyHeader and api_sign_in set claims is different. VerifyHeader attaches a tuple, https://github.com/hassox/guardian/blob/master/lib/guardian/plug/verify_header.ex#L63, where api_sign_in simply adds the claims, https://github.com/hassox/guardian/blob/master/lib/guardian/plug.ex#L171. This means if you call Guardian.Plug.claims on the conn after api_sign_in, for instance in the after_sign_in hook, it will fail as Guardian.Plug.claims looks for the tuple, doesn't get it and gives the :no_session error. I think to fix this it should just be one or the other wins out, setting as a tuple or just the claims. Not sure what approach is wanted or what else it might affect yet but fixing the inconsistency would solve this.

@doomspork
Copy link
Member

Hey @Lbatson I'm also on Erlang 18 and Elixir 1.1.1. Unless I'm reading the @spec wrong the claims are always a tuple. If you look at the method you linked to in Plug you'll see the spec define the claims parameter as being a tuple:

@spec set_claims(Plug.Conn.t, { :ok, Map }, atom) :: Plug.Conn.t

I suspect there might be some other configuration you're missing. Is your project on GitHub? I recently updated the Guardian sample project to the latest dependencies, have you looked at that? My changes are in a PR here: phoenix_guardian/pull/5

@Lbatson
Copy link
Author

Lbatson commented Oct 30, 2015

The @spec does shows the tuple, however the parameter passed to set_claims from api_sign_in is just the Map. Maybe it should be changed to |> set_claims({:ok, full_claims}, the_key) or instead of setting a tuple to the claims just set the claims themselves and modify the other portions that use it with the tuple?

@doomspork
Copy link
Member

@Lbatson looks like you're right. I've been using Guardian.decode_and_verify(jwt) which the Guardian tests use and that seems to work. Once I updated my code to use Guardian.Plug.claims I was able to repo your issue. While I was looking into the api_sign_in I found another issue so I'll open a PR for both.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants