From f5aa2687f59479952439a0b9ae2e4a43bb021a59 Mon Sep 17 00:00:00 2001
From: Andy Summers <andrew.summers@wisc.edu>
Date: Wed, 27 Jul 2016 10:31:11 -0500
Subject: [PATCH] Fix bug with non-HTTP Shib sessions being valid for HTTP
sessions
Previously, the `PreauthUserDetailsProvider` was only checking that a
valid Shib session existed by looking for the regular or HTTP Shib
session header. This check is now strengthened by validating the
correct header exists for the correct instance.
---
composer.json | 2 +-
.../FederatedPreauthUserDetailsProvider.php | 4 +-
...ederatedPreauthUserDetailsProviderTest.php | 102 ------------------
...ederatedPreauthUserDetailsProviderTest.php | 79 ++++++++++++++
.../doit/LocalUserDetailsProviderTest.php | 2 +-
.../{testuser.json => localuser.json} | 3 +-
src/test/resources/testuser_http.json | 16 +++
7 files changed, 100 insertions(+), 108 deletions(-)
delete mode 100644 src/test/edu/wisc/doit/FederatedPreauthUserDetailsProviderTest.php
create mode 100644 src/test/edu/wisc/doit/HTTPFederatedPreauthUserDetailsProviderTest.php
rename src/test/resources/{testuser.json => localuser.json} (84%)
create mode 100644 src/test/resources/testuser_http.json
diff --git a/composer.json b/composer.json
index 0e153b2..c60f3b3 100644
--- a/composer.json
+++ b/composer.json
@@ -4,7 +4,7 @@
"type": "library",
"homepage": "https://git.doit.wisc.edu/adi-ia/uw-php-security",
"license": "Apache-2.0",
- "version": "1.0.2",
+ "version": "1.0.3",
"authors": [{
"name": "UW-Madison DoIT ADI Integrated Applications",
"email": "adi-ia@lists.wisc.edu",
diff --git a/src/main/edu/wisc/doit/FederatedPreauthUserDetailsProvider.php b/src/main/edu/wisc/doit/FederatedPreauthUserDetailsProvider.php
index 94ea410..a0c17d4 100644
--- a/src/main/edu/wisc/doit/FederatedPreauthUserDetailsProvider.php
+++ b/src/main/edu/wisc/doit/FederatedPreauthUserDetailsProvider.php
@@ -28,8 +28,8 @@ class FederatedPreauthUserDetailsProvider implements UserDetailsProvider
public function loadUser()
{
// Return null if no Shib session is found
- if (empty(getenv(FederatedPreauthUserDetailsProvider::SHIB_SESSION_ID)) &&
- empty(getenv(FederatedPreauthUserDetailsProvider::SHIB_SESSION_ID_HTTP))) {
+ if ($this->httpHeaders && !getenv(FederatedPreauthUserDetailsProvider::SHIB_SESSION_ID_HTTP) ||
+ !$this->httpHeaders && !getenv(FederatedPreauthUserDetailsProvider::SHIB_SESSION_ID)) {
return null;
}
diff --git a/src/test/edu/wisc/doit/FederatedPreauthUserDetailsProviderTest.php b/src/test/edu/wisc/doit/FederatedPreauthUserDetailsProviderTest.php
deleted file mode 100644
index 4accd8e..0000000
--- a/src/test/edu/wisc/doit/FederatedPreauthUserDetailsProviderTest.php
+++ /dev/null
@@ -1,102 +0,0 @@
-<?php
-
-use edu\wisc\doit\UserDetailsProvider;
-use edu\wisc\doit\FederatedPreauthUserDetailsProvider;
-
-/**
- * Tests for {@link FederatedPreauthUserDetailsProvider}.
- */
-class FederatedPreauthUserDetailsProviderTest extends \PHPUnit_Framework_TestCase
-{
-
- /** @var array */
- private $attributes;
-
- /** @var UserDetailsProvider */
- private $userProvider;
-
- /**
- * Populate putenv with Shib attributes to simulate a logged in user
- */
- protected function setUp()
- {
- parent::setUp();
- $jsonString = file_get_contents(__DIR__ . "/../../../resources/testuser.json");
- if ($jsonString === false) {
- return null;
- }
-
- $this->attributes = json_decode($jsonString, true);
- }
-
- public function testLoadUser()
- {
- $this->setupHttpPreauthEnvironment();
- $this->userProvider = new FederatedPreauthUserDetailsProvider(true);
- $user = $this->userProvider->loadUser();
- $this->assertNotNull($user);
- $this->assertEquals("bbadger@wisc.edu", $user->getEppn());
- $this->assertEquals("UW123A456", $user->getPvi());
- $this->assertEquals("BUCKINGHAM BADGER", $user->getFullName());
- $this->assertEquals("bucky.badger@wisc.edu", $user->getEmailAddress());
- $this->assertEquals("a_source", $user->getSource());
- $this->assertEquals("123456789", $user->getIsisEmplid());
- $this->assertEquals("BUCKINGHAM", $user->getFirstName());
- $this->assertEquals("BADGER", $user->getLastName());
- }
-
- public function testLoadUserWithNoEPPN()
- {
- $this->setupHttpPreauthEnvironment();
- $this->userProvider = new FederatedPreauthUserDetailsProvider(true);
- // Clear Shib session ID to simulate no session
- putenv(UserDetailsProvider::SHIB_SESSION_ID);
- $user = $this->userProvider->loadUser();
- $this->assertNull($user);
- }
-
- public function testLoadUserWithNoEmail()
- {
- $this->setupHttpPreauthEnvironment();
- $this->userProvider = new FederatedPreauthUserDetailsProvider(true);
- // Clear email to simulate no email
- putenv('HTTP_' . strtoupper(UserDetailsProvider::FED_EMAIL));
- $user = $this->userProvider->loadUser();
- $this->assertFalse($user->getEmailAddress());
- }
-
- /**
- * Setup environment to simulate a Preauth (Shib) environment.
- */
- private function setupPreauthEnvironment()
- {
- putenv(UserDetailsProvider::FED_EPPN . '=' . $this->attributes[UserDetailsProvider::FED_EPPN]);
- putenv(UserDetailsProvider::FED_SPVI . '=' . $this->attributes[UserDetailsProvider::FED_SPVI]);
- putenv(UserDetailsProvider::FED_FULLNAME . '=' . $this->attributes[UserDetailsProvider::FED_FULLNAME]);
- putenv(UserDetailsProvider::FED_FIRST_NAME . '=' . $this->attributes[UserDetailsProvider::FED_FIRST_NAME]);
- putenv(UserDetailsProvider::FED_LAST_NAME . '=' . $this->attributes[UserDetailsProvider::FED_LAST_NAME]);
- putenv(UserDetailsProvider::UDDS . '=' . implode(",", $this->attributes[UserDetailsProvider::UDDS]));
- putenv(UserDetailsProvider::FED_EMAIL . '=' . $this->attributes[UserDetailsProvider::FED_EMAIL]);
- putenv(UserDetailsProvider::SOURCE . '=' . $this->attributes[UserDetailsProvider::SOURCE]);
- putenv(UserDetailsProvider::ISIS_EMPLID . '=' . $this->attributes[UserDetailsProvider::ISIS_EMPLID]);
- putenv(UserDetailsProvider::SHIB_SESSION_ID . '=' . $this->attributes[UserDetailsProvider::SHIB_SESSION_ID]);
- }
-
- /**
- * Setup environment to simulate HTTP Preauth.
- */
- private function setupHttpPreauthEnvironment()
- {
- putenv('HTTP_' . strtoupper(UserDetailsProvider::FED_EPPN) . '=' . $this->attributes[UserDetailsProvider::FED_EPPN]);
- putenv('HTTP_' . strtoupper(UserDetailsProvider::FED_SPVI) . '=' . $this->attributes[UserDetailsProvider::FED_SPVI]);
- putenv('HTTP_' . strtoupper(UserDetailsProvider::FED_FULLNAME) . '=' . $this->attributes[UserDetailsProvider::FED_FULLNAME]);
- putenv('HTTP_' . strtoupper(UserDetailsProvider::FED_FIRST_NAME) . '=' . $this->attributes[UserDetailsProvider::FED_FIRST_NAME]);
- putenv('HTTP_' . strtoupper(UserDetailsProvider::FED_LAST_NAME) . '=' . $this->attributes[UserDetailsProvider::FED_LAST_NAME]);
- putenv('HTTP_' . strtoupper(UserDetailsProvider::UDDS) . '=' . implode(",", $this->attributes[UserDetailsProvider::UDDS]));
- putenv('HTTP_' . strtoupper(UserDetailsProvider::FED_EMAIL) . '=' . $this->attributes[UserDetailsProvider::FED_EMAIL]);
- putenv('HTTP_' . strtoupper(UserDetailsProvider::SOURCE) . '=' . $this->attributes[UserDetailsProvider::SOURCE]);
- putenv('HTTP_' . strtoupper(UserDetailsProvider::ISIS_EMPLID) . '=' . $this->attributes[UserDetailsProvider::ISIS_EMPLID]);
- putenv(UserDetailsProvider::SHIB_SESSION_ID . '=' . $this->attributes[UserDetailsProvider::SHIB_SESSION_ID]);
- }
-
-}
diff --git a/src/test/edu/wisc/doit/HTTPFederatedPreauthUserDetailsProviderTest.php b/src/test/edu/wisc/doit/HTTPFederatedPreauthUserDetailsProviderTest.php
new file mode 100644
index 0000000..683915a
--- /dev/null
+++ b/src/test/edu/wisc/doit/HTTPFederatedPreauthUserDetailsProviderTest.php
@@ -0,0 +1,79 @@
+<?php
+
+use edu\wisc\doit\UserDetailsProvider;
+use edu\wisc\doit\FederatedPreauthUserDetailsProvider;
+
+/**
+ * Tests for {@link FederatedPreauthUserDetailsProvider}.
+ */
+class HTTPFederatedPreauthUserDetailsProviderTest extends \PHPUnit_Framework_TestCase
+{
+
+ /** @var array */
+ private $attributes;
+
+ /** @var UserDetailsProvider */
+ private $userProvider;
+
+ /**
+ * Populate putenv with Shib attributes to simulate a logged in user
+ */
+ protected function setUp()
+ {
+ parent::setUp();
+ $jsonString = file_get_contents(__DIR__ . "/../../../resources/testuser_http.json");
+ if ($jsonString === false) {
+ return null;
+ }
+
+ $this->attributes = json_decode($jsonString, true);
+ $this->userProvider = new FederatedPreauthUserDetailsProvider(true);
+
+ putenv($this->mapAttribute(UserDetailsProvider::FED_EPPN) . '=' . $this->attributes[$this->mapAttribute(UserDetailsProvider::FED_EPPN)]);
+ putenv($this->mapAttribute(UserDetailsProvider::FED_SPVI) . '=' . $this->attributes[$this->mapAttribute(UserDetailsProvider::FED_SPVI)]);
+ putenv($this->mapAttribute(UserDetailsProvider::FED_FULLNAME) . '=' . $this->attributes[$this->mapAttribute(UserDetailsProvider::FED_FULLNAME)]);
+ putenv($this->mapAttribute(UserDetailsProvider::FED_FIRST_NAME) . '=' . $this->attributes[$this->mapAttribute(UserDetailsProvider::FED_FIRST_NAME)]);
+ putenv($this->mapAttribute(UserDetailsProvider::FED_LAST_NAME) . '=' . $this->attributes[$this->mapAttribute(UserDetailsProvider::FED_LAST_NAME)]);
+ putenv($this->mapAttribute(UserDetailsProvider::UDDS) . '=' . implode(",", $this->attributes[$this->mapAttribute(UserDetailsProvider::UDDS)]));
+ putenv($this->mapAttribute(UserDetailsProvider::FED_EMAIL) . '=' . $this->attributes[$this->mapAttribute(UserDetailsProvider::FED_EMAIL)]);
+ putenv($this->mapAttribute(UserDetailsProvider::SOURCE) . '=' . $this->attributes[$this->mapAttribute(UserDetailsProvider::SOURCE)]);
+ putenv($this->mapAttribute(UserDetailsProvider::ISIS_EMPLID) . '=' . $this->attributes[$this->mapAttribute(UserDetailsProvider::ISIS_EMPLID)]);
+ putenv(UserDetailsProvider::SHIB_SESSION_ID_HTTP . '=' . $this->attributes[UserDetailsProvider::SHIB_SESSION_ID_HTTP]);
+ }
+
+ public function testLoadUser()
+ {
+ $user = $this->userProvider->loadUser();
+ $this->assertNotNull($user);
+ $this->assertEquals("bbadger@wisc.edu", $user->getEppn());
+ $this->assertEquals("UW123A456", $user->getPvi());
+ $this->assertEquals("BUCKINGHAM BADGER", $user->getFullName());
+ $this->assertEquals("bucky.badger@wisc.edu", $user->getEmailAddress());
+ $this->assertEquals("a_source", $user->getSource());
+ $this->assertEquals("123456789", $user->getIsisEmplid());
+ $this->assertEquals("BUCKINGHAM", $user->getFirstName());
+ $this->assertEquals("BADGER", $user->getLastName());
+ }
+
+ public function testLoadUserWithNoEPPN()
+ {
+ // Clear Shib session ID to simulate no session
+ putenv(UserDetailsProvider::SHIB_SESSION_ID_HTTP);
+ $user = $this->userProvider->loadUser();
+ $this->assertNull($user);
+ }
+
+ public function testLoadUserWithNoEmail()
+ {
+ // Clear email to simulate no email
+ putenv('HTTP_' . strtoupper(UserDetailsProvider::FED_EMAIL));
+ $user = $this->userProvider->loadUser();
+ $this->assertFalse($user->getEmailAddress());
+ }
+
+ private function mapAttribute($attribute)
+ {
+ return 'HTTP_' . strtoupper($attribute);
+ }
+
+}
diff --git a/src/test/edu/wisc/doit/LocalUserDetailsProviderTest.php b/src/test/edu/wisc/doit/LocalUserDetailsProviderTest.php
index 175219a..52b4bb8 100644
--- a/src/test/edu/wisc/doit/LocalUserDetailsProviderTest.php
+++ b/src/test/edu/wisc/doit/LocalUserDetailsProviderTest.php
@@ -10,7 +10,7 @@ class LocalUserDetailsProviderTest extends \PHPUnit_Framework_TestCase
public function testLoadUser()
{
- $userDetailsService = new LocalUserDetailsProvider(__DIR__ . "/../../../resources/testuser.json");
+ $userDetailsService = new LocalUserDetailsProvider(__DIR__ . "/../../../resources/localuser.json");
$user = $userDetailsService->loadUser();
$this->assertEquals("bbadger@wisc.edu", $user->getEppn());
$this->assertEquals("UW123A456", $user->getPvi());
diff --git a/src/test/resources/testuser.json b/src/test/resources/localuser.json
similarity index 84%
rename from src/test/resources/testuser.json
rename to src/test/resources/localuser.json
index 57b252d..a3a5344 100644
--- a/src/test/resources/testuser.json
+++ b/src/test/resources/localuser.json
@@ -11,6 +11,5 @@
],
"eduWisconsinEmailAddress": "bucky.badger@wisc.edu",
"source": "a_source",
- "isisEmplid": "123456789",
- "Shib-Session-Id": "1234567890"
+ "isisEmplid": "123456789"
}
\ No newline at end of file
diff --git a/src/test/resources/testuser_http.json b/src/test/resources/testuser_http.json
new file mode 100644
index 0000000..7de908f
--- /dev/null
+++ b/src/test/resources/testuser_http.json
@@ -0,0 +1,16 @@
+{
+ "HTTP_EPPN": "bbadger@wisc.edu",
+ "HTTP_EDUWISCONSINSPVI": "UW123A456",
+ "HTTP_CN": "BUCKINGHAM BADGER",
+ "HTTP_EDUWISCONSINCOMMONNAME": "BUCKINGHAM BADGER",
+ "HTTP_EDUWISCONSINGIVENNAME": "BUCKINGHAM",
+ "HTTP_EDUWISCONSINSURNAME": "BADGER",
+ "HTTP_UDDS": [
+ "UW123A456",
+ "UW234A567"
+ ],
+ "HTTP_EDUWISCONSINEMAILADDRESS": "bucky.badger@wisc.edu",
+ "HTTP_SOURCE": "a_source",
+ "HTTP_ISISEMPLID": "123456789",
+ "HTTP_SHIB_SESSION_ID": "1234567890"
+}
\ No newline at end of file
--
GitLab