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.

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

Attach token name and value to request when persist mode is on #67

Closed
wants to merge 1 commit into from
Closed

Attach token name and value to request when persist mode is on #67

wants to merge 1 commit into from

Conversation

ivandokov
Copy link
Contributor

@ivandokov ivandokov commented Dec 22, 2016

PR #60 introduced persist mode for the token, but there is a BC break. When the persist mode is turned on the token name and value are not attached to the request. This fixes the issue.

PS: @akrabat please add Style CI or something else to format the code in PSR2 because PhpStorm automatically does some whitespace changes and stuff on save and I know from my previous PR that you don't like these.

@@ -159,6 +159,9 @@ public function __invoke(ServerRequestInterface $request, ResponseInterface $res
// Generate new CSRF token if persistentTokenMode is false, or if a valid keyPair has not yet been stored
if (!$this->persistentTokenMode || !$this->loadLastKeyPair()) {
$request = $this->generateNewToken($request);
} elseif ($this->persistentTokenMode) {
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer if this if-block was changed as it is not very easy to read.

if ($this->persistentTokenMode === false) {
     $request = $this->generateNewToken($request);
} else {
   $pair = $this->loadLastKeyPair() ? $this->keyPair : $this->generateToken();
   $request = $this->attachRequestAttributes($request, $pair);
}

Copy link
Member

Choose a reason for hiding this comment

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

/cc @akrabat

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed it's not really easy to read, but I don't see a prettier way to put it right. Any suggestions?

Copy link
Member

@geggleto geggleto left a comment

Choose a reason for hiding this comment

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

See above comments.

@alexweissman
Copy link
Contributor

Oh, interesting - I didn't think about the fact that generateNewToken was attaching the token to the request. I guess since I always retrieve them through the getTokenName and getTokenValue methods, I didn't notice this BC.

Good call on adding unit tests for updating the request object, too. This is a great example of the importance of unit testing.

@akrabat akrabat added this to the 0.9.0 milestone Apr 22, 2017
@akrabat
Copy link
Member

akrabat commented Apr 22, 2017

The fork has gone away?

@ivandokov
Copy link
Contributor Author

ivandokov commented Apr 22, 2017

I accidentally deleted it while cleaning my old repos. I don't think it's a problem unless changes are required. In this case I will re-fork it and make a new PR @akrabat

@geggleto
Copy link
Member

@akrabat this looks okay to merge; but I cannot :D

@akrabat
Copy link
Member

akrabat commented Jul 20, 2017

Sorry, I missed your reply @ivandokov

I get this:

rob@swiftsure ~/Projects/slim-framework/Slim-Csrf  (master)$ hub checkout https://github.com/slimphp/Slim-Csrf/pull/67
Error: that fork is not available anymore

So I think that a branch is needed.

@ivandokov
Copy link
Contributor Author

@akrabat I re-created the PR so you can merge it now. I'm sorry for the trouble.

@ivandokov ivandokov closed this Jul 20, 2017
@akrabat akrabat removed this from the 0.9.0 milestone Oct 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants