From f363481ca101a48f216b5aebbbc9c74bb70c5707 Mon Sep 17 00:00:00 2001 From: Aleksander Machniak Date: Sun, 17 Dec 2023 10:08:21 +0100 Subject: [PATCH] CS improvements, fix unwanted output in tests --- program/include/rcmail_oauth.php | 28 ++++++++++++++++------------ tests/Rcmail/Oauth.php | 5 +++++ 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/program/include/rcmail_oauth.php b/program/include/rcmail_oauth.php index 8a73b4bcf7..6c6b8cd7f0 100644 --- a/program/include/rcmail_oauth.php +++ b/program/include/rcmail_oauth.php @@ -426,7 +426,7 @@ public function jwt_decode($jwt) throw new RuntimeException('Failed to validate JWT: expired message'); } - $this->log_debug('jwt: %s', json_encode($body)); + $this->log_debug("'jwt: %s", json_encode($body)); return $body; } @@ -480,7 +480,7 @@ public function login_redirect() 'state' => $_SESSION['oauth_state'], ] + (array) $this->options['auth_parameters']); - $this->log_debug('requesting authorization with scope: %s', $this->options['scope']); + $this->log_debug("requesting authorization with scope: %s", $this->options['scope']); $this->last_error = null; // clean last error $this->rcmail->output->redirect($this->options['auth_uri'] . $delimiter . $query); // exit @@ -541,11 +541,13 @@ public function request_access_token($auth_code, $state = null) // validate state parameter against $_SESSION['oauth_state'] if (!isset($_SESSION['oauth_state']) || ($_SESSION['oauth_state'] !== $state)) { - throw new RuntimeException('state parameter mismatch'); + throw new RuntimeException("state parameter mismatch"); } + $this->rcmail->session->remove('oauth_state'); - $this->log_debug('requesting a grant_type=authorization_code to %s', $oauth_token_uri); + $this->log_debug("requesting a grant_type=authorization_code to %s", $oauth_token_uri); + $response = $this->http_client->post($oauth_token_uri, [ 'form_params' => [ 'grant_type' => 'authorization_code', @@ -555,6 +557,7 @@ public function request_access_token($auth_code, $state = null) 'redirect_uri' => $this->get_redirect_uri(), ], ]); + $data = json_decode($response->getBody(), true); $authorization = $this->parse_tokens('authorization_code', $data); @@ -582,7 +585,6 @@ public function request_access_token($auth_code, $state = null) $this->log_debug("fetched identity: %s", json_encode($fetched_identity, true)); if (!empty($fetched_identity)) { - $identity = $fetched_identity; foreach ($this->options['identity_fields'] as $field) { @@ -663,7 +665,8 @@ public function refresh_access_token(array $token) // send token request to get a real access token for the given auth code try { - $this->log_debug('requesting a grant_type=refresh_token to %s', $oauth_token_uri); + $this->log_debug("requesting a grant_type=refresh_token to %s", $oauth_token_uri); + $response = $this->http_client->post($oauth_token_uri, [ 'form_params' => [ 'grant_type' => 'refresh_token', @@ -779,7 +782,8 @@ protected function is_token_revoked($token) protected function parse_tokens($grant_type, &$data, $previous_data=null) { // TODO move it into to log_info ? - $this->log_debug('received tokens from a grant request "%s": session: %s with scope %s, access_token type %s exp in %ss, refresh_token exp in %ss, id_token present: %s, not-before-policy: %s', + $this->log_debug("received tokens from a grant request %s: session: %s with scope %s, " + . "access_token type %s exp in %ss, refresh_token exp in %ss, id_token present: %s, not-before-policy: %s", $grant_type, $data['session_state'], $data['scope'], $data['token_type'], $data['expires_in'], @@ -790,7 +794,7 @@ protected function parse_tokens($grant_type, &$data, $previous_data=null) if (is_array($previous_data)) { $this->log_debug( - 'changes: session_state: %s, access_token: %s, refresh_token: %s, id_token: %s', + "changes: session_state: %s, access_token: %s, refresh_token: %s, id_token: %s", isset($previous_data['session_state']) ? $previous_data['session_state'] !== $data['session_state'] : null, isset($previous_data['access_token']) ? $previous_data['access_token'] !== $data['access_token'] : null, isset($previous_data['refresh_token']) ? $previous_data['refresh_token'] !== $data['refresh_token'] : null, @@ -800,17 +804,17 @@ protected function parse_tokens($grant_type, &$data, $previous_data=null) // sanity check, check that payload correctly contains access_token if (empty($data['access_token'])) { - throw new RuntimeException('access_token missing ins answer, error from server'); + throw new RuntimeException("access_token missing ins answer, error from server"); } // sanity check, check that payload correctly contains access_token if (empty($data['refresh_token'])) { - throw new RuntimeException('refresh_token missing ins answer, error from server'); + throw new RuntimeException("refresh_token missing ins answer, error from server"); } // (> 0, it means that all token generated before this timestamp date are compromisd and that we need to download a new version of JWKS) if (!empty($data['not-before-policy']) && $data['not-before-policy'] > 0) { - $this->log_debug('all tokens generated before %s timestmp are compromised', $data['not-before-policy']); + $this->log_debug("all tokens generated before %s timestmp are compromised", $data['not-before-policy']); // TODO } @@ -1085,7 +1089,7 @@ public function handle_logout() } $this->logout_redirect_url = $this->options['logout_uri'] . '?' . http_build_query($params); - $this->log_debug('creating logout call: %s', $this->logout_redirect_url); + $this->log_debug("creating logout call: %s", $this->logout_redirect_url); } /** diff --git a/tests/Rcmail/Oauth.php b/tests/Rcmail/Oauth.php index aab2a82984..e547a78228 100644 --- a/tests/Rcmail/Oauth.php +++ b/tests/Rcmail/Oauth.php @@ -214,10 +214,15 @@ function test_request_access_token_with_wrong_state() $oauth->init(); $_SESSION['oauth_state'] = "random-state"; + + StderrMock::start(); $response = $oauth->request_access_token('fake-code', 'mismatch-state'); + StderrMock::stop(); // should be false as state do not match $this->assertFalse($response); + + $this->assertSame("ERROR: OAuth token request failed: state parameter mismatch", trim(StderrMock::$output)); } /**