Use tokenField from config if defined #376

GrandFelix · 2020-05-13T10:25:43Z

fixed wrong used config fields in some places when using custom fields
use tokenField from config if defined else IdentifierInterface::CREDENTIAL_JWT_SUBJECT.

this fixes this #353 (comment)

use tokenField from config if defined else IdentifierInterface::CREDENTIAL_JWT_SUBJECT

in conditions must be used dataField

src/Identifier/TokenIdentifier.php

src/Authenticator/JwtAuthenticator.php

ndm2 · 2020-05-13T13:55:23Z

src/Identifier/TokenIdentifier.php

-        $dataField = $this->getConfig('dataField');
-        if (!isset($data[$dataField])) {
+        $tokenField = $this->getConfig('tokenField');
+        if (!isset($data[$tokenField])) {
            return null;
        }

        $conditions = [
-            $this->getConfig('tokenField') => $data[$dataField],
+            $this->getConfig('dataField') => $data[$tokenField],


Why is this change necessary? Unless I'm missing something, It just flips the config options, which breaks backwards compatibility.

you are right. I came out from this issue #353 (comment) I think data and token name is confusing. I expected that data is db field and token for token. But its other way arround. And also when you use custom token key name, here https://github.com/cakephp/authentication/pull/376/files/90a2daea03f470ef1294d9ac423bd42870a17ece#diff-b7a10d2c99be4b6528582b9471a0fb86R92 was fixed to 'sub' key name and because of that custom naming did not work and I did some debug. But now I see that naming is vice vera as I thought. But this https://github.com/cakephp/authentication/blob/master/src/Authenticator/JwtAuthenticator.php#L92 is still the issue.

~~JwtAuthenticator should use JwtSubjectIdentifier, you don't need to mess around with the TokenIdentifier.~~

Scratch that, JwtSubjectIdentifier extends TokenIdentifier and just changes the default config.

@GrandFelix Switching the use of these config options would be a backwards compatibility break.

Since the issue is about making use of IdentifierInterface::CREDENTIAL_JWT_SUBJECT only JwtAuthenticator needs changes.

yes... will fix that. thanks.

ndm2 · 2020-05-13T14:00:25Z

src/Authenticator/JwtAuthenticator.php

        $result = json_decode(json_encode($result), true);

-        $key = IdentifierInterface::CREDENTIAL_JWT_SUBJECT;
+        $key = $this->getConfig('tokenField', IdentifierInterface::CREDENTIAL_JWT_SUBJECT);


This ties into the question why the config options are being flipped in the identifier... Should this use dataField instead?

ADmad · 2020-05-13T15:12:21Z

Walking through the JwtAuthenticator I see why a lot of people seem to have problem using it and why they need they need the use of sub key (claim in JWT terms) configurable.

The authenticator has returnPayload option set to true by default which means the decoded JWT claims are directly returned as identity.

The way the JwtAuthenticate of my plugin for Cake 3.x works by default is it uses the value of sub claim of JWT to find user record with that id and use that as user/identity record.

Returning the JWT claims as identity isn't the best default IMO.

GrandFelix · 2020-05-13T15:59:18Z

ok.. I have reverted almost everything except JwtAuthenticator $key definition.

ADmad · 2020-05-13T16:02:53Z

src/Authenticator/JwtAuthenticator.php

        $result = json_decode(json_encode($result), true);

-        $key = IdentifierInterface::CREDENTIAL_JWT_SUBJECT;
+        $key = $this->getConfig('dataField', IdentifierInterface::CREDENTIAL_JWT_SUBJECT);


You should add the new key in the default config array using the constant for it's default value. Also dataKey might be a better name.

renamed key var

ADmad · 2020-05-13T16:17:20Z

src/Authenticator/JwtAuthenticator.php


-        $key = IdentifierInterface::CREDENTIAL_JWT_SUBJECT;
-        if (empty($result[$key])) {
+        $dataKey = $this->getConfig('dataField');


I was suggesting dataKey for the option name, not just the variable name 🙂.

pfff, ok. but this would break backward compatibility? IF anyone have already defined this with the default value of sub, when he will change the value this will not work anymore.

Don't really understand what backwards compatibility concern you have. We are adding a new config, so whether it's named dataField or dataKey really doesn't matter.

That's said let's see if anyone else has other suggestions regarding the new key name.

you are right. it looks its too much for today. I have looking at Authentication.JwtSubject config and have messed up. Yes, thats new config. The only concern that is is that all other components have dataField naming. If thats ok I can change to dataKey.

The only concern that is is that all other components have dataField naming. If thats ok I can change to dataKey.

While naming consistency is important it should be contextual too. Personally I haven't seem the term "field" used in context of JWT. In JWT parlance the sub key is a "claim". So perhaps something more verbose like identityClaimKey might be ever better.

I agree. Maybe subjectClaimKey

claimKey or subjectKey are shorter names that are inline with other JWT jargon.

It would be good to add a test for this configuration option to help prevent regressions in the future.

markstory · 2020-05-15T00:53:38Z

Looking good. Are you able to update the tests or do you need a hand with that?

ADmad · 2020-05-15T16:43:19Z

Targeting the PR to next minor would help document this new option better.

GrandFelix added 2 commits May 13, 2020 10:37

Update JwtAuthenticator.php

194a4cd

use tokenField from config if defined else IdentifierInterface::CREDENTIAL_JWT_SUBJECT

switch tokenField and dataField

2613d07

in conditions must be used dataField

dereuromark reviewed May 13, 2020

View reviewed changes

src/Identifier/TokenIdentifier.php Outdated Show resolved Hide resolved

GrandFelix and others added 2 commits May 13, 2020 12:54

var name changed

8717a8f

update tests

90a2dae

ADmad reviewed May 13, 2020

View reviewed changes

src/Authenticator/JwtAuthenticator.php Outdated Show resolved Hide resolved

ndm2 reviewed May 13, 2020

View reviewed changes

GrandFelix added 3 commits May 13, 2020 17:54

Revert: var name changed (8717a8f)

ca49ec9

renamed config

884d922

Revert: update tests (90a2dae)

6ebccb5

ADmad reviewed May 13, 2020

View reviewed changes

GrandFelix and others added 2 commits May 13, 2020 18:10

moved default value to _defaultConfig

83da4d3

renamed key var

Fixing style errors.

b485ab4

ADmad reviewed May 13, 2020

View reviewed changes

renamed config name

4a18882

updated tests

276c81d

markstory self-assigned this May 16, 2020

markstory merged commit 0ee1b43 into cakephp:master May 17, 2020

luuk-ou added a commit to luuk-ou/authentication that referenced this pull request Jun 8, 2020

backport fix cakephp#376

359ab47

Use tokenField from config if defined #376

Use tokenField from config if defined #376

Uh oh!

Conversation

GrandFelix commented May 13, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ADmad May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ADmad May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ADmad commented May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GrandFelix commented May 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ADmad May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markstory commented May 15, 2020

Uh oh!

ADmad commented May 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ADmad May 13, 2020 •

edited

Loading

ADmad May 13, 2020 •

edited

Loading

ADmad commented May 13, 2020 •

edited

Loading

ADmad May 13, 2020 •

edited

Loading