ueberauth / guardian

Elixir Authentication
MIT License
3.44k stars 381 forks source link

The verify_literal_claims function should be more that an equal check #555

Closed jsangilve closed 5 years ago

jsangilve commented 5 years ago

While using JWT, there are situations where the same token is valid for different audiences:

{
  "iss": "https://login.example.com/",
  "sub": "custom|user_id",
  "aud": [
    "example_microservices_api",
    "example_customers_api",
    "example_other_api"
  ],
  "iat": 1545404246,
  "exp": 1545411446,
  "azp": "authorized_party_whatever_id",
  "scope": "openid"
}

In the example above, the token is valid for 3 different audiences that could be handled by 3 completely different apps. For instance, one application could only require the token to have example_microservices_api audience.

Normally, we'd define a Guardian's pipeline using VerifyHeader plug as follows:

plug Guardian.Plug.VerifyHeader, claims: %{aud:  "example_microservices_api"}

This will fail because the aud claim doesn't match the token's aud claim and Guardian.Plug.Verify.verify_literal_claim only does an equal check:

https://github.com/ueberauth/guardian/blob/38aa02508fe9e832d7fdd0bb081a4e6f172b7b56/lib/guardian/token/verify.ex#L68-L80.

Could we change this code to something that consider where claim's value is a list?

def verify_literal_claims(claims, nil, _opts), do: {:ok, claims}

def verify_literal_claims(claims, claims_to_check, _opts) do
  results = for {k, v} <- claims_to_check, into: [] do
    claims
    |> Map.get(k)
    |> verify_literal_claim(k, v)
  end

  errors = Enum.filter(results, &(elem(&1, 0) == :error))

  if Enum.any?(errors), do: hd(errors), else: {:ok, claims}
end

defp verify_literal_claim(claim_values, key, v) when is_list(claim_values),
  do: if v in claim_values, do: {:ok, claims}, else: {:error, key}

defp verify_literal_claim(claim_value, key, v),
  do: if  claim_value == v, do: {:ok, claims}, else: {:error, key}

N.B: I could probably write this code n a better way.

yordis commented 5 years ago

N.B: I could probably write this code n a better way.

PR welcome 💓 try to do not introduce breaking changes and the default behavior is the current one.

Hanspagh commented 5 years ago

I assume this is fixed, so we should close this