README.md: Corrected a mistake in the symfony firewall section by galphanet · Pull Request #32 · acquia/http-hmac-php

galphanet · 2017-12-30T18:55:19Z

The name of the firewall has been corrected from wsse to hmac_auth

itafroma · 2018-01-02T15:08:45Z

Hi, thanks for the contribution! Just to clarify, is this the fix for the problem you experienced in #33 or is this intended to be separate from that?

galphanet · 2018-01-02T17:18:49Z

Hi, no it's another one. I'll push the merge request when it will be ready.

galphanet · 2018-01-02T21:57:49Z

OK so now it's ready. My proposed changes are for Symfony:

Added the ResponseSigner service declaration in documentation as it was never called (seems strange...)
base66_encode the secret is not needed in Guzzle init (I send almost 2 days figuring this...)
Added HmacClient to use in PHPUnit testing on a Symfony project

itafroma

Thanks a lot for this! This is very appreciated. In general, I am for these fixes.

If you don't mind, there's some formatting changes, and a couple of minor changes to the HmacClient, I'd like to see get in before merging.

itafroma · 2018-01-03T06:35:18Z


 // A key consists of your UUID and a MIME base64 encoded shared secret.
-$key = new Key('e7fe97fa-a0c8-4a42-ab8e-2c26d52df059', base64_encode('secret'));
+$key = new Key('e7fe97fa-a0c8-4a42-ab8e-2c26d52df059', 'secret');


I'd prefer we have a note here instead of indicating that the secret should be send unencoded. Something like:

// A key consists of your UUID and a Base64-encoded shared secret. // Note: the API provider may have already encoded the secret. In this case, it should not be re-encoded. $key = new Key('e7fe97fa-a0c8-4a42-ab8e-2c26d52df059', base64_encode('secret'));

itafroma · 2018-01-03T06:54:32Z

@@ -0,0 +1,78 @@
+<?php
+
+namespace Acquia\Hmac\Symfony\Tests;


Let's stick this in Acquia\Hmac\Test\Mocks\Symfony and tests/Mocks/Symfony instead: this isn't part of a Symfony bundle, so we don't need to follow that structure.

itafroma · 2018-01-03T07:05:01Z

+use Symfony\Bridge\PsrHttpMessage\Factory\DiactorosFactory;
+use Symfony\Bridge\PsrHttpMessage\Factory\HttpFoundationFactory;
+
+class HmacClient extends Client


Can you add a docblock here briefly describing what this is class is for?

itafroma · 2018-01-03T07:05:09Z

+
+class HmacClient extends Client
+{
+


Remove this space

itafroma · 2018-01-03T07:05:35Z

+     * @param Key $key
+     * @return Key
+     */
+    public function setKey(Key $key) {


Opening brace should be on its own line

itafroma · 2018-01-03T07:24:18Z

+
+        //set the key from the consumer
+        $this->client->setKey(new Key("my-key", "my-not-really-secret"));
+


Remove extra line

itafroma · 2018-01-03T07:24:29Z

+        $this->client = static::createClient();
+
+        //set the key from the consumer
+        $this->client->setKey(new Key("my-key", "my-not-really-secret"));


Use single quotation marks for strings

itafroma · 2018-01-03T07:24:46Z

+    {
+        $this->client = static::createClient();
+
+        //set the key from the consumer


I don't think we need this comment: the code is self-documenting.

itafroma · 2018-01-03T07:25:33Z

 }
 ```

+PHPUnit testing a controller behind HMAC HTTP authentification in Symfony :


Remove space before the colon.

itafroma · 2018-01-03T07:25:53Z

+PHPUnit testing a controller behind HMAC HTTP authentification in Symfony :
+
+1. Add the service declaration:
+


Remove extra line

* master: Add PHP 7.2 support, change PHPUnit namespace, improve test coverage (acquia#38) Bump minor version in README. Update README ACM-484: Add a base64 key/secret encoder (acquia#37) Ensure response body is rewound after signing response (acquia#35)

Issues addressed

itafroma · 2018-03-19T19:13:44Z

Merged with some changes. Thanks again!

README.md: Corrected a mistake in the symfony firewall section

c0af620

galphanet added 3 commits January 2, 2018 22:41

Added HmacClient for unit testing

c8bb42f

README.md: base64_encode($secret) is not needed

439d91f

Added Symfony ResponseSigner service declaration in doc.

9d8493f

Added doc for PHPUnit test case in Symfony

ea08042

itafroma previously requested changes Jan 3, 2018

View reviewed changes

galphanet and others added 7 commits January 15, 2018 11:41

Updated documentation style

a1cab78

Fix namespace of test client

6b32e34

Fix language and spacing

e01db89

Fix docblocks and formatting

27e6552

Fix spacing

1bb66eb

Use responses instead of exceptions to fix LSP violations

952a605

itafroma added this to the 3.4.0 milestone Mar 19, 2018

vladpavlovic approved these changes Mar 19, 2018

View reviewed changes

SteveEdwardsAcquia approved these changes Mar 19, 2018

View reviewed changes

itafroma merged commit ee04f68 into acquia:master Mar 19, 2018


		//set the key from the consumer
		$this->client->setKey(new Key("my-key", "my-not-really-secret"));

		PHPUnit testing a controller behind HMAC HTTP authentification in Symfony :

		1. Add the service declaration:

Uh oh!

Conversation

galphanet commented Dec 30, 2017

Uh oh!

itafroma commented Jan 2, 2018

Uh oh!

galphanet commented Jan 2, 2018

Uh oh!

galphanet commented Jan 2, 2018

Uh oh!

itafroma left a comment

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!

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!

itafroma commented Mar 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants