I started this week’s work by finishing the integration of cookies into my module. To give you some context, Pubkey Encrypt now uses cookies for temporarily storing the Private key for any user upon login. Previously, we were using sessions for this purpose, but we’ve just shifted to this new approach because Pubkey Encrypt aims to protect a website’s Data-at-Rest in compromised servers. Since sessions get stored in the servers too, the module cannot rely on sessions for keeping any secret information.
Cookies integration in the module looked relatively easy at first. I created an event subscriber for KernelEvents::REQUEST event and called upon the $event->getResponse()->headers->setCookie() function there. But after the module’s behaviour became dependent on values inside $_COOKIE array, all the module tests broke. That’s because the SimpleTest internal browser neither stores cookies in the $_COOKIE array on a curl request nor retains cookies on subsequent curl requests (see issue#457804 - Enable simpletest to manipulate cookies)
After some research, I figured that the way people test code relying on cookies is by manually specifying data in the $_COOKIE array before running the actual test classes. I also found out that upon each curl request, cookies loaded by the SimpleTest internal browser can be fetched from WebTestBase::$cookies. Using this information, I simply created a custom implementation for drupalLogin() in the PubkeyEncryptTestBase class:
/**
* Custom login function to ensure the handling of cookies after a user login.
*
* @param \Drupal\Core\Session\AccountInterface $account
* User object representing the user to log in.
*/
protected function drupalLogin(AccountInterface $account) {
parent::drupalLogin($account);
// When a user logs in, his Private key gets temporarily stored in a cookie
// and should be present there till he logs out. Since SimpleTest by default
// does not provide the functionality of retaining cookies during curl
// requests, hence manually doing it here as it is necessary.
foreach ($this->cookies as $name => $value) {
// We're only concerned with the cookie containing Private key for a user.
if (preg_match('/.private_key/', $name, $matches)) {
$_COOKIE[$name] = urldecode($value['value']);
}
}
}
It should be noted here that WebTestBase::$cookies are url-encoded by default, though it wasn’t initially clear through my research.
This change made the code pass all the tests. After this, I stumbled upon yet another issue that cookies size must not exceed 4000 bytes, otherwise the browsers will start truncating them. The cookie set by Pubkey Encrypt upon a user login typically has a size of around 2000 bytes (the size varies depending upon the size of asymmetric keys chosen during module initialization settings). So I added the functionality of clearing this relevant cookie upon a user logout. Before I had done that, multiple users from a single browser weren’t able to login to the website with Pubkey Encrypt enabled.
After fixing these two issues (see the code here), I manually tested the module’s functionality on Drupal 8.2.x (i.e. the latest dev-branch) and everything seemed to be working absolutely fine. In my weekly meeting with mentors Adam Bergstein (@nerdstein) and Colan Schwartz (@colan), I briefed them about the project progress and we discussed:
-
The issue that reliance on cookies means a website using Pubkey Encrypt should have HTTPS enabled. That’s because cookies are insecure on a HTTP connection. Since our module is expected to run on websites with sensitive content, this requirement seemed reasonable to us.
-
The issue that reliance on cookies means a website using Pubkey Encrypt will load a bit slowly. That’s because cookies get sent by the browser on each HTTP/HTTPS request. Since security always hamper usability in one way or the other, this compromise too seemed reasonable to us.
After that, I spent some time reviewing Mehul Gupta’s GSoC project as a part of our code peer-review agreement. He is also under the supervision of Adam and is working to port the Google Authenticator module to Drupal 8. We started with an informal video chat, via which we communicated to each other our project goals and the work we’ve done so far.
Initially, I passed his code through automated Drupal CodeSniffer inspections. I was delighted to see his adherence to coding standards. There were only a few minor issues, mostly in the comment doc blocks. Then I skimmed his code without testing it in the browser, so to make some sense of it. Lastly, I checked some of the functionalities in his module with a debugger running alongside. I have yet to complete the review process as I aim to read through each and every line of his code, but here are the issues I’ve created so far as a part of the review process:
- Do not alter user.login route by default.
- Allow spaces in code input for improved User Experience.
- Use Real AES Encryption Method instead of relying on a custom one.
- Use t() method instead of relying on the global t() function.
- Use Dependency Injection in forms.
- Adherence to Drupal 8 coding standards.