From 80d9614fc80d5fc53f34d01bb84cbe3784fd9220 Mon Sep 17 00:00:00 2001 From: Davo Date: Wed, 13 Jul 2022 17:02:26 -0500 Subject: [PATCH 1/4] PODB-563 Move type and method constants to ServiceRequest --- src/Interfaces/ILtiServiceConnector.php | 4 +- src/LtiAbstractService.php | 8 ++-- src/LtiAssignmentsGradesService.php | 41 +++++++++++++---- src/LtiCourseGroupsService.php | 10 ++-- src/LtiMessageLaunch.php | 7 ++- src/LtiNamesRolesProvisioningService.php | 5 +- src/LtiServiceConnector.php | 58 ++++++------------------ src/ServiceRequest.php | 50 +++++++++++++++++++- tests/LtiServiceConnectorTest.php | 9 ++-- 9 files changed, 116 insertions(+), 76 deletions(-) diff --git a/src/Interfaces/ILtiServiceConnector.php b/src/Interfaces/ILtiServiceConnector.php index c4bb77b7..62124e96 100644 --- a/src/Interfaces/ILtiServiceConnector.php +++ b/src/Interfaces/ILtiServiceConnector.php @@ -16,7 +16,6 @@ public function makeServiceRequest( ILtiRegistration $registration, array $scopes, IServiceRequest $request, - ?int $requestType = null, bool $shouldRetry = true ): array; @@ -24,8 +23,7 @@ public function getAll( ILtiRegistration $registration, array $scopes, IServiceRequest $request, - string $key, - ?int $requestType = null + string $key ): array; public function setDebuggingMode(bool $enable): void; diff --git a/src/LtiAbstractService.php b/src/LtiAbstractService.php index 3f0eea26..b543b8b4 100644 --- a/src/LtiAbstractService.php +++ b/src/LtiAbstractService.php @@ -36,24 +36,22 @@ public function setServiceData(array $serviceData): self abstract public function getScope(): array; - protected function makeServiceRequest(IServiceRequest $request, ?int $requestType = null): array + protected function makeServiceRequest(IServiceRequest $request): array { return $this->serviceConnector->makeServiceRequest( $this->registration, $this->getScope(), $request, - $requestType ); } - protected function getAll(IServiceRequest $request, string $key = null, ?int $requestType = null): array + protected function getAll(IServiceRequest $request, string $key = null): array { return $this->serviceConnector->getAll( $this->registration, $this->getScope(), $request, - $key, - $requestType + $key ); } } diff --git a/src/LtiAssignmentsGradesService.php b/src/LtiAssignmentsGradesService.php index 277a9e00..2e4ddb61 100644 --- a/src/LtiAssignmentsGradesService.php +++ b/src/LtiAssignmentsGradesService.php @@ -42,11 +42,15 @@ public function putGrade(LtiGrade $grade, LtiLineitem $lineitem = null) $pos = strpos($scoreUrl, '?'); $scoreUrl = $pos === false ? $scoreUrl.'/scores' : substr_replace($scoreUrl, '/scores', $pos, 0); - $request = new ServiceRequest(LtiServiceConnector::METHOD_POST, $scoreUrl); + $request = new ServiceRequest( + ServiceRequest::METHOD_POST, + $scoreUrl, + ServiceRequest::TYPE_SYNC_GRADE + ); $request->setBody($grade); $request->setContentType(static::CONTENTTYPE_SCORE); - return $this->makeServiceRequest($request, LtiServiceConnector::SYNC_GRADE_REQUEST); + return $this->makeServiceRequest($request); } public function findLineItem(LtiLineitem $newLineItem): ?LtiLineitem @@ -64,24 +68,32 @@ public function findLineItem(LtiLineitem $newLineItem): ?LtiLineitem public function updateLineitem(LtiLineItem $lineitemToUpdate): LtiLineitem { - $request = new ServiceRequest(LtiServiceConnector::METHOD_PUT, $this->getServiceData()['lineitems']); + $request = new ServiceRequest( + ServiceRequest::METHOD_PUT, + $this->getServiceData()['lineitems'], + ServiceRequest::TYPE_UPDATE_LINEITEM + ); $request->setBody($lineitemToUpdate) ->setContentType(static::CONTENTTYPE_LINEITEM) ->setAccept(static::CONTENTTYPE_LINEITEM); - $updatedLineitem = $this->makeServiceRequest($request, LtiServiceConnector::UPDATE_LINEITEM_REQUEST); + $updatedLineitem = $this->makeServiceRequest($request); return new LtiLineitem($updatedLineitem['body']); } public function createLineitem(LtiLineitem $newLineItem): LtiLineitem { - $request = new ServiceRequest(LtiServiceConnector::METHOD_POST, $this->getServiceData()['lineitems']); + $request = new ServiceRequest( + ServiceRequest::METHOD_POST, + $this->getServiceData()['lineitems'], + ServiceRequest::TYPE_CREATE_LINEITEM + ); $request->setBody($newLineItem) ->setContentType(static::CONTENTTYPE_LINEITEM) ->setAccept(static::CONTENTTYPE_LINEITEM); - $createdLineItem = $this->makeServiceRequest($request, LtiServiceConnector::CREATE_LINEITEM_REQUEST); + $createdLineItem = $this->makeServiceRequest($request); return new LtiLineitem($createdLineItem['body']); } @@ -100,7 +112,11 @@ public function getGrades(LtiLineitem $lineitem = null) $pos = strpos($resultsUrl, '?'); $resultsUrl = $pos === false ? $resultsUrl.'/results' : substr_replace($resultsUrl, '/results', $pos, 0); - $request = new ServiceRequest(LtiServiceConnector::METHOD_GET, $resultsUrl); + $request = new ServiceRequest( + ServiceRequest::METHOD_GET, + $resultsUrl, + ServiceRequest::TYPE_GET_GRADES + ); $request->setAccept(static::CONTENTTYPE_RESULTCONTAINER); $scores = $this->makeServiceRequest($request); @@ -114,8 +130,9 @@ public function getLineItems(): array } $request = new ServiceRequest( - LtiServiceConnector::METHOD_GET, - $this->getServiceData()['lineitems'] + ServiceRequest::METHOD_GET, + $this->getServiceData()['lineitems'], + ServiceRequest::TYPE_GET_LINEITEMS ); $request->setAccept(static::CONTENTTYPE_LINEITEMCONTAINER); @@ -135,7 +152,11 @@ public function getLineItem(string $url): LtiLineitem throw new LtiException('Missing required scope', 1); } - $request = new ServiceRequest(LtiServiceConnector::METHOD_GET, $url); + $request = new ServiceRequest( + ServiceRequest::METHOD_GET, + $url, + ServiceRequest::TYPE_GET_LINEITEM + ); $request->setAccept(static::CONTENTTYPE_LINEITEM); $response = $this->makeServiceRequest($request)['body']; diff --git a/src/LtiCourseGroupsService.php b/src/LtiCourseGroupsService.php index 9bca7014..4ce8084c 100644 --- a/src/LtiCourseGroupsService.php +++ b/src/LtiCourseGroupsService.php @@ -14,8 +14,9 @@ public function getScope(): array public function getGroups(): array { $request = new ServiceRequest( - LtiServiceConnector::METHOD_GET, - $this->getServiceData()['context_groups_url'] + ServiceRequest::METHOD_GET, + $this->getServiceData()['context_groups_url'], + ServiceRequest::TYPE_GET_GROUPS ); $request->setAccept(static::CONTENTTYPE_CONTEXTGROUPCONTAINER); @@ -30,8 +31,9 @@ public function getSets(): array } $request = new ServiceRequest( - LtiServiceConnector::METHOD_GET, - $this->getServiceData()['context_group_sets_url'] + ServiceRequest::METHOD_GET, + $this->getServiceData()['context_group_sets_url'], + ServiceRequest::TYPE_GET_SETS ); $request->setAccept(static::CONTENTTYPE_CONTEXTGROUPCONTAINER); diff --git a/src/LtiMessageLaunch.php b/src/LtiMessageLaunch.php index 314b2878..d66aaf3c 100644 --- a/src/LtiMessageLaunch.php +++ b/src/LtiMessageLaunch.php @@ -278,8 +278,11 @@ public function getLaunchId() private function getPublicKey() { - $keySetUrl = $this->registration->getKeySetUrl(); - $request = new ServiceRequest(LtiServiceConnector::METHOD_GET, $keySetUrl); + $request = new ServiceRequest( + ServiceRequest::METHOD_GET, + $this->registration->getKeySetUrl(), + ServiceRequest::TYPE_GET_KEYSET + ); // Download key set try { diff --git a/src/LtiNamesRolesProvisioningService.php b/src/LtiNamesRolesProvisioningService.php index 1df1cdd6..afad6d3a 100644 --- a/src/LtiNamesRolesProvisioningService.php +++ b/src/LtiNamesRolesProvisioningService.php @@ -14,8 +14,9 @@ public function getScope(): array public function getMembers(): array { $request = new ServiceRequest( - LtiServiceConnector::METHOD_GET, - $this->getServiceData()['context_memberships_url'] + ServiceRequest::METHOD_GET, + $this->getServiceData()['context_memberships_url'], + ServiceRequest::TYPE_GET_MEMBERSHIPS ); $request->setAccept(static::CONTENTTYPE_MEMBERSHIPCONTAINER); diff --git a/src/LtiServiceConnector.php b/src/LtiServiceConnector.php index c9b753ab..e64e9a1c 100644 --- a/src/LtiServiceConnector.php +++ b/src/LtiServiceConnector.php @@ -15,22 +15,9 @@ class LtiServiceConnector implements ILtiServiceConnector { public const NEXT_PAGE_REGEX = '/<([^>]*)>; ?rel="next"/i'; - public const METHOD_GET = 'GET'; - public const METHOD_POST = 'POST'; - public const METHOD_PUT = 'PUT'; - - // Supported request types which map to an error log message - public const UNSUPPORTED_REQUEST = 0; - public const SYNC_GRADE_REQUEST = 1; - public const CREATE_LINEITEM_REQUEST = 2; - public const GET_LINEITEMS_REQUEST = 3; - public const UPDATE_LINEITEM_REQUEST = 4; - public const AUTH_REQUEST = 5; - private $cache; private $client; private $debuggingMode = false; - private $errorMessages; public function __construct( ICache $cache, @@ -38,15 +25,6 @@ public function __construct( ) { $this->cache = $cache; $this->client = $client; - - $this->errorMessages = [ - static::UNSUPPORTED_REQUEST => 'Logging request data: ', - static::SYNC_GRADE_REQUEST => 'Syncing grade for this lti_user_id: ', - static::CREATE_LINEITEM_REQUEST => 'Creating lineitem: ', - static::GET_LINEITEMS_REQUEST => 'Getting lineitems: ', - static::UPDATE_LINEITEM_REQUEST => 'Updating lineitem: ', - static::AUTH_REQUEST => 'Authenticating: ', - ]; } public function setDebuggingMode(bool $enable): void @@ -86,10 +64,12 @@ public function getAccessToken(ILtiRegistration $registration, array $scopes) 'scope' => implode(' ', $scopes), ]; - $url = $registration->getAuthTokenUrl(); - // Get Access - $request = new ServiceRequest(static::METHOD_POST, $url); + $request = new ServiceRequest( + ServiceRequest::METHOD_POST, + $registration->getAuthTokenUrl(), + ServiceRequest::TYPE_AUTH + ); $request->setPayload(['form_params' => $authRequest]); $response = $this->makeRequest($request); @@ -111,7 +91,6 @@ public function makeRequest(IServiceRequest $request) if ($this->debuggingMode) { $this->logRequest( - static::AUTH_REQUEST, $request, $this->getResponseHeaders($response), $this->getResponseBody($response) @@ -142,15 +121,8 @@ public function makeServiceRequest( ILtiRegistration $registration, array $scopes, IServiceRequest $request, - ?int $requestType = null, bool $shouldRetry = true ): array { - // Set $requestType here, since static properties cannot be evaluated - // as parameters - if (!isset($requestType)) { - $requestType = self::UNSUPPORTED_REQUEST; - } - $request->setAccessToken($this->getAccessToken($registration, $scopes)); try { @@ -164,7 +136,7 @@ public function makeServiceRequest( $key = $this->getAccessTokenCacheKey($registration, $scopes); $this->cache->clearAccessToken($key); - return $this->makeServiceRequest($registration, $scopes, $request, $requestType, false); + return $this->makeServiceRequest($registration, $scopes, $request, false); } throw $e; @@ -182,9 +154,8 @@ public function getAll( array $scopes, IServiceRequest $request, string $key = null, - ?int $requestType = null ): array { - if ($request->getMethod() !== static::METHOD_GET) { + if ($request->getMethod() !== ServiceRequest::METHOD_GET) { throw new \Exception('An invalid method was specified by an LTI service requesting all items.'); } @@ -192,7 +163,7 @@ public function getAll( $nextUrl = $request->getUrl(); while ($nextUrl) { - $response = $this->makeServiceRequest($registration, $scopes, $request, $requestType); + $response = $this->makeServiceRequest($registration, $scopes, $request); $page_results = $key === null ? ($response['body'] ?? []) : ($response['body'][$key] ?? []); $results = array_merge($results, $page_results); @@ -207,7 +178,6 @@ public function getAll( } private function logRequest( - int $requestType, IServiceRequest $request, array $responseHeaders, ?array $responseBody @@ -219,17 +189,17 @@ private function logRequest( 'response_body' => json_encode($responseBody), ]; - $requestBody = $request->getPayload()['body'] ?? ''; + $requestBody = $request->getPayload()['body'] ?? null; if (!empty($requestBody)) { $contextArray['request_body'] = $requestBody; } - $userId = json_decode($requestBody)->userId ?? ''; - - $logMsg = $this->errorMessages[$requestType]; - - error_log($logMsg.$userId.' '.print_r($contextArray, true)); + error_log(implode(' ', array_filter([ + $request->getErrorPrefix(), + json_decode($requestBody)->userId ?? null, + print_r($contextArray, true), + ]))); } private function getAccessTokenCacheKey(ILtiRegistration $registration, array $scopes) diff --git a/src/ServiceRequest.php b/src/ServiceRequest.php index 2324537b..177ad041 100644 --- a/src/ServiceRequest.php +++ b/src/ServiceRequest.php @@ -6,18 +6,43 @@ class ServiceRequest implements IServiceRequest { + // Request methods + public const METHOD_GET = 'GET'; + public const METHOD_POST = 'POST'; + public const METHOD_PUT = 'PUT'; + + // Request types + public const TYPE_UNSUPPORTED = 'unsupported'; + public const TYPE_AUTH = 'auth'; + // MessageLaunch + public const TYPE_GET_KEYSET = 'get_keyset'; + // AGS + public const TYPE_GET_GRADES = 'get_grades'; + public const TYPE_SYNC_GRADE = 'sync_grades'; + public const TYPE_CREATE_LINEITEM = 'create_lineitem'; + public const TYPE_GET_LINEITEMS = 'get_lineitems'; + public const TYPE_GET_LINEITEM = 'get_lineitem'; + public const TYPE_UPDATE_LINEITEM = 'update_lineitem'; + // CGS + public const TYPE_GET_GROUPS = 'get_groups'; + public const TYPE_GET_SETS = 'get_sets'; + // NRPS + public const TYPE_GET_MEMBERSHIPS = 'get_memberships'; + private $method; private $url; + private $type; private $body; private $payload; private $accessToken; private $contentType = 'application/json'; private $accept = 'application/json'; - public function __construct(string $method, string $url) + public function __construct(string $method, string $url, $type = self::UNSUPPORTED) { $this->method = $method; $this->url = $url; + $this->type = $type; } public function getMethod(): string @@ -90,6 +115,27 @@ public function setContentType(string $contentType): IServiceRequest return $this; } + public function getErrorPrefix(): string + { + $defaultMessage = 'Logging request data:'; + $errorMessages = [ + static::TYPE_UNSUPPORTED => $defaultMessage, + static::TYPE_AUTH => 'Authenticating:', + static::TYPE_GET_KEYSET => 'Getting key set:', + static::TYPE_GET_GRADES => 'Getting grades:', + static::TYPE_SYNC_GRADE => 'Syncing grade for this lti_user_id:', + static::TYPE_CREATE_LINEITEM => 'Creating lineitem:', + static::TYPE_GET_LINEITEMS => 'Getting lineitems:', + static::TYPE_GET_LINEITEM => 'Getting a lineitem:', + static::TYPE_UPDATE_LINEITEM => 'Updating lineitem:', + static::TYPE_GET_GROUPS => 'Getting groups:', + static::TYPE_GET_SETS => 'Getting sets:', + static::TYPE_GET_MEMBERSHIPS => 'Getting memberships', + ]; + + return $errorMessages[$this->type] ?? $defaultMessage; + } + private function getHeaders(): array { $headers = [ @@ -101,7 +147,7 @@ private function getHeaders(): array } // Include Content-Type for POST and PUT requests - if (in_array($this->getMethod(), [LtiServiceConnector::METHOD_POST, LtiServiceConnector::METHOD_PUT])) { + if (in_array($this->getMethod(), [ServiceRequest::METHOD_POST, ServiceRequest::METHOD_PUT])) { $headers['Content-Type'] = $this->contentType; } diff --git a/tests/LtiServiceConnectorTest.php b/tests/LtiServiceConnectorTest.php index 977ec53c..95c66b09 100644 --- a/tests/LtiServiceConnectorTest.php +++ b/tests/LtiServiceConnectorTest.php @@ -11,6 +11,7 @@ use Packback\Lti1p3\Interfaces\IServiceRequest; use Packback\Lti1p3\LtiRegistration; use Packback\Lti1p3\LtiServiceConnector; +use Packback\Lti1p3\ServiceRequest; use Psr\Http\Message\StreamInterface; class LtiServiceConnectorTest extends TestCase @@ -47,7 +48,7 @@ public function setUp(): void $this->scopes = ['scopeKey']; $this->token = 'TokenOfAccess'; - $this->method = LtiServiceConnector::METHOD_POST; + $this->method = ServiceRequest::METHOD_POST; $this->url = 'https://example.com'; $this->body = json_encode(['userId' => 'id']); $this->requestHeaders = [ @@ -144,7 +145,7 @@ public function testItMakesAServiceRequest() public function testItRetriesServiceRequestOn401Error() { - $this->method = LtiServiceConnector::METHOD_POST; + $this->method = ServiceRequest::METHOD_POST; $this->url = 'https://example.com'; $this->body = json_encode(['userId' => 'id']); $this->requestHeaders = [ @@ -214,7 +215,7 @@ public function testItRetriesServiceRequestOn401Error() public function testItThrowsOnRepeated401Errors() { - $this->method = LtiServiceConnector::METHOD_POST; + $this->method = ServiceRequest::METHOD_POST; $this->url = 'https://example.com'; $this->body = json_encode(['post' => 'body']); $this->requestHeaders = [ @@ -271,7 +272,7 @@ public function testItThrowsOnRepeated401Errors() public function testItGetsAll() { - $method = LtiServiceConnector::METHOD_GET; + $method = ServiceRequest::METHOD_GET; $key = 'lineitems'; $lineitems = ['lineitem']; $firstResponseHeaders = [ From fc258e0b6c859db6343a3ab789b3b376b83c67d1 Mon Sep 17 00:00:00 2001 From: Davo Date: Wed, 13 Jul 2022 17:11:10 -0500 Subject: [PATCH 2/4] PODB-563 Update the interface --- src/Interfaces/IServiceRequest.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Interfaces/IServiceRequest.php b/src/Interfaces/IServiceRequest.php index 9b3e012b..a7aaadc9 100644 --- a/src/Interfaces/IServiceRequest.php +++ b/src/Interfaces/IServiceRequest.php @@ -19,4 +19,6 @@ public function setBody(string $body): self; public function setAccept(string $accept): self; public function setContentType(string $contentType): self; + + public function getErrorPrefix(): string; } From 345be3278f9d7f19220993732dd9a2719821243c Mon Sep 17 00:00:00 2001 From: Davo Date: Thu, 14 Jul 2022 10:13:45 -0500 Subject: [PATCH 3/4] PODB-563 fix a ref to the old constant --- src/LtiAssignmentsGradesService.php | 2 +- src/LtiServiceConnector.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/LtiAssignmentsGradesService.php b/src/LtiAssignmentsGradesService.php index 2e4ddb61..13bdcd00 100644 --- a/src/LtiAssignmentsGradesService.php +++ b/src/LtiAssignmentsGradesService.php @@ -136,7 +136,7 @@ public function getLineItems(): array ); $request->setAccept(static::CONTENTTYPE_LINEITEMCONTAINER); - $lineitems = $this->getAll($request, null, LtiServiceConnector::GET_LINEITEMS_REQUEST); + $lineitems = $this->getAll($request); // If there is only one item, then wrap it in an array so the foreach works if (isset($lineitems['body']['id'])) { diff --git a/src/LtiServiceConnector.php b/src/LtiServiceConnector.php index e64e9a1c..14fd28fd 100644 --- a/src/LtiServiceConnector.php +++ b/src/LtiServiceConnector.php @@ -213,7 +213,7 @@ private function getAccessTokenCacheKey(ILtiRegistration $registration, array $s private function getNextUrl(array $headers) { $subject = $headers['Link'] ?? ''; - preg_match(LtiServiceConnector::NEXT_PAGE_REGEX, $subject, $matches); + preg_match(static::NEXT_PAGE_REGEX, $subject, $matches); return $matches[1] ?? null; } From 1c166eadd334a23318bff1b9e2ed839908b22d75 Mon Sep 17 00:00:00 2001 From: Davo Date: Thu, 14 Jul 2022 10:52:40 -0500 Subject: [PATCH 4/4] PODB-563 typo --- src/ServiceRequest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ServiceRequest.php b/src/ServiceRequest.php index 177ad041..6e1a330e 100644 --- a/src/ServiceRequest.php +++ b/src/ServiceRequest.php @@ -130,7 +130,7 @@ public function getErrorPrefix(): string static::TYPE_UPDATE_LINEITEM => 'Updating lineitem:', static::TYPE_GET_GROUPS => 'Getting groups:', static::TYPE_GET_SETS => 'Getting sets:', - static::TYPE_GET_MEMBERSHIPS => 'Getting memberships', + static::TYPE_GET_MEMBERSHIPS => 'Getting memberships:', ]; return $errorMessages[$this->type] ?? $defaultMessage;