Add Authorization Utilitites #281

JimBobSquarePants · 2022-07-07T14:45:09Z

Prerequisites

I have written a descriptive pull-request title
I have verified that there are no overlapping pull-requests open
I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
I have provided test coverage for my change (where applicable)

Description

Adds a new type ImageSharpRequestAuthorizationUtilities which completes the HMAC story, allowing developers to create tokens directly using the functions defined in the middleware options.

codecov · 2022-07-07T15:07:46Z

Codecov Report

Merging #281 (eadeef6) into main (0a62c3b) will increase coverage by 0%.
The diff coverage is 88%.

@@         Coverage Diff          @@
##           main   #281    +/-   ##
====================================
  Coverage    84%    85%            
====================================
  Files        75     76     +1     
  Lines      2047   2169   +122     
  Branches    299    303     +4     
====================================
+ Hits       1739   1857   +118     
- Misses      223    227     +4     
  Partials     85     85

Flag	Coverage Δ
unittests	`85% <88%> (+<1%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/ImageSharp.Web/PathUtilities.cs	`100% <ø> (+20%)`	⬆️
...arp.Web.Providers.AWS/Caching/AWSS3StorageCache.cs	`78% <60%> (-10%)`	⬇️
...Sharp.Web/Commands/QueryCollectionRequestParser.cs	`71% <66%> (ø)`
...arp.Web/ImageSharpRequestAuthorizationUtilities.cs	`90% <90%> (ø)`
.../ImageSharp.Web/Middleware/ImageSharpMiddleware.cs	`80% <93%> (-1%)`	⬇️
src/ImageSharp.Web/AsyncHelper.cs	`100% <100%> (ø)`
src/ImageSharp.Web/Caching/UriAbsoluteCacheKey.cs	`100% <100%> (ø)`
...p.Web/Caching/UriAbsoluteLowerInvariantCacheKey.cs	`100% <100%> (ø)`
src/ImageSharp.Web/CaseHandlingUriBuilder.cs	`88% <100%> (+22%)`	⬆️
...Commands/PresetOnlyQueryCollectionRequestParser.cs	`100% <100%> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a62c3b...eadeef6. Read the comment docs.

ronaldbarendse

This will be a great addition and will make (correct) use of the HMAC image processing protection a lot easier! I've added some comments/suggestions and would love to test this when it's ready for review 👍🏻

src/ImageSharp.Web.Providers.AWS/Caching/AWSS3StorageCache.cs

src/ImageSharp.Web/CaseHandlingUriBuilder.cs

src/ImageSharp.Web/Commands/QueryCollectionRequestParser.cs

src/ImageSharp.Web/ImageSharpRequestAuthorizationUtilities.cs

JimBobSquarePants · 2022-07-09T13:45:36Z

@ronaldbarendse Good feedback, thanks! I've pushed all the fixes.

ronaldbarendse

I've successfully tested this PR together with a massively simplified version of umbraco/Umbraco-CMS#12376 (replacing the custom ImageSharpImageUrlTokenGenerator abstraction/implementation with ImageSharpRequestAuthorizationUtilities) 🙌🏻

I've only added some final comments about backwards compatibility and naming, but looks good to me otherwise!

ronaldbarendse · 2022-07-11T15:01:47Z

src/ImageSharp.Web/Middleware/ImageSharpMiddleware.cs

            FormatUtilities formatUtilities,
-            AsyncKeyReaderWriterLock<string> asyncKeyLock)
+            AsyncKeyReaderWriterLock<string> asyncKeyLock,
+            ImageSharpRequestAuthorizationUtilities requestAuthorizationUtilities)


Adding this parameter is a (binary) breaking change which can be avoided by re-adding the previous constructor as obsolete and manually constructing the ImageSharpRequestAuthorizationUtilities:

[Obsolete] public ImageSharpMiddleware( RequestDelegate next, IOptions<ImageSharpMiddlewareOptions> options, ILoggerFactory loggerFactory, IRequestParser requestParser, IEnumerable<IImageProvider> resolvers, IEnumerable<IImageWebProcessor> processors, IImageCache cache, ICacheKey cacheKey, ICacheHash cacheHash, CommandParser commandParser, FormatUtilities formatUtilities, AsyncKeyReaderWriterLock<string> asyncKeyLock) : this( next, options, loggerFactory, requestParser, resolvers, processors, cache, cacheKey, cacheHash, commandParser, formatUtilities, asyncKeyLock, new ImageSharpRequestAuthorizationUtilities(options, requestParser, processors, commandParser, null)) { }

This does require ImageSharpRequestAuthorizationUtilities to allow a null value for IServiceProvider, but that shouldn't be an issue, as it isn't required/used by the methods this middleware calls into anyway.

I know it’s binary breaking but to upgrade you’re gonna have to recompile anyway. Obsolete isn’t actually a workaround due to consumer rulesets which can be violated using the attribute.

While looking into this a bit more, I noticed ImageSharpRequestAuthorizationUtilities is currently only really required to strip unknown commands, as ComputeHMACAsync(ImageCommandContext) just delegates to this.options.OnComputeHMACAsync(ImageCommandContext, byte[]). I also think that stripping unknown commands isn't a request authorization concern, so it's probably best to remove this dependency on ImageSharpRequestAuthorizationUtilities altogether...

ronaldbarendse · 2022-07-11T15:02:11Z

src/ImageSharp.Web/Middleware/ImageSharpMiddleware.cs

            this.logger = loggerFactory.CreateLogger<ImageSharpMiddleware>();
            this.formatUtilities = formatUtilities;
            this.asyncKeyLock = asyncKeyLock;
+            this.authorizationUtilities = requestAuthorizationUtilities;


Shouldn't the field also be named requestAuthorizationUtilities?

Nah, that’s just ocd

ronaldbarendse · 2022-07-11T15:04:43Z

src/ImageSharp.Web/ImageSharpRequestAuthorizationUtilities.cs

+    /// <summary>
+    /// Contains various helper methods for authorizing image requests.
+    /// </summary>
+    public sealed class ImageSharpRequestAuthorizationUtilities


Shouldn't this be renamed to RequestAuthorizationUtilities, similar to FormatUtilities, HMACUtilities, etc?

I wanted to make it more explicit. It’s for ImageSharp requests only.

Yeah, but it's already in the SixLabors.ImageSharp.Web namespace and consumers can otherwise alias the class name if it would conflict with something else:

using ImageSharpRequestAuthorizationUtilities = SixLabors.ImageSharp.Web.RequestAuthorizationUtilities;

And after mentioning that, I acknowledge that you can also use that to do the reverse 😓 So it's more to align with the naming used by all the other utility classes...

The naming is about about consumption intent. All the other types are designed to be used conceptionally within other ImageSharp.Web type implementations (processors generally) whereas this type is designed to be consumed from the users code only. It has to be explicit and unfortunately verbose.

ronaldbarendse · 2022-07-11T22:29:51Z

I've done some further testing, investigation and refactoring to:

Rename ImageSharpRequestAuthorizationUtilities to RequestAuthorizationUtilities;
Remove the dependency on RequestAuthorizationUtilities in the middleware (as mentioned in this comment);
Obsolete HMACUtilities.TokenCommand (as this class isn't responsible for query string/command manipulation and you can now use RequestAuthorizationUtilities.TokenCommand instead);
Include minor fixes like using CaseHandlingUriBuilder.Encode() to generate the HMAC token cache key (that ensures the query string/commands are always lowercased), update code styling and do some re-ordering.

You can see all my changes here: js/auth-utils...ronaldbarendse:ImageSharp.Web:js/auth-utils. I can convert this into a PR targeting js/auth-utils if you want to discuss (and easily merge) these, just let me know! 👍🏻

JimBobSquarePants · 2022-07-12T12:23:50Z

Thanks for having a crack but I think you missed the point of this new type when making some changes.

We absolutely want to be using the new type from the middleware. It ensures the same type process is used on both sides.
We also want to use that type to strip the unknown commands. Whether to expose that method is up for debate.

Moving operations to various types (IEnumerable<IImageWebProcessor>) is never wise, internal or otherwise.

Tests should be cleaned up (I'll copy some of the changes you made across), most definitely but the other random changes (logging, field field order) are unnecessary. Code shouldn't be changed for changes sake. We also never, ever use ObsoleteAttribute in any of the Six Labors libraries. It's a sure fire way to accumulate technical debt that is unmaintainable given the lack of funding and time.

My changes are only binary breaking if someone is directly calling the middleware constructor which they simply should not be doing. I'll be versioning this release 2.1.0 anyway to send a signal that more fundamental changes have taken place.

JimBobSquarePants · 2022-07-18T13:13:39Z

This is probably good to merge now.

ronaldbarendse · 2022-10-05T09:46:59Z

@JimBobSquarePants Can this also be released in a 2.0.3 patch version? I'd love to finish the HMAC authorization support in Umbraco (see PR umbraco/Umbraco-CMS#12376) 😇

JimBobSquarePants · 2022-10-05T12:27:38Z

@ronaldbarendse I'm actually hoping to release a 2.1.0 build very soon. I've been working on a TagHelper which is allowing me to dogfood the utilites a bit better before releasing.

ronaldbarendse · 2023-02-10T13:20:09Z

I see the work on the TagHelper PR has stalled a bit and currently has some unresolved comments, but I would really like to make use of the new authorization utilities. Would it be possible for you to create a 2.0.3 release containing just these changes? 🙏🏻

JimBobSquarePants · 2023-02-10T14:14:19Z

Let me see what Ican do. No promises though. I've got a lot on just now.

JimBobSquarePants added 4 commits July 6, 2022 00:36

Sketch out auth utils class

06f99a4

Add overloads and cleanup

879f990

Wire up utils

e42ef8e

Update Directory.Build.targets

2ad2da1

JimBobSquarePants added 2 commits July 9, 2022 00:18

Add encoding tests

5aaff20

Cover the new authorization token methods

a6bdd95

ronaldbarendse suggested changes Jul 9, 2022

View reviewed changes

ronaldbarendse mentioned this pull request Jul 9, 2022

v10: Add HMAC image processing protection umbraco/Umbraco-CMS#12376

Closed

1 task

Address feedback

ae90cdf

JimBobSquarePants changed the title ~~WIP Add Authorization Utilitites~~ Add Authorization Utilitites Jul 9, 2022

JimBobSquarePants marked this pull request as ready for review July 9, 2022 13:44

JimBobSquarePants requested a review from a team July 9, 2022 13:44

JimBobSquarePants added bug enhancement labels Jul 9, 2022

ronaldbarendse suggested changes Jul 11, 2022

View reviewed changes

JimBobSquarePants added 2 commits July 18, 2022 22:46

Minor cleanup.

b875731

Merge branch 'main' into js/auth-utils

eadeef6

JimBobSquarePants merged commit 108a615 into main Jul 20, 2022

JimBobSquarePants deleted the js/auth-utils branch July 20, 2022 02:39

ronaldbarendse mentioned this pull request Apr 28, 2023

v12: Add HMAC image processing protection umbraco/Umbraco-CMS#14181

Merged

1 task

Uh oh!

Add Authorization Utilitites #281

Add Authorization Utilitites #281

Uh oh!

Conversation

JimBobSquarePants commented Jul 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Prerequisites

Description

Uh oh!

codecov bot commented Jul 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ronaldbarendse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JimBobSquarePants commented Jul 9, 2022

Uh oh!

ronaldbarendse left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronaldbarendse commented Jul 11, 2022

Uh oh!

JimBobSquarePants commented Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JimBobSquarePants commented Jul 18, 2022

Uh oh!

ronaldbarendse commented Oct 5, 2022

Uh oh!

JimBobSquarePants commented Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronaldbarendse commented Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JimBobSquarePants commented Feb 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JimBobSquarePants commented Jul 7, 2022 •

edited

Loading

codecov bot commented Jul 7, 2022 •

edited

Loading

ronaldbarendse left a comment •

edited

Loading

JimBobSquarePants commented Jul 12, 2022 •

edited

Loading

JimBobSquarePants commented Oct 5, 2022 •

edited

Loading

ronaldbarendse commented Feb 10, 2023 •

edited

Loading