setPayload flag and customized header properties #21

brianjmiller · 2015-01-15T18:48:00Z

Trying this one again. It provides but fixes the same that was in #20 (which addresses #19).

It also now adds the ability to set custom properties in the header and have the verification on the other end still work. This was achieved by adding to the constructor for the JWS object a way to pass the header as an array rather than just the two individual options. The constructor change was the least change to the JWS::load method. Alternatively we could do the getHeader/setHeader dance.

Hopefully my tests have it covered this time. Note that I didn't alter the doc strings for the JWS constructor as I didn't know the best way to do so, nor whether it was worth publicizing. It might be left as an exercise for the reader who wants custom header properties.

Please have a look and let me know if any changes are needed. Thanks.

brianjmiller · 2015-01-15T18:48:19Z

@Spomky interested in giving a look too?

odino · 2015-01-18T17:23:29Z

src/Namshi/JOSE/JWS.php

+            }
+        }
+
+        parent::__construct(array(), $header);


TBH I think this will be confusing, as the algorith can either be the algorithm itself or a dictionary or headers. I think we should change the constructor rather than have this as it leads to misunderstanding and legacy

Yeah, I'm okay with that, I was just trying to maintain backwards compatibility to prevent the big version bump.

odino · 2015-01-18T17:25:50Z

thanks man! I just have a couple things I want to clarify before being able to merge this. If we change the public API - as we probably should - then we also need to modify the README and I will bump the version to 3.0.0

cheers!

brianjmiller · 2015-01-19T16:36:55Z

Yep, see my comments. Do you want to tackle the 3.0 bump changes?

odino · 2015-01-19T17:20:40Z

yeah super open to keep the code as simple as possible even if it require
version bumps! :)

On Mon, Jan 19, 2015 at 8:36 PM, Brian J. Miller notifications@github.com
wrote:

Yep, see my comments. Do you want to tackle the 3.0 bump changes?

—
Reply to this email directly or view it on GitHub
#21 (comment).

Nadalin Alessandro
http://www.linkedin.com/in/alessandronadalin
www.odino.org
www.twitter.com/_odino_

odino · 2015-01-21T08:09:52Z

btw are you using this branch anywhere in production? :)

brianjmiller · 2015-01-21T15:29:59Z

No, ultimately (within the next couple weeks?) it will be a dependency in the OSS lib here:

https://github.com/RusticiSoftware/TinCanPHP

odino · 2015-01-21T15:57:11Z

ok :) @Spomky any comment?

brianjmiller · 2015-02-03T20:20:34Z

@odino finally got to this one. Changes include switching JWS to accept an array of options that becomes the header in the object with still auto setting the 'typ' property. Also edited the README to reflect the new construction interface and include the major version bump to 3. Tests updated. phpDoc doesn't seem to have a way to document associative array parameter values in depth so I followed an SO answer that suggested using the WordPress guidelines and so did that. (See https://make.wordpress.org/core/handbook/inline-documentation-standards/php-documentation-standards/#1-1-parameters-that-are-arrays)

odino · 2015-02-04T20:36:11Z

i think im generally ok with this! @Spomky do you want to provide some feedback?

Spomky · 2015-02-04T22:19:03Z

Hi,

I think it is useless to go deep into the explanation.
The problem is that if you are too restrictive (e.g. only accepts string=>string or string=>integer) you will create issues/warning for projects that want to allow custom headers with JSON Serializable objects.

IMO the following description is enough:

* @param array $header An associative array of headers. The value can be any type accepted by json_encode or a JSON serializable object
* @see http://php.net/manual/en/function.json-encode.php
* @see http://php.net/manual/en/jsonserializable.jsonserialize.php

odino · 2015-02-05T09:42:57Z

BTW I think a nice way to handle with this problem is to:

switch to 3.X so that people wouldnt automatically upgrade
leave the automatic claims from the JWS class
have a new class that extends it, call it whatever, that adds the autoclaims on top of the JWS, if you dont pass them

That way its easier for people to pick which one to use and we keep the JWS class as simple as possible. Thoughts?

brianjmiller · 2015-02-05T19:45:46Z

@Spomky that seems reasonable on the doc string, I might link directly to the JWS rfc as well as a suggestion of what "should" be there. I called out those two properties in particular to show that 'typ' has a default and because 'alg' is the bear minimum which used to be constructed for you. Seems like we're moving away from that which is fine.

@odino what would the new class be called? Would the constructor of the new class still take the header as its argument or would you revert it to what the original constructor in 2.x accepted?

brianjmiller · 2015-02-13T16:28:52Z

I went ahead with some of these changes. I created a new class called EasyJWS that inherits from JWS. I moved the automatic handling of 'exp' and 'iat' into that class so that the base JWS class is barebones. Restoring the original constructor functionality was problematic because of the static load method still needing to construct with an array for the header. We could basically duplicate the load method from JWS in EasyJWS and make it work but I didn't like the non-DRYness of it.

I also updated the doc strings per @Spomky and updated the README to call out the JWT -> JWS -> EasyJWS structure.

@odino please have a look. thanks.

odino · 2015-02-14T19:14:38Z

looks good! any suggestion for the class name instead of EasyJWS? it feels weird :)

brianjmiller · 2015-02-14T20:05:04Z

Yeah, +1 if someone has a better suggestion, that was the best I could come up with.

odino · 2015-02-25T09:06:55Z

hey brian could you please rebase your changes?

brianjmiller · 2015-02-26T15:46:45Z

@odino rebased. Probably someone should review in light of the encoder changes, etc. that landed. I skimmed them but didn't look at them closely. Tests are passing as much as they were. Would be nice to get the 3 failures eliminated.

odino · 2015-03-01T14:01:58Z

WDYM by the 3 failure aliminated? :)

brianjmiller · 2015-03-02T17:04:00Z

I've been getting 3 failures consistently from the test suite since first started using the library. I think it may be environment specific, I thought I'd seen Travis blow up with the same but I can't seem to find that now. Here is the output from my environment:

repos/jose$ phpunit -c phpunit.xml.dist
PHPUnit 4.5.0 by Sebastian Bergmann and contributors.

Configuration read from /Users/brian/repos/jose/phpunit.xml.dist

...................F.F.F..............

Time: 154 ms, Memory: 6.75Mb

There were 3 failures:

1) Namshi\JOSE\Test\Signer\ES256Test::testSigningWorksProperly
Failed asserting that null is of type "string".

/Users/brian/repos/jose/tests/Namshi/JOSE/Test/Signer/ES256Test.php:41
/usr/local/pear/share/pear/PHPUnit/TextUI/Command.php:176
/usr/local/pear/share/pear/PHPUnit/TextUI/Command.php:129

2) Namshi\JOSE\Test\Signer\ES384Test::testSigningWorksProperly
Failed asserting that null is of type "string".

/Users/brian/repos/jose/tests/Namshi/JOSE/Test/Signer/ES384Test.php:31
/usr/local/pear/share/pear/PHPUnit/TextUI/Command.php:176
/usr/local/pear/share/pear/PHPUnit/TextUI/Command.php:129

3) Namshi\JOSE\Test\Signer\ES512Test::testSigningWorksProperly
Failed asserting that null is of type "string".

/Users/brian/repos/jose/tests/Namshi/JOSE/Test/Signer/ES512Test.php:31
/usr/local/pear/share/pear/PHPUnit/TextUI/Command.php:176
/usr/local/pear/share/pear/PHPUnit/TextUI/Command.php:129

FAILURES!
Tests: 38, Assertions: 62, Failures: 3.

brianjmiller · 2015-03-13T14:16:02Z

@odino anything else needed here?

odino · 2015-03-23T09:01:05Z

sorry man :( had very little time lately and we also had to rework on this library due to this

Ill try to give it a shot this weekend though I cant promise anything :-S

brianjmiller · 2015-04-01T13:19:18Z

Catching up now on https://auth0.com/blog/2015/03/31/critical-vulnerabilities-in-json-web-token-libraries/. Presumably this PR needs to be rebased again and potentially examined based on how it handles the header.

odino · 2015-04-01T13:33:06Z

oh yeah, definitely :( I think it actually needs to be rebased cause we pushed some fixes a few weeks ago to exactly target those vulnerabilities

brianjmiller · 2015-04-08T18:34:32Z

@odino rebased again, would love to get this landed so that we can push forward with our other library's additions, dependencies are hard :-).

cirpo · 2015-04-21T06:32:09Z

@brianjmiller could you please rebase it again? We just created version 3.0.0 which includes phpSeclib
thanks

brianjmiller · 2015-04-21T13:31:46Z

Merging this change would then bump to 4.0.0 because it is non-backwards compatible. This will be the third rebase, will it actually be merged in timely manner? I can't continue to spend time rebasing and sitting on projects waiting on the dependency. Alternatively I could revert to the original implementation which didn't cause the backwards incompatible breakage, but was not prefer from a "complexity" of code standpoint. Either way appreciate the efforts, just need to make progress, thanks @cirpo.

cirpo · 2015-04-21T13:36:49Z

@brianjmiller yes, we will bump up to 4.0.0 and yes I will merge it in less than 24h!

sorry and thanks a lot for your patience

cirpo · 2015-04-21T13:45:35Z

@brianjmiller it would be great if you can also modify the README pointing out the incompatibility between 4.0.0 and 3.0.0

* Switch JWS constructor to accept header as array * Includes bump to major version 4 in README * Move auto claim handling into new EasyJWS class

brianjmiller · 2015-04-21T20:46:25Z

@cirpo squashed + rebased. The number of commits was getting pretty ugly from the rebases (the conflict fixes) so I figured I'd clean up the history a bit.

cirpo · 2015-04-22T06:20:14Z

@brianjmiller thanks, I'm gonna look into that today, thanks

cirpo · 2015-04-22T17:32:34Z

tests/Namshi/JOSE/Test/JWSTest.php

@@ -100,16 +97,6 @@ public function testVerificationRS256()
        $this->assertEquals('b', $payload['a']);
    }

-    public function testValidationOfAValidJWS()


is there any reason why these tests are deleted?

The tests are specifically hitting the valid method which is moved into the EasyJWS class, so the corresponding tests got moved into EasyJWSTest. The other tests in this file hit verify which is the important method for the base JWS class after these changes.

cirpo · 2015-04-22T17:36:58Z

@brianjmiller I made some comments, could you also please mention the differences between version 2, 3 and 4 in the README? Otherwise I will merge the PR, commit with an updated README/CHANGELOG and tag it 4.0.
Thanks, I promise we will get to the release in no time :)

brianjmiller · 2015-04-23T13:27:38Z

@cirpo should be addressed. I wasn't clear on the bump from 1 to 2 or whether there were prior releases, you'd have to round that out if you want it.

brianjmiller · 2015-04-24T13:44:23Z

@cirpo updated with indication in README.

cirpo · 2015-04-24T15:22:30Z

@brianjmiller thank you! I was thinking about the EasyJws name, what about SimpleJWS?
We can even merge and tag now, I feel you're going to kill us :P

brianjmiller · 2015-04-24T19:19:45Z

I think I went "Easy" because it implies the public interface is easy to use, as opposed to "Simple" because that might imply under featured. It honestly doesn't matter much to me, I don't love either :-).

brianjmiller · 2015-04-24T19:23:36Z

@cirpo new commit EasyJWS -> SimpleJWS.

cirpo · 2015-04-25T07:04:53Z

@brianjmiller thanks, merging!

setPayload flag and customized header properties

cirpo · 2015-04-25T07:10:16Z

done and tagged!

brianjmiller · 2015-04-27T17:46:02Z

phew, thanks!

odino reviewed Jan 18, 2015
View reviewed changes

brianjmiller mentioned this pull request Feb 5, 2015

Implement attachments and statement signing RusticiSoftware/TinCanPHP#23

Merged

brianjmiller force-pushed the master branch from 14fbaf7 to 0faad03 Compare February 26, 2015 15:43

brianjmiller force-pushed the master branch from 0faad03 to e06c1bb Compare April 8, 2015 18:26

brianjmiller added 2 commits April 21, 2015 15:03

Fix JWTTest generateSigninInput test

4278a06

Allow for custom properties in JWS header

c2e29fd

* Switch JWS constructor to accept header as array * Includes bump to major version 4 in README * Move auto claim handling into new EasyJWS class

brianjmiller force-pushed the master branch from e06c1bb to c2e29fd Compare April 21, 2015 20:43

cirpo reviewed Apr 22, 2015
View reviewed changes

Add major version information to README

e99ec80

Mark 3.x.x to 4.x.x as incompatible in README

fbcdf92

Rename EasyJWS to SimpleJWS

3186960

cirpo added a commit that referenced this pull request Apr 25, 2015

Merge pull request #21 from brianjmiller/master

a418f6c

setPayload flag and customized header properties

cirpo merged commit a418f6c into namshi:master Apr 25, 2015

setPayload flag and customized header properties #21

setPayload flag and customized header properties #21

Conversation

brianjmiller commented Jan 15, 2015

brianjmiller commented Jan 15, 2015

odino Jan 18, 2015

Choose a reason for hiding this comment

brianjmiller Jan 19, 2015

Choose a reason for hiding this comment

odino commented Jan 18, 2015

brianjmiller commented Jan 19, 2015

odino commented Jan 19, 2015

odino commented Jan 21, 2015

brianjmiller commented Jan 21, 2015

odino commented Jan 21, 2015

brianjmiller commented Feb 3, 2015

odino commented Feb 4, 2015

Spomky commented Feb 4, 2015

odino commented Feb 5, 2015

brianjmiller commented Feb 5, 2015

brianjmiller commented Feb 13, 2015

odino commented Feb 14, 2015

brianjmiller commented Feb 14, 2015

odino commented Feb 25, 2015

brianjmiller commented Feb 26, 2015

odino commented Mar 1, 2015

brianjmiller commented Mar 2, 2015

brianjmiller commented Mar 13, 2015

odino commented Mar 23, 2015

brianjmiller commented Apr 1, 2015

odino commented Apr 1, 2015

brianjmiller commented Apr 8, 2015

cirpo commented Apr 21, 2015

brianjmiller commented Apr 21, 2015

cirpo commented Apr 21, 2015

cirpo commented Apr 21, 2015

brianjmiller commented Apr 21, 2015

cirpo commented Apr 22, 2015

cirpo Apr 22, 2015

Choose a reason for hiding this comment

brianjmiller Apr 23, 2015

Choose a reason for hiding this comment

cirpo commented Apr 22, 2015

brianjmiller commented Apr 23, 2015

brianjmiller commented Apr 24, 2015

cirpo commented Apr 24, 2015

brianjmiller commented Apr 24, 2015

brianjmiller commented Apr 24, 2015

cirpo commented Apr 25, 2015

cirpo commented Apr 25, 2015

brianjmiller commented Apr 27, 2015