Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

View restricted access content #51

Open
wants to merge 3 commits into
base: 2.x
Choose a base branch
from

Conversation

kylehuynh205
Copy link

@kylehuynh205 kylehuynh205 commented Jul 6, 2023

What does this Pull Request do?

What's new?

A in-depth description of the changes made by this PR. Technical details and
possible side effects.

  • Attach a JWT token in Rest requests to Cantaloupe
  • Attach a JWT token in GET request to /node/nid/book-manifest in the manifest parser if the node is restricted access.

How should this be tested?

A description of what steps someone could take to:

  • Setup isle-dc with access control with Group in this PR
  • Replace the openseadragon module with this branch.
  • Create a book repository item node and its children OR with sample content ingested, visit https://islandora.traefik.me/node/29

Interested parties

Tag (@ mention) interested parties or, if unsure, @Islandora/committers

Comment on lines +102 to +107
'loadTilesWithAjax' => TRUE,
'ajaxWithCredentials' => TRUE,
'ajaxHeaders' => [
"Authorization" => "Bearer " . $access_token,
'token' => $access_token,
],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much the same as the previous PR, these tokens are generally expected to be valid for a limited length of time, so we would have to propagate cache headers preventing them from being used beyond their lifetime to prevent serving up responses with invalid tokens.

Comment on lines +162 to +167
'loadTilesWithAjax' => TRUE,
'ajaxWithCredentials' => TRUE,
'ajaxHeaders' => [
"Authorization" => "Bearer " . $access_token,
'token' => $access_token,
],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much the same as the previous PR, these tokens are generally expected to be valid for a limited length of time, so we would have to propagate cache headers preventing them from being used beyond their lifetime to prevent serving up responses with invalid tokens.

$variables['#attached']['drupalSettings']['openseadragon'][$openseadragon_viewer_id] = [
'basePath' => Url::fromUri($iiif_address),
'fitToAspectRatio' => $viewer_settings['fit_to_aspect_ratio'],
'options' => [
'id' => $openseadragon_viewer_id,
'prefixUrl' => 'https://cdnjs.cloudflare.com/ajax/libs/openseadragon/2.4.2/images/',
'tileSources' => $tile_sources,

// For dsu-utsc.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer quite true if this is to be pushed into the base code.

@@ -148,6 +157,14 @@ function template_preprocess_openseadragon_iiif_manifest_block(&$variables) {
'id' => $openseadragon_viewer_id,
'prefixUrl' => 'https://cdnjs.cloudflare.com/ajax/libs/openseadragon/2.4.2/images/',
'tileSources' => $tile_sources,

// For dsu-utsc.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer quite true if this is to be pushed into the base code.

@@ -85,7 +87,12 @@ public function getTileSources($manifest_url) {

try {
// Request the manifest.
$manifest_response = $this->httpClient->get($manifest_url);
// $manifest_response = $this->httpClient->get($manifest_url);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented out code shouldn't be here.

Comment on lines +91 to +95
$manifest_response = $this->httpClient->request('GET', $manifest_url, [
'headers' => [
'Authorization' => 'Bearer ' . $access_token,
],
]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure that this makes sense in the case $access_token was not passed, as we would generate an Authorization header of just Bearer (or possibly Bearer null?).

Really, if we want to be sure to always have a token here, why not just generate it here?

Comment on lines +100 to +107

// For dsu-utsc.
'loadTilesWithAjax' => TRUE,
'ajaxWithCredentials' => TRUE,
'ajaxHeaders' => [
"Authorization" => "Bearer " . $access_token,
'token' => $access_token,
],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is feedback from a Slack conversation.

Testing this on our instance, if we leave this two blocks in (kylehuynh205@9ceb5d4#diff-6d693de3726ed28241d0e4b5045e66eff4e2e82da162a08975768d342ec2ed7eR161-R167 and kylehuynh205@9ceb5d4#diff-6d693de3726ed28241d0e4b5045e66eff4e2e82da162a08975768d342ec2ed7eR101-R107), images are not displayed in the viewer, and we get CORS errors.

If I remove the two blocks, everything works fine, including the original issue I had with the viewer not working on paged objects that used OSD.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a good compromise here would be to add these values loadTilesWithAjax, ajaxWithCredentials, ajaxHeaders, but do not set them by default, and make them configurable in the settings page for the OSD module? So something like:

'loadTilesWithAjax' => FALSE,
'ajaxWithCredentials' => FALSE,

...then if the above is set to TRUE, add the ajaxHeaders section.

ruebot added a commit to yorkulibraries/openseadragon that referenced this pull request Aug 17, 2023
ruebot added a commit to yorkulibraries/openseadragon that referenced this pull request Aug 17, 2023
@seth-shaw-asu
Copy link
Member

@ruebot , could your alternate solution -- yorkulibraries@ce29240 -- be turned into a PR so we can get a solution to this merged? Or is there something else we would still need from this PR for the full solution?

@ruebot
Copy link
Member

ruebot commented Aug 25, 2023

@ruebot happy to do a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants