Use percent encoding in OAuth according to rfc 5849 section 3.6

[SvenSchindler]

The current OAuth signature calculator does not encode path elements with an asterisk correctly.

Eg. the path

/foo/*bar

should be encoded as

/foo/%2Abar

However, the current calculator uses the reserved form charset in its calculation and does not encode asterisks at all. This results in an invalid signature.

This request uses percent encoding according to RFC 5849 section 3.6 for OAuth signature generation in order to to support url paths such as foo/*bar/

[slandelle]

@SvenSchindler Thanks! Could you please add some tests to OAuthSignatureCalculatorTest demonstrating the issue and the fix?

[slandelle]

@SvenSchindler I'm concerned with the soundness of your test. I suspect you've been reinjecting the result of your change into a test, hence you're proving nothing. Are you aware of an online OAuth1 implementation that could serve as a TCK?

[SvenSchindler]

Hi, I can totally understand. Without modifying the calculator, however, I can't check the actual signature in the test because obviously nonce and timestamp get updated with each run. I used http://lti.tools/oauth/ to verify the resulting signature.

[SvenSchindler]

Do you have any recommendations about how to improve the test? We could refactor the Signature Calculator to allow passing a specific nonce and timestamp value so that a test can verify a reproducible signature. This wouldn't improve the verification of the this issue though.

[slandelle]

Hi. Sorry for the late reply.

I think we could simply introduce a package protected method and have calculateAndAddSignature compute nonce and timestamp, and then delegate to it. This way, we would directly test this method and force those parameters. WDYT?

[SvenSchindler]

Hi, no worries. Thanks for your advice. I actually had a similar idea, just didn't want to meddle with the code too much. If its fine with you then I'll implement this, update the test to actually check a computed signature and then you can have a look.

[slandelle]

@SvenSchindler Yes, it would be great. Actually, I realize how much this part of the code could be optimized. I'd like to tackle it but it'd rather have proper tests first... Thanks a lot for your help on this!

[SvenSchindler]

No worries :). I just noticed that this protected method does actually already exist, its called calculateSignature. This should be sufficient for testing the signature generation.

[slandelle]

Yeah, I saw that. Then:

• I wonder how much testing only calculateSignature in isolation makes sense. If so, I guess constructAuthHeader should be made package private too and tested in isolation too.
• There's also generateTimestamp and generateNonce that are protected (should be package protected) so one can override for testing and return static values.

[SvenSchindler]

I agree, on a long term, however, I would consider passing some kind of date and nonce provider to the OAuthSignatureCalculatorConstructor which can easily be replaced in tests. I'll have a look at the constructAuthHeader as well.

[SvenSchindler]

I revised the test to make it a bit simpler to verify the signature generation using http://lti.tools/oauth/ . I'm not a big fan of multiple asserts though.

[slandelle]

Thanks!

I'm going to merge as is and will iterate over this.

Thanks a lot!

[SvenSchindler]

Great, thank you very much :)