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.

Sign up for GitHub

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

Jump to bottom

Ensure that headers are correctly cased before signature verification… #59

Merged
merged 3 commits into from
Dec 21, 2017
Merged

Ensure that headers are correctly cased before signature verification… #59

merged 3 commits into from
Dec 21, 2017

Conversation

OGKevin
Copy link
Contributor

@OGKevin OGKevin commented Dec 21, 2017

Closes #49

@OGKevin OGKevin added this to the 0.12.4 milestone Dec 21, 2017
@OGKevin OGKevin self-assigned this Dec 21, 2017
epels
epels reviewed Dec 21, 2017
View reviewed changes
Copy link

@epels epels left a comment

Choose a reason for hiding this comment

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

@OGKevin

BunqSdk/Security/SecurityUtils.cs Outdated
@@ -127,6 +133,20 @@ private static string GenerateRequestHeadersSortedString(HttpRequestMessage requ
);
}

private static string GetHeaderNameCorrectyCased(string headerName)
Copy link

Choose a reason for hiding this comment

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

GetHeaderNameCorrectyCased

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lets create a follow up issue to add tests in all SDK's. I've done a quick dirty test by manipulating the headers right after the response is received to see if it indeed works, which it does. But a test unit is indeed needed to ensure that in the future it does not break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#60

BunqSdk/Security/SecurityUtils.cs Outdated
@@ -127,6 +133,20 @@ private static string GenerateRequestHeadersSortedString(HttpRequestMessage requ
);
}

private static string GetHeaderNameCorrectyCased(string headerName)
Copy link

Choose a reason for hiding this comment

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

Test?

BunqSdk/Security/SecurityUtils.cs Outdated
private static string GetHeaderNameCorrectyCased(string headerName)
{
headerName = headerName.ToLower();
headerName = headerName.First().ToString().ToUpper() + headerName.Substring(1);
Copy link

Choose a reason for hiding this comment

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

https://msdn.microsoft.com/nl-nl/library/system.globalization.textinfo.totitlecase(v=vs.110).aspx ?

Copy link
Contributor

Choose a reason for hiding this comment

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

what emile says ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regex is already used in the other SDK's. So for consistency i would like to keep using regex.

Copy link

Choose a reason for hiding this comment

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

@OGKevin This is what I meant: https://dotnetfiddle.net/QdElDN

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I understand. But for the sake of maintaining, I prefer consistency over all the SDK's. So lets keep the SDK's as consistent with each other as we can.

@andrederoos andrederoos changed the title Insure that headers are correclty cased before signature verification… Insure that headers are correctly cased before signature verification… Dec 21, 2017
@OGKevin OGKevin mentioned this pull request Dec 21, 2017
Open
andrederoos
andrederoos approved these changes Dec 21, 2017
View reviewed changes
BunqSdk/Security/SecurityUtils.cs Outdated
@@ -127,6 +133,20 @@ private static string GenerateRequestHeadersSortedString(HttpRequestMessage requ
);
}

private static string GetHeaderNameCorrectyCased(string headerName)
Copy link
Contributor

Choose a reason for hiding this comment

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

GetHeaderNameCorrectlyCased

BunqSdk/Security/SecurityUtils.cs Outdated
private static string GetHeaderNameCorrectyCased(string headerName)
{
headerName = headerName.ToLower();
headerName = headerName.First().ToString().ToUpper() + headerName.Substring(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

what emile says ;-)

@andrederoos andrederoos merged commit 9e66f21 into develop Dec 21, 2017
@andrederoos andrederoos deleted the feature/make_sure_headers_are_correctly_cased_bunq/sdk_csharp#49 branch December 21, 2017 16:02
@OGKevin OGKevin changed the title Insure that headers are correctly cased before signature verification… Ensure that headers are correctly cased before signature verification… Dec 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epels epels epels left review comments

@andrederoos andrederoos andrederoos approved these changes

Assignees

@OGKevin OGKevin

Labels
None yet
Projects
None yet
Milestone
0.12.4
Development

Successfully merging this pull request may close these issues.
3 participants
@OGKevin @andrederoos @epels