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

@@ -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",

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;
         }
 

src/test/edu/wisc/doit/FederatedPreauthUserDetailsProviderTest.php → src/test/edu/wisc/doit/HTTPFederatedPreauthUserDetailsProviderTest.php

@@ -6,7 +6,7 @@ use edu\wisc\doit\FederatedPreauthUserDetailsProvider;
 /**
  * Tests for {@link FederatedPreauthUserDetailsProvider}.
  */
-class FederatedPreauthUserDetailsProviderTest extends \PHPUnit_Framework_TestCase
+class HTTPFederatedPreauthUserDetailsProviderTest extends \PHPUnit_Framework_TestCase
 {
 
     /** @var array */
@@ -21,18 +21,28 @@ class FederatedPreauthUserDetailsProviderTest extends \PHPUnit_Framework_TestCas
     protected function setUp()
     {
         parent::setUp();
-        $jsonString = file_get_contents(__DIR__ . "/../../../resources/testuser.json");
+        $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()
     {
-        $this->setupHttpPreauthEnvironment();
-        $this->userProvider = new FederatedPreauthUserDetailsProvider(true);
         $user = $this->userProvider->loadUser();
         $this->assertNotNull($user);
         $this->assertEquals("bbadger@wisc.edu", $user->getEppn());
@@ -47,56 +57,23 @@ class FederatedPreauthUserDetailsProviderTest extends \PHPUnit_Framework_TestCas
 
     public function testLoadUserWithNoEPPN()
     {
-        $this->setupHttpPreauthEnvironment();
-        $this->userProvider = new FederatedPreauthUserDetailsProvider(true);
         // Clear Shib session ID to simulate no session
-        putenv(UserDetailsProvider::SHIB_SESSION_ID);
+        putenv(UserDetailsProvider::SHIB_SESSION_ID_HTTP);
         $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()
+    private function mapAttribute($attribute)
     {
-        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]);
+        return 'HTTP_' . strtoupper($attribute);
     }
 
 }

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());

src/test/resources/testuser.json → 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

src/test/resources/testuser_http.json

+{
+  "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