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

Add Guardian.Plug.Backdoor #354

Closed
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@jayjun
Copy link

jayjun commented Jul 10, 2017

Takes over from #120 by rebasing to latest master.

This pull request also adds the type option to set token type, and new_claims to inject claims into Guardian.Plug.sign_in/4.

Ping @aaronrenner @doomspork.

aaronrenner and others added some commits Feb 17, 2016

Add Guardian.Plug.Backdoor
This plug helps developers write faster acceptance tests, by not requiring to go through the entire sign in process for each test. This feature is inspired by Clearance’s backdoor: https://github.com/thoughtbot/clearance#fast-feature-specs

Updated the Changelog.

Fixes #90.
@doomspork

This comment has been minimized.

Copy link
Member

doomspork commented Jul 10, 2017

This is fantastic @jayjun! @hassox will these changes impact your v1 changes?

@hassox

This comment has been minimized.

Copy link
Member

hassox commented Jul 10, 2017

It will but that's ok.

I'm not sure that trying to construct all the different parts of a token via the URL is a great way to go though. There are lots of different variations that will be possible in V1 and I'm concerned that trying to construct a token from url params will become tricky at best but impossible in some cases. What about cases where a different secret is used depending on the request etc. I'm also a little concerned about setting the new_claims when the plug is initialized since depending on how you do it it might not be appropriate for all tests but could be set once.

What would you folks think of putting a token into the URL rather than the components? This would let the token contain exactly what you need, be signed by the correct secret (for those using custom secrets) and in V1 allow you to specify exactly which token type you're intending on using.

{:ok, token, _} = Guardin.encode_and_sign(user, :access, new_claims)
navitage_to "/?access_token=#{token}"
@jayjun

This comment has been minimized.

Copy link
Author

jayjun commented Jul 10, 2017

I like it, for all the reasons you mentioned. Though I also like modifying User:X directly in the browser when debugging.

I don’t know what token variations are possible in v1, but even passing token components through the URL should still result in calling Guardian.encode_and_sign/3 anyway.

I suggest we go ahead with your suggestion first. If and when we implement constructing a token via URL, it will be a layer added on this work.

@hassox

This comment has been minimized.

Copy link
Member

hassox commented Jul 10, 2017

Cool... Another thought is that this is really not good to let escape by accident into environments other than dev and test. Can you put a guard on the call so that we don't do this unless it's the correct environment?

TBH this plug makes me a little nervous :\

import Plug.Conn

@doc false
def init(opts \\ []) do

This comment has been minimized.

@doomspork

doomspork Jul 10, 2017

Member

I concur with @hassox's suggestion, how about we add something like this to the init function:

unless Mix.env == :test, do: raise "Guardian.Plug.Backdoor should only be used in test!"

This comment has been minimized.

@doomspork

doomspork Jul 10, 2017

Member

@hassox I don't even see a need for this in :dev, do you? I can see someone mistakenly deploying their code without setting MIX_ENV=prod and this would leak. Thoughts?

This comment has been minimized.

@hassox

hassox Jul 10, 2017

Member

Agreed... The whole thing makes me a bit nervous but I understand the need.

@jayjun Could I also ask that we improve the naming of this so the it's abundantly clear that it's only for testing? Something like Guardian.Test.Plug.Backdoor or something?

@odarriba

This comment has been minimized.

Copy link

odarriba commented Jul 10, 2017

I'm not sure where this approach come from, so maybe what I'm going to propose doesn't fit for the needs.

If it's only purpose is testing, what about just creating a test helper to add the session needed for authentication? I've seen this pattern before in other libs and seems less intrusive (you don't have to include a if in your plug pipeline) and you authenticate the user clearly in the test.

What do you think about it?

PS. You are doing a great job in this release mates!

@jayjun

This comment has been minimized.

Copy link
Author

jayjun commented Jul 11, 2017

@odarriba It’s inspired by Clearance‘s backdoor for faster tests, and Mislav‘s backdoor hack for faster edit-compile-run cycles.

If I‘m not wrong, a test session can be added in just one call to Guardian.Plug.sign_in/4 without a test helper. That is what this backdoor essentially wraps.

Guardian.Plug.Backdoor is a plug so tests or users can bypass authentication plugs by appending ”magic parameters” to URLs. Alternative tokens can be tested really quickly.

navigate_to "/?as=User:5"        # assert success
navigate_to "/admin/?as=User:5"  # assert failure
navigate_to "/admin/?as=Admin:1" # assert success

As for raising, we’re not supposed to use Mix.env/0 during runtime. Maybe we swap the backdoor for no-op outside test environments?

I’m also happy to scrap this whole idea if risks outweigh benefits.

@odarriba

This comment has been minimized.

Copy link

odarriba commented Jul 11, 2017

If there is protection against using it in prod environments, I don't see any issues then!

@cybrox

This comment has been minimized.

Copy link

cybrox commented Jul 14, 2017

@odarriba With 7f07ab8, any chance of this bleeding into production is eliminated at compile time.

Really looking forward to seeing this merged 👍

@ocisly

This comment has been minimized.

Copy link

ocisly commented Jul 23, 2017

Hello folks, I am in no way involved with this project and came across this PR purely by chance.
However, I couldn't help but notice that you are blacklisting Mix.env() == :prod rather than whitelisting dev and test. So what happens if someone has a custom MIX_ENV, e.g. MIX_ENV=staging or, more worryingly, MIX_ENV=production?

@jayjun

This comment has been minimized.

Copy link
Author

jayjun commented Jul 23, 2017

@codesparkle Excellent point! Changed to whitelist :dev and :test rather than blacklist :prod.

@hassox

This comment has been minimized.

Copy link
Member

hassox commented Jul 23, 2017

@jayjun I've been thinking about this behaviour a lot over the last couple of weeks.

I'm not sure that this belongs in Guardian itself. The challenge I have is that it's very focused on a particular use case that - whilst valid - also provides a way to severely compromise security if it's used incorrectly.

I'm hearing the need and like that there is a solution being developed but I'm thinking that a community library would probably be more appropriate.

@jayjun @scrogson @doomspork thoughts?

@doomspork

This comment has been minimized.

Copy link
Member

doomspork commented Jul 23, 2017

@hassox I think that's a very valid concern. We could put this code in the org under a Guardian.Test package and look to include some additional test helpers. Then you could take security a step further by ensuring the dependency is limited to test.

@jayjun

This comment has been minimized.

Copy link
Author

jayjun commented Jul 24, 2017

@hassox @doomspork In hindsight, I absolutely agree that this should be a separate community package, either in Guardian.Test or by itself. That way, it can be excluded from production like so,

{:guardian_backdoor, ">= 0.0.0", only: [:test, :dev]}

As for restricting it to tests, I must add that backdoors have been invaluable for rapid development. In fact, not providing a community-vetted package may encourage others to write their own, which could cause more harm.

Having the backdoor in its own package has the benefit of allowing anyone to use it in tests only, or in development too.

@doomspork

This comment has been minimized.

Copy link
Member

doomspork commented Jul 24, 2017

@jayjun / @hassox — if we can agree on a package name I'll get the repo put up and the initial project kicked off.

@scrogson

This comment has been minimized.

Copy link
Member

scrogson commented Jul 24, 2017

Just so I understand...the point of this is to make it easier to build integration tests with things like Hound and Wallaby? I actually haven't used either of those testing libraries, so I need to educate myself there.

@hassox

This comment has been minimized.

Copy link
Member

hassox commented Jul 24, 2017

@doomspork I don't have a strong opinion on a name. I think it should be focused on testing though ;)

@doomspork

This comment has been minimized.

Copy link
Member

doomspork commented Jul 24, 2017

@scrogson the original PR (and this one) are based on the ideas shared in this ThoughtBot article:

https://github.com/thoughtbot/clearance#fast-feature-specs
https://robots.thoughtbot.com/faster-tests-sign-in-through-the-back-door

@scrogson

This comment has been minimized.

Copy link
Member

scrogson commented Jul 24, 2017

Cool.

So I'm just thinking of some options here...

I think because this functionality needs to be plugged into the user-land code we can't really rely on mix only including the code in :dev and :test as suggested above.

I would suggest that this code be conditionally included directly by the user-land code. Probably the best place would be in the user's guardian pipeline (v1).

defmodule MyApp.AuthPipeline do
  use Guardian.Plug.Pipeline, otp_app: :my_app

  if Application.get_env(:my_app, :guardian_backdoor) do
    plug Guardian.Plug.Backdoor
  end

  plug Guardian.Plug.VerifySession, claims: ...
  plug Guardian.Plug.VerifyHeader, claims: ...
  plug Guardian.Plug.EnsureAuthenticated
  plug Guardian.Plug.LoadResource, ensure: true
end

Then in config/test.exs:

config :my_app, :guardian_backdoor, true

The config option could be anything really. What do you think?

I realize that this still has the potential for mis-use as the backdoor can just be plugged without the wrapped conditional.

@doomspork

This comment has been minimized.

Copy link
Member

doomspork commented Jul 24, 2017

You're right, it can't be conditionally included at the mix level. Good catch @scrogson!

@scrogson

This comment has been minimized.

Copy link
Member

scrogson commented Jul 24, 2017

Maybe instead of the wrapped conditional, we could require the otp_app option to be passed? The plug can check if Application.get_env(:my_app, :guardian_backdoor) returns true. If not, it won't be plugged.

If the otp_app option isn't provided, we blow up at compile time. We could also print a warning when the backdoor is plugged and we're not in the test environment. These should all be determined at compile time.

@jayjun

This comment has been minimized.

Copy link
Author

jayjun commented Jul 25, 2017

How about :guardian_backdoor with Guardian.Plug.Backdoor as the module name?

@jayjun

This comment has been minimized.

Copy link
Author

jayjun commented Jul 25, 2017

Conditional packages typically aren’t controlled at mix.exs alone. It’s idiomatic to add Code.ensure_loaded?/1 in userland.

# mix.exs
{:guardian_backdoor, ..., only: [:test]}

# router.ex
if Code.ensure_loaded?(Guardian.Plug.Backdoor) do
  plug Guardian.Plug.Backdoor
end

Yes, it’s still dangerous without the conditional, but at least

plug Guardian.Plug.Backdoor

can’t even compile outside :test.

@scrogson

This comment has been minimized.

Copy link
Member

scrogson commented Jul 25, 2017

@jayjun yep, you are right about that. I like it!

My main concern now is maintaining yet another package.

navigate_to "/?token=\#{token}"
```
If you aren't using [Hound][hound], a simple `GET /?as=User:5` request will

This comment has been minimized.

@jsteiner

jsteiner Aug 16, 2017

It looks like there's some spots referencing as (here and in a test below). Is this outdated? I don't see any code to support it.

This comment has been minimized.

@jayjun

jayjun Aug 30, 2017

Author

@jsteiner You’re right! as is changed to token. Examples updated.

@jayjun jayjun force-pushed the jayjun:backdoor branch from 50e2757 to 2ae0938 Aug 30, 2017

@jayjun jayjun force-pushed the jayjun:backdoor branch from 2ae0938 to 29a6b37 Aug 30, 2017

@doomspork

This comment has been minimized.

Copy link
Member

doomspork commented Oct 25, 2017

@jayjun we have decided to move this out of guardian and into it's own package. I will create the basic project structure in https://github.com/ueberauth/guardian_backdoor if you'd like to move these changes to there 😁

@doomspork doomspork closed this Oct 25, 2017

@jayjun jayjun deleted the jayjun:backdoor branch Oct 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment