Convert signature verification specs to request specs

[ClearlyClaire]

Prerequisite for fixing #18474

I decided to hardcode the signature headers so that:

• the tests are more robust
• other implementers can have a look at examples Mastodon will take as input, together with a keypair to test things with

[ClearlyClaire]

cc @mjankowski since you're working a lot on specs and code style lately, I'd like your feedback on it, especially the long lines, and the routing issues (I used your pattern, but it clears the existing routes)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c7c7279) 84.28% compared to head (1c569c1) 84.31%.
Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #28443      +/-   ##
==========================================
+ Coverage   84.28%   84.31%   +0.02%     
==========================================
  Files        1039     1039              
  Lines       28226    28226              
  Branches     4550     4550              
==========================================
+ Hits        23790    23798       +8     
+ Misses       3287     3281       -6     
+ Partials     1149     1147       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mjankowski]

cc @mjankowski since you're working a lot on specs and code style lately, I'd like your feedback on it, especially the long lines, and the routing issues (I used your pattern, but it clears the existing routes)

On the routes/controller - I think what you are doing here is the least bad way to do this. It's just not as smooth in request specs as controller specs where it's more expected to do this. If we find a better way we can update, but at least for now this method matches what's done elsewhere (a CSP spec, maybe?)

On the long lines with the signature values ... the only things I can think of here both from readability and linting perspective would be to put the whole signature value into a heredoc assigned variable, and then do string interpolation when using the value. If the value is purposefully wrong, then just using the raw value makes sense. If the value is calculated from the headers, it might be nice to actually do the calculation here, and/or leave a comment line showing how it was built -- especially if as noted in the PR description, there's some side benefit here to potentially showing others what's being done and what's right/wrong approaches, etc.

[ClearlyClaire]

On the routes/controller - I think what you are doing here is the least bad way to do this. It's just not as smooth in request specs as controller specs where it's more expected to do this. If we find a better way we can update, but at least for now this method matches what's done elsewhere (a CSP spec, maybe?)

It's similar to what you did with the CSP spec, yeah. We could also private send to eval_block instead of draw, but this wouldn't work because of the catch-all unmatched_route fallback we have in config/routes.rb. Instead, I re-added the one route the code uses.

On the long lines with the signature values ... the only things I can think of here both from readability and linting perspective would be to put the whole signature value into a heredoc assigned variable, and then do string interpolation when using the value. If the value is purposefully wrong, then just using the raw value makes sense. If the value is calculated from the headers, it might be nice to actually do the calculation here, and/or leave a comment line showing how it was built -- especially if as noted in the PR description, there's some side benefit here to potentially showing others what's being done and what's right/wrong approaches, etc.

Do you have an example on how you'd do it? I'm not sure how a heredoc would help, these values do not have linebreaks.

The idea is that implementers would have values to play with without having to run Mastodon itself.

[mjankowski]

... but this wouldn't work because of the catch-all unmatched_route fallback we have in config/routes.rb

That rings a bell from the CSP one -- I recall having to work around that as well.

Do you have an example on how you'd do it? I'm not sure how a heredoc would help, these values do not have linebreaks.

You'd have to massage it a little bit ... something like this would work:

irb(main):001> original = 'Z8ilar3J7bOwqZkMp7sL8sRs4B1FT+UorbmvWoE+A5UeoOJ3KBcUmbsh+k3wQwbP5gMNUrra9rEWabpasZGphLsbDxfbsWL3Cf0PllAc7c1c7AFEwnewtExI83/qqgEkfWc2z7UDutXc2NfgAx89Ox8
DXU/fA2GG0jILjB6UpFyNugkY9rg6oI31UnvfVi3R7sr3/x8Ea3I9thPvqI2byF6cojknSpDAwYzeKdngX3TAQEGzFHz3SDWwyp3jeMWfwvVVbM38FxhvAnSumw7YwWW4L7M7h4M68isLimoT3yfCn2ucBVL5Dz8koBpYf/40w7QidClAw
CafZQFC29yDOg=='
=> "Z8ilar3J7bOwqZkMp7sL8sRs4B1FT+UorbmvWoE+A5UeoOJ3KBcUmbsh+k3wQwbP5gMNUrra9rEWabpasZGphLsbDxfbsWL3Cf0PllAc7c1c7AFEwnewtExI83/qqgEkfWc2z7UDutXc2NfgAx89Ox8DXU/fA2GG0jILjB6Up...
irb(main):002> 
irb(main):003" http_signature_value = <<-SIGNATURE.squish.tr(' ', '')
irb(main):004"   Z8ilar3J7bOwqZkMp7sL8sRs4B1FT+UorbmvWoE+A5UeoOJ3KBcUmbsh+k3wQwbP5gMNUrra9rEWabpa
irb(main):005"   sZGphLsbDxfbsWL3Cf0PllAc7c1c7AFEwnewtExI83/qqgEkfWc2z7UDutXc2NfgAx89Ox8DXU/fA2GG
irb(main):006"   0jILjB6UpFyNugkY9rg6oI31UnvfVi3R7sr3/x8Ea3I9thPvqI2byF6cojknSpDAwYzeKdngX3TAQEGz
irb(main):007"   FHz3SDWwyp3jeMWfwvVVbM38FxhvAnSumw7YwWW4L7M7h4M68isLimoT3yfCn2ucBVL5Dz8koBpYf/40
irb(main):008"   w7QidClAwCafZQFC29yDOg==
irb(main):009> SIGNATURE
=> "Z8ilar3J7bOwqZkMp7sL8sRs4B1FT+UorbmvWoE+A5UeoOJ3KBcUmbsh+k3wQwbP5gMNUrra9rEWabpasZGphLsbDxfbsWL3Cf0PllAc7c1c7AFEwnewtExI83/qqgEkfWc2z7UDutXc2NfgAx89Ox8DXU/fA2GG0jILjB6Up...
irb(main):010> 
irb(main):011> http_signature_value == original
=> true
irb(main):012> 

There may be more elegant way there, but that would work.

[ClearlyClaire]

That rings a bell from the CSP one -- I recall having to work around that as well.

There's no such thing in the API CSP test, there's the reloading at teardown, though, which I do here too.

squish.tr(' ', '')

Right, I guess I can do that… this is a little awkward though, especially since it's embedded in a header value that are in a similar situation (too long, no newlines) but for which spaces matter.

[nightpool]

I wouldn't worry too much about the long lines, but it might be nice to include a simple dev Ruby file or rake task or whatever that shows how you generated the spec values for future maintainability if the Signature headers need to change (e.g. if we change the requests or add a new request type we want to spec)

[ClearlyClaire]

I added some methods and assertions on the specs to demonstrate how the signature strings are built. It's not exhaustive because that's not the point of the specs, but this should be enough to illustrate how this works and quickly build specs.

[renchap]

Looks good to me and I like having more explicit values everywhere.

If you get an approval from @mjankowski (as he is much knowledgable than myself on specs) then go for merge :)

[mjankowski]

Looks solid as move from controller->request spec.

If we get feedback from people on the educational uses, we can always revise style/naming on that front later.