ueberauth / guardian

Elixir Authentication
MIT License
3.44k stars 381 forks source link

claims["sub"] recommendation #448

Closed bjunc closed 6 years ago

bjunc commented 6 years ago

It seems that prior to v1, the convention for sub naming was %{"sub" => "User:" <> id}. As of v1, it now just seems that it's %{"sub" => id} (no User: prefix). I was curious what the rationale was for this change. I realize that any string can be used as the sub, but there must have been a reason to change the examples in the docs? Thanks!

yordis commented 6 years ago

@bjunc to be honest it doesn't really matter.

That details is something that belongs to your application design. If you will have the same Guardian module for multiple resources, for example: User and Admin you would probably do something like that so in your resource_from_claims/1 you could pattern matching for use the correct Ecto schema or whatever you are using for knowing which model you should load.

But that is an application specific business rule in how you do it.

I would keep doing it to be honest, it makes your code easier to know and people will understand the model that is being used.

def resource_from_claims(%{ "sub" => "User:" <> id }) do
    resource = MyApp.User.get_user(id)
    {:ok,  resource}
  end

def resource_from_claims(%{ "sub" => "Admin:" <> id }) do
    resource = MyApp.Admin.get_admin(id)
    {:ok,  resource}
  end

  def resource_from_claims(_claims) do
    {:error, :reason_for_error}
  end

make sure you do the same in the serialization of the sub of your claim for sure.

But, everything ends of on application design and Guardian shouldn't dictate that

bjunc commented 6 years ago

Yeah, I realize this isn't a technical limitation, or security issue, etc.. Rather, since the documentation changed, maybe there was a "best practice" I wasn't aware of – especially because I would have though User:1 would be the best practice. Anyway, thanks for the response. I had ultimately come to the same conclusion, and stuck with the User:1 convention:

#summarized for brevity

def subject_for_token(%User{} = resource, _claims) do
  {:ok, "User:" <> to_string(resource.id)}
end

def resource_from_claims(%{"sub" => "User:" <> id}) do
  {:ok, Repo.get(User, id)}
end