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

Support signed URLs in IIIF requests #5

Merged
merged 2 commits into from
Nov 6, 2024
Merged

Conversation

kdid
Copy link
Contributor

@kdid kdid commented Oct 11, 2024

Summary

Add support for HMAC-signed URLs with an expires parameter.
This will make certain operations (like one-off downloads and redirects) a lot simpler.

@kdid kdid force-pushed the 5073-iiif-signed-urls branch from 3a606c3 to b450227 Compare October 11, 2024 20:17
@kdid kdid requested a review from mbklein October 11, 2024 20:19
@kdid kdid force-pushed the 5073-iiif-signed-urls branch from b450227 to 355b5ac Compare October 11, 2024 20:40
Copy link
Contributor

@mbklein mbklein left a comment

Choose a reason for hiding this comment

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

This is really fantastic and exactly what I was thinking. I just have two suggestions:

  1. [Fully my fault, since I didn't consider this before writing the spec ticket] Change the id claim field to sub, expires to exp, and use pascalCase for the max-[dimension] fields, just to be consistent with JWT best practices.
  2. Instead of just returning true or false from validateJwtClaims, maybe return { valid: [true|false], reason: reason } and then use the reason as the body of the 403 response (e.g., disallowed ${field} or subject doesn't match or maxWidth exceeded). I don't think that gives away too much about the implementation, and it would demonstrate to tinkerers that we're serious about checking the parameters. 😄

@kdid kdid force-pushed the 5073-iiif-signed-urls branch from 355b5ac to e7b9155 Compare October 22, 2024 20:13
@kdid
Copy link
Contributor Author

kdid commented Oct 22, 2024

@mbklein - this is ready for re-review. Code is deployed live on staging.

Comment on lines 96 to 102
if (jwtClaims['max-width'] && params.size.width > jwtClaims['max-width']) {
errors.push("Width too large");
}

if (jwtClaims['max-height'] && params.size.height > jwtClaims['max-height']) {
errors.push("Height too large");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

max-width and max-height aren't being enforced, because params.size isn't being parsed into width and height. It looks like 2048,1536 or !300,300 or even ,768 or pct:50. This is where all the stuff about the “full frame image” came from in the documentation of those fields – there was a whole discussion in the serverless-iiif repo about how to handle max dimensions in light of the fact that a savvy and patient user could simply request individual tiles that didn't violate the maximum and stitch them together.

The iiif-processor gem offers a way to calculate the effective width and height of the full image based on the parameters given even when a region is requested, but all things considered, I'm not convinced it's worth going to great lengths to support it right now. Maybe the best thing to do is remove max-width and max-height from the code and the docs and come back to it later if we feel the need. I'm willing to be persuaded otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you decide to remove the max-* support, consider the PR approved once that's done. Otherwise, I'm happy to pair on the rest of implementation if we have time.

@kdid kdid force-pushed the 5073-iiif-signed-urls branch from 1fb41bf to 10e07f1 Compare November 6, 2024 19:54
@kdid
Copy link
Contributor Author

kdid commented Nov 6, 2024

@mbklein - I think this is ready for re-review!

It's deployed on staging and there should be a number of shared test evens on the viewer function (I was logged in as admin).

I wasn't sure what we'd want to do if the S3 object did not exist at all so I just logged it and let it pass on through? My thought was that right now the authorizer doesn't care if the object exists or not. Open to counter arguments.

@mbklein mbklein merged commit 1325f78 into main Nov 6, 2024
6 checks passed
@mbklein mbklein deleted the 5073-iiif-signed-urls branch November 6, 2024 22:07
@kdid kdid self-assigned this Dec 12, 2024
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.

2 participants