Handle claims conversion #171

lcobucci · 2017-02-17T17:10:13Z

There're some weird stuff to improve @Ocramius (and I couldn't resist adding the last commit 😂).

Fixes #144
Fixes #43

Ocramius · 2017-02-17T19:19:03Z

src/Token/Builder.php

+
+    private function formatClaims(array $claims): array
+    {
+        if (isset($claims[RegisteredClaims::AUDIENCE]) && count($claims[RegisteredClaims::AUDIENCE]) === 1) {


Please don't sneak in this optimisation here :-P

Also, if you assume that it always is a packed array, simply use an isset() on index 0 and another one on index 1

Yeah it's definitely better then count() in that case

Ocramius · 2017-02-17T19:20:58Z

src/Token/Builder.php

+        return $claims;
+    }
+
+    private function convertDate(DateTimeImmutable $date)


Return type declaration needed

I see it's int|string. Can't it be float?

Note that json_encode() does some trickery around floats, making them integers when no part is after the comma

Ref: https://3v4l.org/Uq1CL

I was with the old behaviour in my mind: https://3v4l.org/1Q7hh

Then when we convert it back to a DateTime we would have DateTimeImmutable::createFromFormat('U.u', '1487372450.0231') instead of DateTimeImmutable::createFromFormat('U.u', '1487372450.023126'), what could cause some weird errors.

Ah, I see what you mean. If you can, you should cover the edge case you just mentioned in the test suite

It's being covered on https://github.com/lcobucci/jwt/pull/171/files#diff-89e66398726c97dea8259f9c0d31b5f5R417

But can be more explicit, ofc

Ocramius · 2017-02-17T19:22:01Z

src/Token/Builder.php

+        $seconds = $date->format('U');
+        $microseconds = $date->format('u');
+
+        return $microseconds === '000000' ? (int) $seconds : $seconds . '.' . $microseconds;


Big assumption on the number of decimal digits. Instead, use 0 === (int) $microseconds

DateTimeInterface#format('u') always returns a 6 chars string. It can be compared to 0 though, there's no much difference

Yeah, but nothing prevents the engine from making this 7 chars, or similar, in future versions. This code is very fragile

Ocramius · 2017-02-17T19:26:59Z

src/Token/Parser.php

-        return (array) $this->decoder->jsonDecode($this->decoder->base64UrlDecode($data));
+        $claims = (array) $this->decoder->jsonDecode($this->decoder->base64UrlDecode($data));
+
+        if (isset($claims[RegisteredClaims::AUDIENCE])) {


Note that casting null to (array) yields an empty array. I don't know if that's valid for $claims, but worth considering

I don't want to set if it doesn't exist in the original token

Makes sense

Ocramius · 2017-02-17T19:27:45Z

src/Token/Parser.php

+        }
+
+        foreach (RegisteredClaims::DATE_CLAIMS as $claim) {
+            if (!isset($claims[$claim])) {


areay_intersect_key()?

In order to use it I would also have to do an array_flip(RegisteredClaims::DATE_CLAIMS) (or an array_keys($claims) and use array_intersect() instead) not sure if will help us...

Or define the constants in the keys ;-)

Ocramius · 2017-02-17T19:29:34Z

src/Token/Parser.php

+            $value .= '.0';
+        }
+
+        return DateTimeImmutable::createFromFormat('U.u', $value);


I'd use two different expressions for when a microtime is set or not: this way you avoid overwriting $value

Ocramius · 2017-02-17T19:30:12Z

src/Token/RegisteredClaims.php

@@ -27,6 +27,8 @@
        self::SUBJECT
    ];

+    const DATE_CLAIMS = [self::ISSUED_AT, self::NOT_BEFORE, self::EXPIRATION_TIME];


One per line, please

Processed 😉

lcobucci · 2017-02-19T09:30:42Z

@Ocramius could you please review it again?

lcobucci added the Improvement label Feb 17, 2017

lcobucci added this to the 4.0.0 milestone Feb 17, 2017

lcobucci self-assigned this Feb 17, 2017

lcobucci requested a review from Ocramius February 17, 2017 17:10

Ocramius previously requested changes Feb 17, 2017

View reviewed changes

lcobucci force-pushed the handle-claims-conversion branch from 31a7d7f to f7f7c64 Compare February 19, 2017 09:26

lcobucci added 5 commits February 19, 2017 10:27

Add a list of all date claims

60e2dae

Use datetime objects to build the token

8d9fdb7

Convert numeric dates into datetime objects on parsing

7b07d2c

Make the token always rely on datetime objects

bfb220b

Force Parser to always use "aud" as array

3fe29d0

lcobucci force-pushed the handle-claims-conversion branch from f7f7c64 to c74ef19 Compare February 19, 2017 09:29

Encode "aud" as string when we just have one permitted client

ee3669e

lcobucci force-pushed the handle-claims-conversion branch from c74ef19 to ee3669e Compare February 19, 2017 10:15

lcobucci requested a review from henriquemoody February 23, 2017 15:47

Ocramius approved these changes Feb 23, 2017

View reviewed changes

Ocramius merged commit 6dbb242 into master Feb 23, 2017

lcobucci deleted the handle-claims-conversion branch February 23, 2017 22:26

lcobucci mentioned this pull request Apr 17, 2018

Exp in microseconds #229

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle claims conversion #171

Handle claims conversion #171

lcobucci commented Feb 17, 2017

Ocramius Feb 17, 2017

Ocramius Feb 17, 2017

lcobucci Feb 17, 2017

Ocramius Feb 17, 2017

Ocramius Feb 17, 2017

Ocramius Feb 17, 2017

Ocramius Feb 17, 2017

lcobucci Feb 17, 2017 •

edited

Loading

Ocramius Feb 17, 2017

lcobucci Feb 17, 2017

lcobucci Feb 17, 2017

Ocramius Feb 17, 2017

lcobucci Feb 17, 2017 •

edited

Loading

Ocramius Feb 17, 2017

lcobucci Feb 17, 2017

Ocramius Feb 17, 2017

lcobucci Feb 17, 2017

Ocramius Feb 17, 2017

Ocramius Feb 17, 2017

lcobucci Feb 17, 2017

Ocramius Feb 17, 2017

Ocramius Feb 17, 2017

Ocramius Feb 17, 2017

lcobucci commented Feb 19, 2017

Handle claims conversion #171

Handle claims conversion #171

Conversation

lcobucci commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcobucci Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcobucci Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcobucci commented Feb 19, 2017

lcobucci Feb 17, 2017 •

edited

Loading

lcobucci Feb 17, 2017 •

edited

Loading