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

Already on GitHub? Sign in to your account

Support for audience, issuer and expiration check + unit tests #3

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants

woloski commented Mar 21, 2013

Added support for

  • Audience check (DecodeToObject(jwt, secret, audience: "foo")
  • Expiration check (DecodeToObject(jwt, secret, checkExpiration: true) default: false
  • Issuer check (DecodeToObject(jwt, secret, issuer: "bar")

I kept it backward compat but added an override that takes the secret as a byte[] instead of a string. This is a common practice, since you don't know how those bytes were encoded (you are currently assuming UTF8), specially in interop scenario. All the checks are done optionally and are turned off by default as well.

Let me know if this is ok and if you can publish latest to NuGet so I can take the dependency.

@mikelehen mikelehen commented on the diff Mar 22, 2013

JWT/JWT.cs
+ {
+ private static Dictionary<JwtHashAlgorithm, Func<byte[], byte[], byte[]>> HashAlgorithms;
+ private static JavaScriptSerializer jsonSerializer = new JavaScriptSerializer();
+
+ static JsonWebToken()
+ {
+ HashAlgorithms = new Dictionary<JwtHashAlgorithm, Func<byte[], byte[], byte[]>>
+ {
+ { JwtHashAlgorithm.HS256, (key, value) => { using (var sha = new HMACSHA256(key)) { return sha.ComputeHash(value); } } },
+ { JwtHashAlgorithm.HS384, (key, value) => { using (var sha = new HMACSHA384(key)) { return sha.ComputeHash(value); } } },
+ { JwtHashAlgorithm.HS512, (key, value) => { using (var sha = new HMACSHA512(key)) { return sha.ComputeHash(value); } } }
+ };
+ }
+
+ /// <summary>
+ /// Creates a JWT given a payload, the signing key (as a string that will be decoded with UTF8), and the algorithm to use.
@mikelehen

mikelehen Mar 22, 2013

Contributor

The string will be encoded as UTF8, not decoded.

@mikelehen mikelehen commented on the diff Mar 22, 2013

README.md
@@ -11,7 +11,7 @@ The easiest way to install is via NuGet. See [here](https://nuget.org/packages/
{ "claim1", 0 },
{ "claim2", "claim2-value" }
};
- var secretKey = "GQDstcKsx0NHjPOuXOYg5MbeJ1XT0uFiwDVvVBrk";
+ var secretKey = Convert.FromBase64String("GQDstcKsx0NHjPOuXOYg5MbeJ1XT0uFiwDVvVBrk");
@mikelehen

mikelehen Mar 22, 2013

Contributor

This shouldn't be Base64-decoded (it'll make the output below wrong and it also complicates the example).

Contributor

mikelehen commented Mar 22, 2013

Thanks for the pull request. Unfortunately, I'm not quite happy with it. I think the claim-checking is probably best done by the application code, not inside the JWT library. The JWT spec mentions, "The interpretation of the audience value is generally application specific." and I assumed issuer was mostly informational, not something you'd generally want to validate. Since these are just string comparisons, it's no easier for us to validate them internally than for the application code to handle it. Expiration time might be reasonable for us to validate internally, but even for that, the spec mentions "Implementers MAY provide for some small leeway, usually no more than a few minutes, to account for clock skew." So I'm inclined to keep this library just focused on encoding / decoding tokens and validating their signature and leave the claim-handling to the application code.

I'm not opposed to the change that allows specifying the secret key as bytes, and the unit tests are definitely welcome. So feel free to modify the pull request to just include those changes.

Also, FYI- You seem to have converted all tabs to spaces. I'm not horribly opposed to this, but it makes it more difficult to review your changes.

woloski commented Mar 22, 2013

I respect your decision but not share it. Why repeat those checks over and over for each app. Also the spec says that those are optional but they still belong to the spec, hence to libraries that implement it. Anyway, it is your library so you do what you want with it. I didnt break functionallity, I built on top. It is not clear to me what you are loosing by incorporating this (people can still use the lib as it was).

woloski commented Mar 22, 2013

Sorry about the space/tabs. Not sure how that happened. I use tabs

@mikelehen mikelehen closed this Jun 9, 2013

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