Add Smart CDN Signature Auth helper #71

kvz · 2024-11-25T15:58:03Z

Allows you to generate signed URLs for Transloadit's Smart CDN.

rmehner · 2024-11-25T16:23:20Z

@kvz let me know if there's anything to review.

kvz · 2024-11-26T08:23:14Z

Appreciate the offer! This would be ready for you now @rmehner!

Acconut · 2024-11-26T08:36:49Z

test/unit/transloadit/test_assembly.rb

      it "must call the create! method with the same parameters" do
        VCR.use_cassette "submit_assembly" do
          file = open("lib/transloadit/version.rb")
          mocker = Minitest::Mock.new
          mocker.expect :call, nil, [file]
          @assembly.stub :create!, mocker do
-            @assembly.submit!(file)
+            @assembly.create!(file)


IIRC this test ensured that the deprecated submit! method calls create!. By replacing the submit! call with a create! call, the test is now useless. Either we keep the test as is or we remove it entirely.

Okay i see.

I thought the output was smelly, reverted this 👌

Acconut · 2024-11-26T08:40:22Z

test/unit/transloadit/test_smart_cdn.rb

+    end
+
+    it "requires input" do
+      assert_raises ArgumentError, "input is required" do


Internally we have discusses use cases where customers might leave ${fields.input} unset because they don't need it at all or prefer using named query parameters. When a customer wants to sign such a URL, should we require them to pass nil and an explicit empty string to the signature method? I prefer an empty string, which would also work with this implementation.

That's right, I now changed it to allow empty string inputs as well as empty string e.g. width, which will show up in the URL

Acconut · 2024-11-26T08:43:15Z

test/unit/transloadit/test_smart_cdn.rb

+    end
+
+    it "allows overriding credentials" do
+      url = @transloadit.signed_smart_cdn_url(


I wonder what the use case here is. AFAICT signed_smart_cdn_url is not a static method but an instance method and thus a Transloadit instance is always needed to call it. When would someone want to pass in other credentials to method instead of creating another Transloadit instance?

Removed this now with the exception of the reference implementation

Acconut · 2024-11-26T08:47:37Z

lib/transloadit.rb

+      next if value.nil?
+      if value.is_a?(Array)
+        value.each do |val|
+          next if val.nil? || val.to_s.empty?


Does this filter out empty strings? While I don't have a use case, I also think we should not place unnecessary restrictions on the URL parameters. If a customer wants to have an empty parameter value, they can have it.

I now changed it to allow empty string e.g. width, which will show up in the URL

Acconut · 2024-11-26T08:52:17Z

test/unit/transloadit/test_smart_cdn_node_parity.rb

+
+        url = @transloadit.signed_smart_cdn_url(**params)
+        node_url = run_node_script(params)
+        assert_equal node_url, url


These test cases only check if the Ruby implementation matches the Node.js script in this repository. We should also compare the generated URLs against static strings here to ensure they are what we expect them to be:

Suggested change

assert_equal node_url, url

assert_equal node_url, url

assert_equal "https://....", url

This makes the tests easier to understand since you directly see what the expected output is. In addition, we test the Node.js script to ensure it doesn't produce total garbage :)

I changed it to have just one test file, that tests both against node as a hardcoded string

rmehner

Don't have enough time today, but @Acconut already did some good reviews there :)

rmehner · 2024-11-26T12:48:40Z

lib/transloadit.rb

+    raise ArgumentError, "workspace is required" if workspace.nil?
+    raise ArgumentError, "template is required" if template.nil?
+    raise ArgumentError, "input is required" if input.nil?


Do you want to guard explicitly against nil being passed? Or is this to make sure that the argument is passed at all? If so, that's already taken care of by Ruby itself, since you don't define a default, making the keywords required.

We want to allow empty strings, but not omitting or other falsey values

I don't think we want to allow empty strings for workspace and template. They must always be set. Only input could be an empty string.

Implemented and rolled tests for this now too 👌

lib/transloadit.rb

rmehner · 2024-11-26T12:52:23Z

lib/transloadit.rb

+    query_params["exp"] = [expire_at.to_s]
+
+    # Sort parameters to ensure consistent ordering
+    sorted_params = query_params.sort.map do |key, values|


Save some CPU cycles, use flat_map :D

Perfect 🙏

Co-authored-by: Robin Mehner <robin@coding-robin.de>

Co-Authored-By: Robin Mehner <robin@coding-robin.de>

kvz · 2024-11-28T10:16:05Z

I think I did all I could here, but I don't have access to cut a release on Rubygems, can you add me there @rmehner ?

rmehner · 2024-11-28T13:01:20Z

I think I did all I could here, but I don't have access to cut a release on Rubygems, can you add me there @rmehner ?

What's your rubygems.org username?

rmehner · 2024-11-28T13:02:58Z

@kvz also could you please add yourself here: https://github.com/transloadit/ruby-sdk/blob/main/transloadit.gemspec#L10-L11?

kvz · 2024-11-28T13:10:47Z

@kvz also could you please add yourself here: https://github.com/transloadit/ruby-sdk/blob/main/transloadit.gemspec#L10-L11?

✅ done!

What's your rubygems.org username?

Just signed up as kvz

kvz · 2024-11-28T13:16:34Z

Thank you!

Successfully registered gem: transloadit (3.1.0)

If anything looks out the order to you please let me know :)

rmehner · 2024-11-28T13:18:18Z

You could draft a release here: https://github.com/transloadit/ruby-sdk/releases, so it shows up in the releases tab of this repo.

kvz · 2024-11-28T13:24:04Z

Check, just did it.

Would you say these steps are accurate? https://github.com/transloadit/ruby-sdk?tab=readme-ov-file#releasing-on-rubygems

rmehner · 2024-11-28T13:48:32Z

Yes, it's accurate. We could ease it a little bit with the help of gem-release, but since we don't do it so often and the steps are pretty well documented, I don't think it's worth the effort here.

kvz added 9 commits November 25, 2024 16:10

Create ruby-sdk.code-workspace

b46c49d

Update README.md

17158a6

Update test_assembly.rb

f6d0867

Create smartcdn-sig.ts

d18e470

wip

aab63fd

Wip

59d4da5

Create Makefile

9f999f8

Update smartcdn-sig.ts

6a61540

Cleanup

b9746b8

kvz added 7 commits November 25, 2024 17:40

Move to tl instance

3725846

Wip

cbfdbaa

Inline smart cdn method

829ef0a

Readme

e85010e

More comprehensive node comparison

3e8ef4d

Make no tsx fatal for TEST_NODE_PARITY

504f692

Update ci.yml

319abb0

kvz marked this pull request as ready for review November 25, 2024 20:07

kvz requested review from rmehner and Acconut November 25, 2024 20:07

kvz self-assigned this Nov 25, 2024

Acconut reviewed Nov 26, 2024

View reviewed changes

kvz added 7 commits November 26, 2024 11:22

Document test flags

e4dd9cf

Support empty input

c76ecc8

Update ruby-sdk.code-workspace

7c86587

Assert empty string param behavior

ee85868

Make empty string param show up in url

12d65b3

Update test_smart_cdn_node_parity.rb

25de91e

Harcode urls

1023da6

kvz added 7 commits November 26, 2024 12:48

Merge parity testing into regular suite

cacf4de

Retire expireIn

e075093

Remove ability to override auth keys

ee44731

Fix: redundant self

2b11d9a

Fix linting

c3d475c

Fixes

437c276

Validate both implementations against hardcoded URL

5b9bfdd

kvz requested a review from Acconut November 26, 2024 12:51

rmehner reviewed Nov 26, 2024

View reviewed changes

kvz and others added 3 commits November 26, 2024 14:57

Update lib/transloadit.rb

72f4c84

Co-authored-by: Robin Mehner <robin@coding-robin.de>

Add flat_map. Thx @rmehner!

c373b90

Co-Authored-By: Robin Mehner <robin@coding-robin.de>

Update transloadit.rb

c6c553f

Co-Authored-By: Robin Mehner <robin@coding-robin.de>

kvz requested a review from rmehner November 26, 2024 14:02

Acconut approved these changes Nov 28, 2024

View reviewed changes

kvz added 2 commits November 28, 2024 10:58

Merge branch 'main' into cdn-sigauth

2625559

Ensure workspace and template can't be empty strings

42f9904

kvz merged commit a3d8049 into main Nov 28, 2024
6 checks passed

kvz deleted the cdn-sigauth branch November 28, 2024 10:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Smart CDN Signature Auth helper #71

Add Smart CDN Signature Auth helper #71

kvz commented Nov 25, 2024 •

edited

Loading

rmehner commented Nov 25, 2024

kvz commented Nov 26, 2024 •

edited

Loading

Acconut Nov 26, 2024

kvz Nov 26, 2024

Acconut Nov 26, 2024

kvz Nov 26, 2024

Acconut Nov 26, 2024

kvz Nov 26, 2024

Acconut Nov 26, 2024

kvz Nov 26, 2024

Acconut Nov 26, 2024

kvz Nov 26, 2024

rmehner left a comment

rmehner Nov 26, 2024

kvz Nov 26, 2024

Acconut Nov 28, 2024

kvz Nov 28, 2024

rmehner Nov 26, 2024

kvz Nov 26, 2024

kvz commented Nov 28, 2024

rmehner commented Nov 28, 2024

rmehner commented Nov 28, 2024

kvz commented Nov 28, 2024

kvz commented Nov 28, 2024

rmehner commented Nov 28, 2024

kvz commented Nov 28, 2024

rmehner commented Nov 28, 2024

	assert_equal node_url, url
	assert_equal node_url, url
	assert_equal "https://....", url

Add Smart CDN Signature Auth helper #71

Add Smart CDN Signature Auth helper #71

Conversation

kvz commented Nov 25, 2024 • edited Loading

rmehner commented Nov 25, 2024

kvz commented Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmehner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvz commented Nov 28, 2024

rmehner commented Nov 28, 2024

rmehner commented Nov 28, 2024

kvz commented Nov 28, 2024

kvz commented Nov 28, 2024

rmehner commented Nov 28, 2024

kvz commented Nov 28, 2024

rmehner commented Nov 28, 2024

kvz commented Nov 25, 2024 •

edited

Loading

kvz commented Nov 26, 2024 •

edited

Loading