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

Adding JwtBuilder #140

Merged
merged 58 commits into from
Dec 13, 2017
Merged

Conversation

paule96
Copy link
Contributor

@paule96 paule96 commented Oct 31, 2017

Start the work on a fluent-API to decode und create a JWT.

@paule96 paule96 mentioned this pull request Oct 31, 2017
@abatishchev abatishchev changed the title Start working on #113 Adding JwtBuilder Oct 31, 2017
@@ -43,5 +43,8 @@

<ItemGroup Condition="'$(TargetFramework)' == 'netstandard1.3'">
<PackageReference Include="System.Security.Cryptography.Csp" Version="4.3.0" />
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.5.0-preview-20170810-02" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know why both MsTest and xUnit are here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh sorry. that was a test, to get the test runing. I remove this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

/// <summary>
/// Build an Decode JWT for with a Fluent-API.
/// </summary>
public class Builder
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd call it JwtBuilder withing main JWT project since it so far doesn't add new dependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reflection stuff is new. This is why I upgrade to .net standard 2.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you think we don't need the sub-namespace?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd not have sub-namespaces until there are too many classes in the root or there is a strict need such as grouping/separation

/// Check if we have enaught information to build a new Token.
/// </summary>
/// <returns>Returns true if all requierd information are there to create a token.</returns>
private bool canWeBuild()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CanBuild()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

/// Check if we have enaught informations to decode a token.
/// </summary>
/// <returns>Returns ture if all requiered informations are there to create a token.</returns>
private bool canWeDecode()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CanDecode()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

urlEncoder != null
)
{
if (verify && secret == null && secret.Length > 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return (!verify || (verify && secret == null && secret.Length > 0);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

/// All predefined Headers specified by RFC. (https://tools.ietf.org/html/rfc7519)
/// Last update 31.10.2017
/// </summary>
public enum HeaderNames
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HeaderName (singular)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

/// </summary>
/// <param name="value">A object that have the Describtion Attribut set</param>
/// <returns>The string of the Describtion</returns>
private static string getValueOfDescription(object value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetValueOfDescription (CamelCase)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

private static string getValueOfDescription(object value)
{
var info = value.GetType().GetField(value.ToString());
var attributes = (DescriptionAttribute[])info.GetCustomAttributes(typeof(DescriptionAttribute), false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is extension method GetCustomAttribute<DescriptionAttribute>()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

/// </summary>
/// <param name="Header">A Instance of a dictionary that contains the headers</param>
/// <param name="PayLoad">A instance of a dictionary that contans the payload</param>
public JWTData(Dictionary<string, string> Header, Dictionary<string, object> PayLoad)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have class for this - JwtParts. You can reuse it once merge the projects

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @abatishchev I don't know if I can reuse it. I don't think this to classes represent the same stuff.

JWParts are more a think to make the work with a encoded token easier. But JWTDate is more for the decoded Token work. So you can easy add an remove data from the token befor you encode the token.
I'm not shure if I can merge this two worlds together.

Because to get a JWTData from a JWTParts you need for example a URLDecoder.

/// Create a new instance of JWTData and initalize Header and Payload.
/// </summary>
/// <returns>A Instance of <see cref"JWTData"/></returns>
public JWTData() : this(Header: new Dictionary<string, string>(), PayLoad: new Dictionary<string, object>())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parameters should be camelCase'd

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@paule96
Copy link
Contributor Author

paule96 commented Oct 31, 2017

I think currently about the Extensions folder to put all the Builder-API-Methods as extension methods in separate classes, so that the builder class don't explode. What do you think about this?

@paule96
Copy link
Contributor Author

paule96 commented Nov 5, 2017

I have start the work to the extension methods. Please take a look at this.

@nbarbettini
Copy link
Contributor

Approving so I don't block the merge later. Looking good @paule96! This adds something I've wanted in JWT.NET for a long time. 😄

I left a number of comments with small nitpicks like typos, etc. I think the library should use DateTimeOffset instead of DateTime, but that might be a larger discussion.

Cheers!

@abatishchev
Copy link
Member

@nbarbettini thanks a lot! You've spotted many silly errors. Made me scan the codebase once again, fix all of them, find some more. Your helps as always is appreciated :)

@abatishchev abatishchev merged commit 3e62ccd into jwt-dotnet:master Dec 13, 2017
@abatishchev
Copy link
Member

Wow, we did it. Big thanks to @paule96 for your contribution!

@abatishchev
Copy link
Member

abatishchev commented Dec 13, 2017

Pushed to NuGet as 3.2.0-beta

@paule96
Copy link
Contributor Author

paule96 commented Dec 14, 2017

Hey developers,

I'm happy to see this code now in the main repo! Thanks to all your feedback. 👍
@abatishchev @nbarbettini

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

Successfully merging this pull request may close these issues.

3 participants