Adding encryption decorator by prisis · Pull Request #107 · php-cache/cache

prisis · 2016-11-06T23:11:01Z

Question	Answer
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#8
License	MIT
Doc PR

Description

Adding a encryption decorator for psr6

TODO

Add tests
Add documentation
Updated Changelog.md
Rename package from encrypted to encryption

update

All components use the double pipe (php-cache#75)

Update dependencies

update

Nyholm

Awesome. I like this! I just had some minor comments

Nyholm · 2016-11-07T09:14:11Z

+
+### Use
+
+Read the [documentation on usage](http://www.php-cache.com/en/latest/tagging/).


Update link

Nyholm · 2016-11-07T09:14:19Z

+
+### Implement
+
+Read the [documentation on implementation](http://www.php-cache.com/en/latest/implementing-cache-pools/tagging/).


Update link

Nyholm · 2016-11-07T09:14:35Z

+to tag related items, and then clear the cached data for that tag only. It is a part of the PHP Cache organisation. To read about
+features like tagging and hierarchy support please read the shared documentation at [www.php-cache.com](http://www.php-cache.com).
+
+*Note: Performance will be best with a driver such as memcached or redis, which automatically purges stale records.*


Should be removed

Nyholm · 2016-11-07T09:15:09Z

+
+This repository has traits and interfaces that makes a PSR-6 cache implementation encrypted. Using tags allow you
+to tag related items, and then clear the cached data for that tag only. It is a part of the PHP Cache organisation. To read about
+features like tagging and hierarchy support please read the shared documentation at [www.php-cache.com](http://www.php-cache.com).


These rows should be updated/removed.

prisis · 2016-11-07T20:47:29Z

@Nyholm any idea why i cant get testSaveDeferredWhenChangingValues to pass?

Nyholm · 2016-11-07T20:47:41Z

Ill see.

Nyholm · 2016-11-07T20:53:16Z

The testSaveDeferredWhenChangingValues test makes sure that the raw value has not changed on the deferred queue before we do a save.

Since you encrypt the value you break the test. It is the test that is flawed though. I'll fix it.

prisis · 2016-11-07T21:01:39Z

Maybe i can find a workaround

Nyholm

The issue with the deferred save has to do with the cloning. When we get an item from the deferred queue we clone it to make sure it wont be changed. https://github.com/php-cache/cache/blob/0.3/src/Adapter/Common/AbstractCachePool.php#L88

The issue is that we have a CacheItem with reference A and we put a EncryptedItemDecorator with reference B on the deferred queue. When we later call Pool::get we clone the EncryptedItemDecorator to a new place in memory and it gets reference C. Though, EncryptedItemDecorator (C) has still a reference to CacheItem (A).

The EncryptedItemDecorator should implement __clone() to propagate the clone function down to the CacheItem. Something like:

class EncryptedItemDecorator {
  // ...
  public function __clone() {
    $this->cacheItem = clone $this->cacheItem;
  }
}

Nyholm · 2016-11-07T21:57:18Z

        "symfony/options-resolver": "^2.7 || ^3.0"
    },
    "require-dev": {
+        "defuse/php-encryption":   "^2.0",


Is this the best encryption package for us?

Is the best i know, you know a other one?

I have no idea. =)

I trust that you have done some research to find a popular and robust package. And also one that you like.

If you like you can read https://paragonie.com/blog/2016/05/defuse-security-s-php-encryption-library-version-2-0-0-released and if @paragonie-scott recommend it, so it musst be really secure :D

Aside from libsodium, you'll be hard-pressed to do better. @defuse really knocked it out of the park. :)

Nyholm · 2016-11-07T21:58:02Z

+ * with this source code in the file LICENSE.
+ */
+
+namespace Cache\Encryption;


You should rename the folder to "Encryption". Right now all files are in a folder named "Encrypted"

Nyholm · 2016-11-07T21:58:58Z

+use Cache\Taggable\TaggableItemInterface;
+use Defuse\Crypto\Crypto;
+use Defuse\Crypto\Key;
+use Psr\Cache\CacheItemInterface;


You miss

use Psr\Cache\CacheItemPoolInterface;

Applied Style updates (php-cache#108)

prisis · 2016-11-07T23:02:08Z

@Nyholm should i ignore this https://scrutinizer-ci.com/g/php-cache/cache/inspections/a5e25a31-409b-4228-876e-f715b2b126b5/issues/files/src/Encryption/EncryptedItemDecorator.php?status=new&orderField=path&order=asc&honorSelectedPaths=0 ?

Nyholm · 2016-11-07T23:08:58Z

I think we can ignore them for now but this sure is an issue.. let's fix that before making our first tag.

codecov-io · 2016-11-07T23:58:17Z

Current coverage is 83.56% (diff: 89.61%)

Merging #107 into master will increase coverage by 0.46%

@@             master       #107   diff @@
==========================================
  Files            22         24     +2   
  Lines          1006       1083    +77   
  Methods         218        241    +23   
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            836        905    +69   
- Misses          170        178     +8   
  Partials          0          0

Powered by Codecov. Last update 55d7af5...74639aa

cryptiklemur · 2016-11-08T16:49:16Z

+class EncryptedItemDecorator implements CacheItemInterface, HasExpirationDateInterface, TaggableItemInterface
+{
+    /**
+     * @type CacheItemInterface


IIRC these should all be var. I think type was removed from that spec

we can fix it with styleci, we use it in all packages @type

cryptiklemur · 2016-11-08T16:51:25Z

+        }
+    ],
+    "require":     {
+        "php":                   "^5.5 || ^7.0",


i think ^5.5|^7.0 is the "right" way to do this now?

Also, should we continue supporting 5.5? @Nyholm

Lets support 5.5 if we have no reason to drop it.

The current syntax (at least the one in the docs) are dubble pipe.

cryptiklemur · 2016-11-08T16:51:30Z

+        "psr/cache":             "^1.0"
+    },
+    "require-dev": {
+        "phpunit/phpunit":         "^4.0 || ^5.1",


it should be double pipes https://getcomposer.org/doc/articles/versions.md

cryptiklemur · 2016-11-08T16:51:42Z

+<?xml version="1.0" encoding="UTF-8"?>
+
+<phpunit backupGlobals="false"
+  backupStaticAttributes="false"


formatting is strange here

we use the same format in all phpunits

cryptiklemur · 2016-11-08T16:52:10Z

Really like this!

prisis · 2016-11-08T23:08:30Z

So the last thing to do is add documentation

Nyholm

Is this ready for merge?

prisis · 2016-11-09T12:43:01Z

test are all green, only need to add docs for it

prisis added 5 commits July 31, 2016 19:18

Merge pull request #4 from php-cache/master

4ae764f

update

Merge pull request #5 from php-cache/master

a7231cf

All components use the double pipe (php-cache#75)

Merge pull request #6 from php-cache/master

c7c2d7a

Update dependencies

Merge pull request #7 from php-cache/master

5904102

update

Adding encrypted decorator for psr6

7f0d0fc

prisis changed the title ~~Add encrypting decorator~~ [WIP] Add encrypting decorator Nov 6, 2016

fixed testItemModifiersReturnsStatic

ecb4cbc

prisis changed the title ~~[WIP] Add encrypting decorator~~ [WIP] Adding encryption decorator Nov 6, 2016

Nyholm reviewed Nov 7, 2016

View reviewed changes

rename classes | added cs fixes | remove old readme lines

96967dd

This was referenced Nov 7, 2016

Support encrypted cache php-cache/integration-tests#61

Closed

Applied Style updates #108

Merged

Nyholm reviewed Nov 7, 2016

View reviewed changes

prisis added 6 commits November 7, 2016 23:44

Rename folder | fix phpdoc in EncryptedItemDecorator

22e0115

fix last test

975d328

added cs fixes

b951e24

Merge pull request #8 from php-cache/master

f5ed8f0

Applied Style updates (php-cache#108)

Merge branch 'master' into add-encrypting-decorator

588ca24

update php header

e966694

added a Changelog

27bc164

cryptiklemur requested changes Nov 8, 2016

View reviewed changes

fix format

74639aa

cryptiklemur approved these changes Nov 9, 2016

View reviewed changes

Nyholm approved these changes Nov 9, 2016

View reviewed changes

Nyholm merged commit 3e13831 into php-cache:master Nov 9, 2016

prisis deleted the add-encrypting-decorator branch November 9, 2016 21:33

prisis changed the title ~~[WIP] Adding encryption decorator~~ Adding encryption decorator Nov 9, 2016


		### Use

		Read the [documentation on usage](http://www.php-cache.com/en/latest/tagging/).


		### Implement

		Read the [documentation on implementation](http://www.php-cache.com/en/latest/implementing-cache-pools/tagging/).

Conversation

prisis commented Nov 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

TODO

Uh oh!

Nyholm 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!

prisis commented Nov 7, 2016

Uh oh!

Nyholm commented Nov 7, 2016

Uh oh!

Nyholm commented Nov 7, 2016

Uh oh!

prisis commented Nov 7, 2016

Uh oh!

Nyholm 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!

prisis commented Nov 7, 2016

Uh oh!

Nyholm commented Nov 7, 2016

Uh oh!

codecov-io commented Nov 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 83.56% (diff: 89.61%)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prisis Nov 8, 2016 • 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!

Choose a reason for hiding this comment

Uh oh!

cryptiklemur commented Nov 8, 2016

Uh oh!

prisis commented Nov 8, 2016

Uh oh!

Nyholm left a comment

Choose a reason for hiding this comment

Uh oh!

prisis commented Nov 9, 2016

Uh oh!

Reviewers

prisis commented Nov 6, 2016 •

edited

Loading

codecov-io commented Nov 7, 2016 •

edited

Loading

prisis Nov 8, 2016 •

edited

Loading