Skip to content

Commit

Permalink
rework
Browse files Browse the repository at this point in the history
  • Loading branch information
johanib committed Dec 18, 2024
1 parent 8b84379 commit e044ca8
Show file tree
Hide file tree
Showing 15 changed files with 51 additions and 105 deletions.
5 changes: 0 additions & 5 deletions config/services.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,6 @@ services:
arguments:
$errorPageHelper: '@Surfnet\Tiqr\Service\ErrorPageHelper'

Surfnet\Tiqr\Service\TrustedDevice\DateTime\ExpirationHelper:
arguments:
$cookieLifetime: "%trusted_device_cookie_lifetime%"
$gracePeriod: 60

Surfnet\Tiqr\Service\TrustedDevice\Crypto\CryptoHelperInterface:
class: Surfnet\Tiqr\Service\TrustedDevice\Crypto\HaliteCryptoHelper

Expand Down
2 changes: 1 addition & 1 deletion src/Controller/AuthenticationNotificationController.php
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ public function __invoke(Request $request): Response
return $this->generateNotificationResponse('no-trusted-device');
}

if (!$this->trustedDeviceService->isTrustedDevice($cookie, $nameId, $notificationAddress)) {
if ($this->trustedDeviceService->isTrustedDevice($cookie, $nameId, $notificationAddress) === false) {
$this->logger->notice(
sprintf(
'A trusted device cookie is found for notification address "%s" and user "%s", but has signature mismatch',
Expand Down
6 changes: 2 additions & 4 deletions src/Features/Context/TiqrContext.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@
use Surfnet\Tiqr\Service\TrustedDevice\TrustedDeviceService;
use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\Configuration;
use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValue;
use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValueInterface;
use Surfnet\Tiqr\Tiqr\Exception\UserNotExistsException;
use Surfnet\Tiqr\Tiqr\TiqrConfigurationInterface;
use Surfnet\Tiqr\Tiqr\TiqrUserRepositoryInterface;
Expand Down Expand Up @@ -361,7 +360,7 @@ public function weHaveATrustedDevice(string $notificationAddress): void
$request->cookies->set($cookie->getName(), $cookie->getValue());
}
$cookieValue = $this->trustedDeviceService->read($request, $userId, $notificationAddress);
Assertion::isInstanceOf($cookieValue, CookieValueInterface::class);
Assertion::isInstanceOf($cookieValue, CookieValue::class);
Assertion::true($this->trustedDeviceService->isTrustedDevice($cookieValue, $userId, $notificationAddress));
}

Expand Down Expand Up @@ -603,8 +602,7 @@ public function aPushNotificationIsSentWithATrustedDevice(string $notificationAd
$cryptoHelper = new HaliteCryptoHelper($config);

$cookieValue = CookieValue::from($cookieUserId, $notificationAddress);
$helper = new CookieHelper($config, $cryptoHelper, new NullLogger());
$cookieName = $helper->buildCookieName($userId, $notificationAddress);
$cookieName = 'tiqr-trusted-device' . hash('sha256', $userId . '_' . $notificationAddress);

$encryptedValue = $cryptoHelper->encrypt($cookieValue);

Expand Down
2 changes: 1 addition & 1 deletion src/Features/mfaFatigueMitigation.feature
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ Feature: When an user needs to authenticate
Then we have a authenticated app
And we have a trusted cookie for address: "0000000000111111111122222222223333333333"

Scenario: When a user authenticates without a trusted cookie, a notification should not be sent
Scenario: When a user authenticates without a trusted cookie, a push notification should not be sent
Given I am on "/demo/sp"
And I fill in "NameID" with my identifier
When I press "authenticate"
Expand Down
5 changes: 2 additions & 3 deletions src/Service/TrustedDevice/Crypto/CryptoHelperInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,10 @@
namespace Surfnet\Tiqr\Service\TrustedDevice\Crypto;

use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValue;
use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValueInterface;

interface CryptoHelperInterface
{
public function encrypt(CookieValueInterface $cookieValue): string;
public function encrypt(CookieValue $cookieValue): string;

public function decrypt(string $cookieData): CookieValueInterface;
public function decrypt(string $cookieData): CookieValue;
}
5 changes: 2 additions & 3 deletions src/Service/TrustedDevice/Crypto/HaliteCryptoHelper.php
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
use Surfnet\Tiqr\Service\TrustedDevice\Exception\EncryptionFailedException;
use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\Configuration;
use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValue;
use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValueInterface;

class HaliteCryptoHelper implements CryptoHelperInterface
{
Expand All @@ -54,7 +53,7 @@ public function __construct(Configuration $configuration)
* derived key, or the secret key input in the HKDF. Encrypting many messages using the same
* secret key is not a problem in this design.
*/
public function encrypt(CookieValueInterface $cookieValue): string
public function encrypt(CookieValue $cookieValue): string
{
try {
$plainTextCookieValue = new HiddenString($cookieValue->serialize());
Expand All @@ -77,7 +76,7 @@ public function encrypt(CookieValueInterface $cookieValue): string
* Again using the encryption key, used to encrypt the data.
* The decrypt method will return a deserialized CookieValue value object
*/
public function decrypt(string $cookieData): CookieValueInterface
public function decrypt(string $cookieData): CookieValue
{
try {
// Decryption: (we use the default encoding: Halite::DECODE_BASE64URLSAFE)
Expand Down
19 changes: 5 additions & 14 deletions src/Service/TrustedDevice/DateTime/ExpirationHelper.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,25 +23,16 @@
use DateTime as CoreDateTime;
use Surfnet\StepupBundle\DateTime\DateTime;
use Surfnet\Tiqr\Service\TrustedDevice\Exception\InvalidAuthenticationTimeException;
use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValueInterface;
use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\Configuration;
use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValue;
use TypeError;

class ExpirationHelper implements ExpirationHelperInterface
{
private CoreDateTime $now;

public function __construct(
/**
* The trusted device cookie lifetime in seconds
* See: config/openconext/parameters.yaml trusted_device_cookie_lifetime
*/
readonly private int $cookieLifetime,
/**
* The period in seconds that we still acknowledge the
* cookie even tho the expiration was reached. This accounts
* for server time/sync differences that may occur.
*/
readonly private int $gracePeriod,
private readonly Configuration $configuration,
?CoreDateTime $now = null
) {
if ($now === null) {
Expand All @@ -50,7 +41,7 @@ public function __construct(
$this->now = $now;
}

public function isExpired(CookieValueInterface $cookieValue): bool
public function isExpired(CookieValue $cookieValue): bool
{
try {
$authenticationTimestamp = $cookieValue->authenticationTime();
Expand All @@ -75,7 +66,7 @@ public function isExpired(CookieValueInterface $cookieValue): bool
);
}

$expirationTimestamp = $authenticationTimestamp + $this->cookieLifetime + $this->gracePeriod;
$expirationTimestamp = $authenticationTimestamp + $this->configuration->lifetimeInSeconds;
$currentTimestamp = $this->now->getTimestamp();

// Is the current time greater than the expiration time?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

namespace Surfnet\Tiqr\Service\TrustedDevice\DateTime;

use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValueInterface;
use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValue;

/**
* Used to verify if the authentication time from the CookieValue
Expand All @@ -34,5 +34,5 @@
*/
interface ExpirationHelperInterface
{
public function isExpired(CookieValueInterface $cookieValue): bool;
public function isExpired(CookieValue $cookieValue): bool;
}
8 changes: 4 additions & 4 deletions src/Service/TrustedDevice/Http/CookieHelper.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
use Surfnet\Tiqr\Service\TrustedDevice\Crypto\CryptoHelperInterface;
use Surfnet\Tiqr\Service\TrustedDevice\Exception\CookieNotFoundException;
use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\Configuration;
use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValueInterface;
use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValue;
use Symfony\Component\HttpFoundation\Cookie;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
Expand All @@ -40,7 +40,7 @@ public function __construct(
) {
}

public function write(Response $response, CookieValueInterface $value): void
public function write(Response $response, CookieValue $value): void
{
$cookieName = $this->buildCookieName($value->getUserId(), $value->getNotificationAddress());

Expand All @@ -57,7 +57,7 @@ public function write(Response $response, CookieValueInterface $value): void
/**
* Retrieve the current cookie from the Request if it exists.
*/
public function read(Request $request, string $userId, string $notificationAddress): CookieValueInterface
public function read(Request $request, string $userId, string $notificationAddress): CookieValue
{
$cookieName = $this->buildCookieName($userId, $notificationAddress);
if (!$request->cookies->has($cookieName)) {
Expand Down Expand Up @@ -111,7 +111,7 @@ private function getTimestamp(int $expiresInSeconds): int
return $currentTimestamp + $expiresInSeconds;
}

public function buildCookieName(string $userId, string $notificationAddress): string
private function buildCookieName(string $userId, string $notificationAddress): string
{
return $this->configuration->prefix . hash('sha256', $userId . '_' . $notificationAddress);
}
Expand Down
8 changes: 3 additions & 5 deletions src/Service/TrustedDevice/Http/CookieHelperInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,15 @@

namespace Surfnet\Tiqr\Service\TrustedDevice\Http;

use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValueInterface;
use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValue;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;

interface CookieHelperInterface
{
public function write(Response $response, CookieValueInterface $value): void;
public function write(Response $response, CookieValue $value): void;

public function read(Request $request, string $userId, string $notificationAddress): CookieValueInterface;
public function read(Request $request, string $userId, string $notificationAddress): CookieValue;

public function fingerprint(Request $request, string $userId, string $notificationAddress): string;

public function buildCookieName(string $userId, string $notificationAddress): string;
}
19 changes: 8 additions & 11 deletions src/Service/TrustedDevice/TrustedDeviceService.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
use Surfnet\Tiqr\Service\TrustedDevice\Exception\InvalidAuthenticationTimeException;
use Surfnet\Tiqr\Service\TrustedDevice\Http\CookieHelperInterface;
use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValue;
use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValueInterface;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;

Expand All @@ -50,25 +49,18 @@ public function registerTrustedDevice(Response $response, string $userId, string
}

public function isTrustedDevice(
CookieValueInterface $cookie,
CookieValue $cookie,
string $userId,
string $notificationAddress,
): bool {
if (!$this->isCookieValid($cookie, $userId, $notificationAddress)) {
return false;
}

$this->logger->notice('Push-notification MFA is allowed for the device based on the cookie.');
return true;
}


private function store(Response $response, CookieValueInterface $cookieValue): void
{
$this->cookieHelper->write($response, $cookieValue);
}

public function read(Request $request, string $userId, string $notificationAddress): ?CookieValueInterface
public function read(Request $request, string $userId, string $notificationAddress): ?CookieValue
{
try {
return $this->cookieHelper->read($request, $userId, $notificationAddress);
Expand All @@ -87,7 +79,12 @@ public function read(Request $request, string $userId, string $notificationAddre
}
}

private function isCookieValid(CookieValueInterface $cookie, string $userId, string $notificationAddress): bool
private function store(Response $response, CookieValue $cookieValue): void
{
$this->cookieHelper->write($response, $cookieValue);
}

private function isCookieValid(CookieValue $cookie, string $userId, string $notificationAddress): bool
{
if ($cookie instanceof CookieValue && ($cookie->getUserId() !== $userId || $cookie->getNotificationAddress() !== $notificationAddress)) {
$this->logger->error(
Expand Down
7 changes: 2 additions & 5 deletions src/Service/TrustedDevice/ValueObject/CookieValue.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,9 @@
use DateTime;
use InvalidArgumentException;

use Surfnet\Tiqr\Service\TrustedDevice\Exception\InvalidAuthenticationTimeException;

use function strtolower;
use function strtotime;

readonly class CookieValue implements CookieValueInterface
readonly class CookieValue
{
private function __construct(
private string $userId,
Expand All @@ -51,7 +48,7 @@ public static function from(string $userId, string $notificationAddress): self
/**
* @throws \JsonException
*/
public static function deserialize(string $serializedData): CookieValueInterface
public static function deserialize(string $serializedData): CookieValue
{
$data = json_decode($serializedData, true, 512, JSON_THROW_ON_ERROR);

Expand Down
34 changes: 0 additions & 34 deletions src/Service/TrustedDevice/ValueObject/CookieValueInterface.php

This file was deleted.

26 changes: 16 additions & 10 deletions tests/Unit/Service/TrustedCookie/DateTime/ExpirationHelperTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@
use PHPUnit\Framework\TestCase;
use Surfnet\Tiqr\Service\TrustedDevice\DateTime\ExpirationHelper;
use Surfnet\Tiqr\Service\TrustedDevice\Exception\InvalidAuthenticationTimeException;
use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\Configuration;
use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValue;
use Surfnet\Tiqr\Service\TrustedDevice\ValueObject\CookieValueInterface;

class ExpirationHelperTest extends TestCase
{
Expand Down Expand Up @@ -99,24 +99,30 @@ public function expirationExpectations(): array

public function gracePeriodExpectations(): array
{
// Cookie lifetime 3600 with a grace period of 5 seconds
$helper = $this->makeExpirationHelper(3600, time(), 5);
// Cookie lifetime 3600
$helper = $this->makeExpirationHelper(3600, time());
return [
'within grace period (outer bound)' => [false, $helper, $this->makeCookieValue(time() - 3605)],
'within grace period' => [false, $helper, $this->makeCookieValue(time() - 3601)],
'within grace period (lower bound)' => [false, $helper, $this->makeCookieValue(time() - 3600)],
'outside grace period' => [true, $helper, $this->makeCookieValue(time() - 3606)],
'outside grace period' => [true, $helper, $this->makeCookieValue(time() - 3601)],
];
}

private function makeExpirationHelper(int $expirationTime, int $now, int $gracePeriod = 0) : ExpirationHelper
private function makeExpirationHelper(int $expirationTime, int $now) : ExpirationHelper
{
$time = new \DateTime();
$time->setTimestamp($now);
return new ExpirationHelper($expirationTime, $gracePeriod, $time);

$config = new Configuration(
'',
$expirationTime,
'000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f',
'none',
);

return new ExpirationHelper($config, $time);
}

private function makeCookieValue(int $authenticationTime) : CookieValueInterface
private function makeCookieValue(int $authenticationTime) : CookieValue
{
$dateTime = new \DateTime();
$dateTime->setTimestamp($authenticationTime);
Expand All @@ -128,7 +134,7 @@ private function makeCookieValue(int $authenticationTime) : CookieValueInterface
return CookieValue::deserialize(json_encode($data));
}

private function makeCookieValueUnrestrictedAuthTime($authenticationTime) : CookieValueInterface
private function makeCookieValueUnrestrictedAuthTime($authenticationTime) : CookieValue
{
$data = [
'userId' => 'userId',
Expand Down
Loading

0 comments on commit e044ca8

Please sign in to comment.