Allow OSD to show on restricted paged objects.

[ruebot]

GitHub Issue: Not sure if there is an actual issue for this.

• See also: Slack conversation

What does this Pull Request do?

• Alternate implementation of of View restricted access content #51
• This is essentially @kylehuynh205 and @dannylamb's work, I haven't done anything here other than test and package it all up.

How should this be tested?

You'd probably need the group based access control setup.

1. Create a paged object, and put access controls on it.
2. Load of the page.
3. OSD should not display
4. Add this commit to OSD, and drush cr
5. Refresh the page, and you should see OSD.

Interested parties

@seth-shaw-asu

[seth-shaw-asu]

@ruebot, tests are failing.

[seth-shaw-asu]

Is this only for paged content? Does this not apply to single page image viewing?

[ruebot]

Is this only for paged content? Does this not apply to single page image viewing?

OSD on single paged worked before and after this work. It was just paged objects with OSD that we discovered the issue.

I'll take a look at the tests.

[ruebot]

@seth-shaw-asu tests should be good now.

[seth-shaw-asu]

Is this only for paged content? Does this not apply to single page image viewing?

OSD on single paged worked before and after this work. It was just paged objects with OSD that we discovered the issue.

It wasn't working for single page items in my test. Is there a particular delegate I need to add to cantaloupe for this to work?

[ruebot]

@seth-shaw-asu honestly not sure. My testing is on the York playbook, which was originally based on the Islandora playbook a few years ago before everything was pulled into a single repo. Our setup as drifted since then, so I imagine our Cantaloupe setup is slightly different than whatever is considered canonical Islandora at this point.

[adam-vessey]

This would still exhibit the issue with caching, the same as #51 and #40. With Drupal's page cache enabled and allowed to cache for a duration longer than the token expiry, it could attempt to authenticate with expired tokens.

So, could add a few more steps to the test, something like:
6. Enable Drupal's page cache with a long expiry
7. Configure the JWT's expiry to be shorter than the page cache's expiry.
8. View something with OSD, and things should work.
9. After the JWT's expiry time elapses and before the page cache's expiry, without running a drush cr (or equivalent), request the OSD page, and it should still show.

... may be some oddities in the exact criteria of making use of the page cache being used in the last step, needing to allow your browser to use/request cached assets (so no Ctrl+Shift+R/Cmd+Shift+R kind of thing?).

Furthermore, may have to repeat all these tests steps between anonymous and authenticated users. Really, might want to avoid apply tokens for anonymous requests, for better cacheability of the response?

[ruebot]

Ok. Well, if anybody wants to carry the torch from here, go for it. I don't have the time to carry the torch past the finish line.

Feel free to close the PR if need be.

[adam-vessey]

Converting to draft 'til caching issues are addressed, so misc branch updates stop having this PR show up in the weekly tech call with the known issues intact.

[seth-shaw-asu]

@adam-vessey ,

Perhaps we could grab the expiry from the token and use that to generate a max-age parameter; throwing in the user tag for good measure?

# This line is already in the code:
$access_token = \Drupal::service('jwt.authentication.jwt')->generateToken();

# We would just add the caching metadata:
$variables['#cache']['max-age'] = \Drupal::service('jwt.transcoder')->decode($access_token)->getPayload()['exp'] - time();
$variables['#cache']['contexts'] = ['user:' . \Drupal::currentUser()->id()];

This would need some sanity checking around the existence of the 'exp' key; but do you think that could resolve the caching issues?

[seth-shaw-asu]

Of course, this all assumes you have something that can validate the JWT for file download. I took the UTSC private_files_adapter down to essentials to get this (assuming an islandora_jwt.module), which should work assuming you have other user-based access controls for file downloads:

# See https://github.com/digitalutsc/private_files_adapter/blob/main/private_files_adapter.module#L46

use Drupal\Core\Access\AccessResult;
use Drupal\Core\Access\AccessResultInterface;
use Drupal\Core\Entity\EntityInterface;
use Drupal\Core\Session\AccountInterface;

islandora_jwt_ext_file_access(EntityInterface $entity, string $operation, AccountInterface $account): AccessResultInterface {

  # We are only concerred about anonymous users providing a JWT token.
  if (!$account->isAnonymous() || empty($request->server->get('HTTP_AUTHORIZATION')) {
    return AccessResult::neutral();
  }

  # Authenticate the JWT
  $authenticated_account = \Drupal::service('jwt.authentication.jwt')->authenticate($request);
  $access = $entity->access($operation, $authenticated_account);
  return ($access) ? $access : AccessResult::neutral();
}

Caveat: untested code.

[adam-vessey]

Perhaps we could grab the expiry from the token and use that to generate a max-age parameter

Yeah, is pretty much all that has been required all along (and possibly to avoid dealing with tokens entirely for anonymous, as max-age doesn't really work for anonymous, IIRC?): https://www.drupal.org/docs/drupal-apis/cache-api/cache-max-age

Unfortunately, max-age does not work for anonymous users

In dealing with the "user" bit, you seem to be mixing up cache contexts and tags. Contexts take just the name of the context (probably user, in this case), and do some magic behind the scenes to build out a cache ID relevant for only the passed contexts (such that the cached resource is only good in this specific context). Cache tags are for invalidation, which are more: If the access was dependent upon some property of the user (such as some custom fields) then might make sense to use the user tag to invalidate any cache entries for when there are changes/updates/edits made to the given user object; however, this probably isn't what is desired in the general case here.

Really not sure about that file access stuff? Like: The JWT should be used to establish the user earlier in the request (via https://git.drupalcode.org/project/jwt/-/blob/2.x/src/Authentication/Provider/JwtAuth.php?ref_type=heads#L58-88): It shouldn't be necessary to touch the JWT outside of it, the appropriate user should already be present as the "current user" of the request.

[seth-shaw-asu]

In dealing with the "user" bit, you seem to be mixing up cache contexts and tags.

Not at all surprised if I am; Drupal's caching internals are still a mystery to me.

If the access was dependent upon some property of the user (such as some custom fields) then might make sense to use the user tag to invalidate any cache entries for when there are changes/updates/edits made to the given user object; however, this probably isn't what is desired in the general case here.

Considering that permissions are usually role-based, would it make sense to add a user tag to account in changes of the user's role?

Really not sure about that file access stuff? Like: The JWT should be used to establish the user earlier in the request (via https://git.drupalcode.org/project/jwt/-/blob/2.x/src/Authentication/Provider/JwtAuth.php?ref_type=heads#L58-88): It shouldn't be necessary to touch the JWT outside of it, the appropriate user should already be present as the "current user" of the request.

Ah, I see, because the request would have already checked the applies function and thus executed the authenticate and set the current user before hitting the file_download hook... so checking the JWT there would be redundant.

[adam-vessey]

Considering that permissions are usually role-based, would it make sense to add a user tag to account in changes of the user's role?

No, such would be better handled via the user.permissions context (or possibly user.roles, if checking roles explicitly anywhere); though, given we already intend to do things on the wider user context, such is unnecessary:
https://www.drupal.org/docs/drupal-apis/cache-api/cache-contexts#core-contexts

comparing (and folding) cache contexts becomes simpler: if both a.b.c and a.b are present, it's obvious that a.b encompasses a.b.c, and thus it's clear why the a.b.c can be omitted, why it can be "folded" into the parent

Ideally caches should be usable by more than one user. Using the context of user.permissions or user.roles could potentially allow reuse by other users with the same set of permissions/roles, but given that specific user info is presently encoded into the JWT, is probably undesirable.

The validity of the JWT doesn't change when the users permissions change, like: The token is more to identify/authenticate the user. The permissions/access granted to the user (which would be affected by role/permission change) is a separate concern that the access check stuff deals with (that should be separately cacheable). JWTs could be more specific; however, the JWT module would ideally need to change to represent this to be able to attach cacheable metadata to JWTs such that you could grab such cacheable metadata wherever they are used, such that the JWT would effectively know what cache contexts/tags/etc are relevant and bubble them out.

ramble ramble Authentication vs authorization, kind of stuff, which gets messy to discuss due to the name of the headers used, but anyway.