From 25fbf08f2791b3ad8a1521184677455835009362 Mon Sep 17 00:00:00 2001 From: Greg Corbett Date: Fri, 17 Feb 2023 10:32:24 +0000 Subject: [PATCH 01/10] Add an abstract class for OIDC based integration - it's abstract because the idea is that it will be extended by specific, non-abstract, classes - like a token class to handle EOSC AAI. --- .../AuthTokens/OIDCAuthToken.php | 98 +++++++++++++++++++ 1 file changed, 98 insertions(+) create mode 100644 lib/Authentication/AuthTokens/OIDCAuthToken.php diff --git a/lib/Authentication/AuthTokens/OIDCAuthToken.php b/lib/Authentication/AuthTokens/OIDCAuthToken.php new file mode 100644 index 000000000..7549a7436 --- /dev/null +++ b/lib/Authentication/AuthTokens/OIDCAuthToken.php @@ -0,0 +1,98 @@ +authorities; + } + + /** + * {@see IAuthentication::getCredentials()} + * @return string An empty string as passwords are not used by this token. + */ + public function getCredentials() + { + return ""; // none used in this token, handled by IdP + } + + /** + * A custom object used to store additional user details. + * Allows non-security related user information (such as email addresses, + * telephone numbers etc) to be stored in a convenient location. + * {@see IAuthentication::getDetails()} + * + * @return Object or null if not used + */ + public function getDetails() + { + return $this->userDetails; + } + + /** + * {@see IAuthentication::getPrinciple()} + * @return string unique principle string of user + */ + public function getPrinciple() + { + return $this->principal; + } + + /** + * {@see IAuthentication::setAuthorities($authorities)} + */ + public function setAuthorities($authorities) + { + $this->authorities = $authorities; + } + + /** + * {@see IAuthentication::setDetails($userDetails)} + * @param Object $userDetails + */ + public function setDetails($userDetails) + { + $this->userDetails = $userDetails; + } + + /** + * {@see IAuthentication::validate()} + */ + public function validate() + { + } + + /** + * {@see IAuthentication::isPreAuthenticating()} + */ + public static function isPreAuthenticating() + { + return true; + } + + /** + * Returns true, this token reads the session attributes and so + * does not need to be stateful itself. + * {@see IAuthentication::isStateless()} + */ + public static function isStateless() + { + return true; + } +} From b6d8bd9ad80c3dade4f00a7479165ef3a55a213f Mon Sep 17 00:00:00 2001 From: Greg Corbett Date: Fri, 17 Feb 2023 10:36:36 +0000 Subject: [PATCH 02/10] Add a function to populate the token --- lib/Authentication/AuthTokens/OIDCAuthToken.php | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/lib/Authentication/AuthTokens/OIDCAuthToken.php b/lib/Authentication/AuthTokens/OIDCAuthToken.php index 7549a7436..a7f84ed24 100644 --- a/lib/Authentication/AuthTokens/OIDCAuthToken.php +++ b/lib/Authentication/AuthTokens/OIDCAuthToken.php @@ -7,6 +7,8 @@ abstract class OIDCAuthToken implements IAuthentication private $userDetails = null; private $authorities = array(); private $principal; + protected $acceptedIssuers; + protected $authRealm; /** * {@see IAuthentication::eraseCredentials()} @@ -95,4 +97,17 @@ public static function isStateless() { return true; } + + /** + * Set principal/User details from the session. + */ + protected function setTokenFromSession() + { + if (in_array($_SERVER['OIDC_CLAIM_iss'], $this->acceptedIssuers, true)) { + $this->principal = $_SERVER['REMOTE_USER']; + $this->userDetails = array( + 'AuthenticationRealm' => array($this->authRealm) + ); + } + } } From 59d65ca38e48f60dd3b1aa243dda1bfc443a51d7 Mon Sep 17 00:00:00 2001 From: Greg Corbett Date: Fri, 17 Feb 2023 10:37:47 +0000 Subject: [PATCH 03/10] Add methods to check group memberships --- .../AuthTokens/OIDCAuthToken.php | 83 ++++++++++++++++++- 1 file changed, 82 insertions(+), 1 deletion(-) diff --git a/lib/Authentication/AuthTokens/OIDCAuthToken.php b/lib/Authentication/AuthTokens/OIDCAuthToken.php index a7f84ed24..80555e2ba 100644 --- a/lib/Authentication/AuthTokens/OIDCAuthToken.php +++ b/lib/Authentication/AuthTokens/OIDCAuthToken.php @@ -9,6 +9,11 @@ abstract class OIDCAuthToken implements IAuthentication private $principal; protected $acceptedIssuers; protected $authRealm; + protected $groupHeader; + protected $groupSplitChar; + protected $bannedGroups; + protected $requiredGroups; + protected $helpString; /** * {@see IAuthentication::eraseCredentials()} @@ -99,7 +104,7 @@ public static function isStateless() } /** - * Set principal/User details from the session. + * Set principal/User details from the session and check group membership. */ protected function setTokenFromSession() { @@ -108,6 +113,82 @@ protected function setTokenFromSession() $this->userDetails = array( 'AuthenticationRealm' => array($this->authRealm) ); + + // Check group membership is acceptable. + $this->checkBannedGroups(); + $this->checkRequiredGroups(); + } + } + + /** + * Check the token lists all the required groups. + */ + protected function checkRequiredGroups() + { + $groupArray = explode( + $this->groupSplitChar, + $_SERVER[$this->groupHeader] + ); + + // Build up a list of missing groups. + $missingGoodGroups = []; + foreach ($this->requiredGroups as $group) { + if (!in_array($group, $groupArray)) { + $missingGoodGroups[] = $group; + } + } + + // If the list of missing groups is not empty, reject the user. + if (!empty($missingGoodGroups)) { + $this->rejectUser( + 'You are missing the following group(s):', + $missingGoodGroups + ); + } + } + + /** + * Check the token lists non of the banned groups. + */ + protected function checkBannedGroups() + { + $groupArray = explode($this->groupSplitChar, $_SERVER[$this->groupHeader]); + + $presentBadGroups = []; + foreach ($this->bannedGroups as $group) { + if (in_array($group, $groupArray)) { + $presentBadGroups[] = $group; + } + } + + // If the list of present bad groups is not empty, reject the user. + if (!empty($presentBadGroups)) { + $this->rejectUser( + 'We do not grant access to GOCDB to members of the following group(s):', + $presentBadGroups + ); } } + + /** + * Craft a BadCredentialsException exception. + * + * Uses the given error message to provide the end user more context. + * + * @param string $errorContext Context for the error. + * @param string[] $groupArray An array of group memberships + */ + protected function rejectUser($errorContext, $groupArray) + { + // For readability, when listing groups to the user, + // start each one on a new line with a '-' character. + $prependString = '
- '; + $groupString = implode($prependString, $groupArray); + throw new BadCredentialsException( + null, + 'You do not belong to the correct group(s) ' . + 'to gain access to this site.

' . $errorContext . + $prependString . $groupString . '

' . $this->helpString + ); + } } From c771f70418d6b1ad0eb6d98bc9150df01eb717cf Mon Sep 17 00:00:00 2001 From: Greg Corbett Date: Fri, 17 Feb 2023 10:38:59 +0000 Subject: [PATCH 04/10] Add token for the EOSC Proxy IdPs - currently only supports aai-demo.eosc-portal.eu --- .../AuthTokens/EOSCAAIAuthToken.php | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 lib/Authentication/AuthTokens/EOSCAAIAuthToken.php diff --git a/lib/Authentication/AuthTokens/EOSCAAIAuthToken.php b/lib/Authentication/AuthTokens/EOSCAAIAuthToken.php new file mode 100644 index 000000000..4b6e6080b --- /dev/null +++ b/lib/Authentication/AuthTokens/EOSCAAIAuthToken.php @@ -0,0 +1,31 @@ +acceptedIssuers = array("https://aai-demo.eosc-portal.eu/auth/realms/core"); + $this->authRealm = "EOSC Proxy IdP"; + $this->groupHeader = "OIDC_CLAIM_eduperson_entitlement"; + $this->groupSplitChar = ','; + $this->bannedGroups = array(); + $this->requiredGroups = array("urn:geant:eosc-portal.eu:res:gocdb.eosc-portal.eu"); + $this->helpString = 'Please seek assistance by opening a ticket against the ' . + '"EOSC AAI: Core Infrastructure Proxy" group in ' . + 'https://eosc-helpdesk.eosc-portal.eu/'; + + if (isset($_SERVER['OIDC_access_token'])) { + $this->setTokenFromSession(); + } + } +} From e1c9d7751cbf26d8a4528291f39035f222d5d28c Mon Sep 17 00:00:00 2001 From: Greg Corbett Date: Thu, 16 Feb 2023 16:55:55 +0000 Subject: [PATCH 05/10] Handle exceptions from OIDC based Auth Tokens --- htdocs/web_portal/index.php | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/htdocs/web_portal/index.php b/htdocs/web_portal/index.php index 2e21d49f6..af5b04566 100644 --- a/htdocs/web_portal/index.php +++ b/htdocs/web_portal/index.php @@ -26,6 +26,8 @@ // Require GocContextPath which is used in most of the views scripts require_once __DIR__.'/GocContextPath.php'; +use org\gocdb\security\authentication\BadCredentialsException; + // Set the timezone date_default_timezone_set("UTC"); @@ -84,6 +86,18 @@ function rejectIfNotAuthenticated($message = null){ try { Draw_Page($Page_Type); +} catch (BadCredentialsException $error) { + /** + * `show_view('error.php', ...` is not suitable here. + * - setting raw to FALSE triggers another exception because it tries + * to render a pretty error in a GOCDB window, which fails because the + * user isn't authroised. + * - setting raw to TRUE also isn't ideal as it displays html tags in the + * otherwise nicely formatted output. + * die-ing like this atleast gives the user a somewhart nicely formatted + * error. + */ + die($error->getMessage()); } catch (ErrorException $e) { /* ErrorExceptions may be thrown by an invalid configuration so it is not safe to try to give a pretty output. Set 'raw' to true. */ From c35e4b536a432b2dda41159097ffc50bc2266e25 Mon Sep 17 00:00:00 2001 From: gregcorbett Date: Mon, 20 Feb 2023 17:19:51 +0000 Subject: [PATCH 06/10] Add docstring to OIDCAuthToken --- lib/Authentication/AuthTokens/OIDCAuthToken.php | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/lib/Authentication/AuthTokens/OIDCAuthToken.php b/lib/Authentication/AuthTokens/OIDCAuthToken.php index 80555e2ba..012bdb02c 100644 --- a/lib/Authentication/AuthTokens/OIDCAuthToken.php +++ b/lib/Authentication/AuthTokens/OIDCAuthToken.php @@ -2,6 +2,20 @@ namespace org\gocdb\security\authentication; +/** + * An abstract class for the logic of integrating with IdPs via OIDC. + * + * It is expected that concrete subclasses are created for each + * new IdP GOCDB integrates with via OIDC, providing specific information + * for that IdP. + * + * Any subclass will require installation/config of mod_auth_openidc + * before use. + * + * Any subclass token is expected to be stateless because it relies on the + * mod_auth_openidc session and simply reads the attributes stored in the + * session. + */ abstract class OIDCAuthToken implements IAuthentication { private $userDetails = null; From 7677f3aa216614053628794148dc15f784fd6009 Mon Sep 17 00:00:00 2001 From: gregcorbett Date: Mon, 27 Feb 2023 16:05:21 +0000 Subject: [PATCH 07/10] SPG fix Co-authored-by: Rowan --- lib/Authentication/AuthTokens/OIDCAuthToken.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Authentication/AuthTokens/OIDCAuthToken.php b/lib/Authentication/AuthTokens/OIDCAuthToken.php index 012bdb02c..59406a6da 100644 --- a/lib/Authentication/AuthTokens/OIDCAuthToken.php +++ b/lib/Authentication/AuthTokens/OIDCAuthToken.php @@ -162,7 +162,7 @@ protected function checkRequiredGroups() } /** - * Check the token lists non of the banned groups. + * Check the token lists none of the banned groups. */ protected function checkBannedGroups() { From 0db9af6d688bb4c5a9409b94b6f8b4c07c2b4b91 Mon Sep 17 00:00:00 2001 From: gregcorbett Date: Thu, 23 Mar 2023 14:45:10 +0000 Subject: [PATCH 08/10] Refer to correct argument of show_view --- htdocs/web_portal/index.php | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/htdocs/web_portal/index.php b/htdocs/web_portal/index.php index af5b04566..13bd0ba58 100644 --- a/htdocs/web_portal/index.php +++ b/htdocs/web_portal/index.php @@ -88,19 +88,19 @@ function rejectIfNotAuthenticated($message = null){ } catch (BadCredentialsException $error) { /** - * `show_view('error.php', ...` is not suitable here. - * - setting raw to FALSE triggers another exception because it tries - * to render a pretty error in a GOCDB window, which fails because the - * user isn't authroised. - * - setting raw to TRUE also isn't ideal as it displays html tags in the - * otherwise nicely formatted output. + * `show_view('error.php', ..., $rawOutput)` is not suitable here. + * - setting rawOutput to FALSE triggers another exception because it + * tries to render a pretty error in a GOCDB window, which fails because + * the user isn't authroised. + * - setting rawOutput to TRUE also isn't ideal as it displays html tags + * in the otherwise nicely formatted output. * die-ing like this atleast gives the user a somewhart nicely formatted * error. */ die($error->getMessage()); } catch (ErrorException $e) { /* ErrorExceptions may be thrown by an invalid configuration so it is - not safe to try to give a pretty output. Set 'raw' to true. */ + not safe to try to give a pretty output. Set 'rawOutput' to true. */ show_view('error.php', $e->getMessage(), NULL, TRUE); die(); } catch(Exception $e) { From 4d6f481c4e273916984ecff50abddcef3557d6b9 Mon Sep 17 00:00:00 2001 From: gregcorbett Date: Thu, 23 Mar 2023 14:47:23 +0000 Subject: [PATCH 09/10] SPG fix --- htdocs/web_portal/index.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/htdocs/web_portal/index.php b/htdocs/web_portal/index.php index 13bd0ba58..3a2a012f3 100644 --- a/htdocs/web_portal/index.php +++ b/htdocs/web_portal/index.php @@ -94,7 +94,7 @@ function rejectIfNotAuthenticated($message = null){ * the user isn't authroised. * - setting rawOutput to TRUE also isn't ideal as it displays html tags * in the otherwise nicely formatted output. - * die-ing like this atleast gives the user a somewhart nicely formatted + * die-ing like this atleast gives the user a somewhat nicely formatted * error. */ die($error->getMessage()); From 8e8160b821e0fadd874e8ece715fbf807e150a56 Mon Sep 17 00:00:00 2001 From: gregcorbett Date: Wed, 29 Mar 2023 14:56:51 +0100 Subject: [PATCH 10/10] Shorten long line - it was over 80 characters, which PSR12 says we should not do. - it also makes it more consistent with other lines in this file. Co-authored-by: Rowan --- lib/Authentication/AuthTokens/OIDCAuthToken.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/Authentication/AuthTokens/OIDCAuthToken.php b/lib/Authentication/AuthTokens/OIDCAuthToken.php index 59406a6da..d14f7e5bb 100644 --- a/lib/Authentication/AuthTokens/OIDCAuthToken.php +++ b/lib/Authentication/AuthTokens/OIDCAuthToken.php @@ -166,7 +166,10 @@ protected function checkRequiredGroups() */ protected function checkBannedGroups() { - $groupArray = explode($this->groupSplitChar, $_SERVER[$this->groupHeader]); + $groupArray = explode( + $this->groupSplitChar, + $_SERVER[$this->groupHeader] + ); $presentBadGroups = []; foreach ($this->bannedGroups as $group) {