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

Add SHA-256 support for auth signatures #479

Merged
merged 1 commit into from
Mar 11, 2021

Conversation

YomesInc
Copy link
Contributor

@YomesInc YomesInc commented Mar 7, 2021

Brief Summary of Changes

Added support for a signature_algorithm config parameter.

Default value: sha1
Possible values: sha1, sha256

What Does This PR Address?

  • New feature

Are Tests Included?

  • Yes

Reviewer, Please Note:

⚠️ Note that we added a new Error that might get thrown - If config.signature_algorithm is neither sha1 nor sha256 then an exception will throw.

This is in a very, very unlikely edge case a breaking change to the SDK.

The edge case:

If a developer has previously set config.signature_algorithm to something other than sha1 or sha256, then upgrading to this code would suddenly throw a new error that wasn't there before.

This edge case is very unlikely as config.signature_algorithm wasn't available before this change.

Up to you whether you consider this a breaking change.

A possible alternative way to handle this would be to fallback to SHA-1 if an unknown algorithm is given in config instead of throwing an exception.

This will make this change not be a breaking change, but may potentially confuse developers (e.g., if they set config.signature_algorithm to sha-256 instead of sha256 they won't know why they're getting SHA-1).

I believe the approach we used is better.


The bug that I described in 02b3abe as merely theoretical and something that may happen in the future actually happened when we were working on this branch (turns out the same bug I fixed in uploader_spec.js was in utils_spec.js as well). I went ahead and moved the backing up of the config to the main beforeEach of utils_spec.js.

@YomesInc YomesInc requested a review from patrick-tolosa March 7, 2021 21:16
@patrick-tolosa patrick-tolosa requested a review from strausr March 11, 2021 11:07
@patrick-tolosa patrick-tolosa merged commit 0ac8957 into master Mar 11, 2021
patrick-tolosa added a commit that referenced this pull request Jun 16, 2021
* Update README.md

Added Travis badge to README

* Support multiple resource_types in ZIP generation (#348)

Support multiple resource_types in ZIP generation

* Refactor the order of the assertions in account_spec (#352)

The reasoning for this is that if the first expect fails it will not send the other requests,
which will pollute our account

* Refactor out a duplicate test (#353)

This test was a duplicate of line 65 in the same file: 'Accepts credentials as an argument'

* Added support for sources in video tag (#265)

(Update video HTML 5 tag to include support for h265 and vp9)

* Version 1.20.0

* Refactor in a wait period for eager uploads (#355)

* Refactor in a wait period for eager uploads

* Improve provisioning api tests (#354)

Refactor out a duplicate test
improve provisioning API tests for GET sub_accounts
- Add a test for prefix
- Add a test for ids[] in the API
Fix prefix name in "update_sub_account" test
Improve the atomic-ness of the provisioning test

* updated promise types for resources methods (#358)

* updated types for resources promises

* Refactor test descriptions

* Added types for upload response callback (#360)

* Add types for Structured Metadata functions (#359)

* Add types for Structured Metadata functions

* Align all structured metadata tests with reference implementation (#351)

* Align all structured metadata tests with reference implementation

* Add `rate_limits` back to responses sent from Admin API (#361)

* Add `rate_limits` back to responses sent from Admin API

* Version 1.21.0

* Verify protocol in `CLOUDINARY_URL` and `CLOUDINARY_ACCOUNT_URL` (#363)

* Update issue templates (#365)

* Add support for 32 char SHA-256 signatures (#368)

* Fix normalize_expression when variable is named like a keyword (e.g., "$width") (#367)

* Fix `normalize_expression` to ignore predefined variables

* Refactor location of test files (#373)

* Update mocha and add mocha-types (#374)

* Tests - Create testConstants file 

- Create /testUtils/testConstants
- Remove all TIMEOUT constants from spechelper
- Fix all imports for other spec files

* Fix incorrect text implementation for list resources(#382)

* Rename utility function "only" (#379)

* Refactor the location of all custom assertions

* Refactor - Consolidate constants in one file

* Rename UNIQUE_JOB_ID to UNIQUE_JOB_SUFFIX (#384)

* Feature/support download backup version api (#380)

* Add support for pow operator in expressions (#386)

* Feature/create temporary clouds for tests (#372)

* Allow external PRs to pass tests using a temporary account

* Enabled skipped tests caused by running the suites on a fresh cloud (#387)

- Add timeouts to ensure files have a chance to update
- Fix an implementation bug on the wait() function
- Move wait() to a utility file
- Create a sample.jpg file when creating a new cloud

* updated tests to run linter (#388)

* updated tests to run linter

* Support for creating folders using Admin API (#370)

* Add `create_folder` to Admin API

* Add deprecation warning for node 6, add tests for node 14 (#389)

* Added indent rule (#390)

* Enable eslint rule no-unreachable (#393)

* Add eslint rule - Prefer dot notation (#392)

* Add support for cinemagraph_analysis parameter in upload, explicit, and resource (#391)

* Mock the test that tries to upload a gs file (#394)

* Add retry to integration tests (#396)

* Add retry to integration tests

* Feature/encode sdk version (#371)

* Add query param with SDK encoded suffix (_s=...)
* Set default behaviour to false (instead of true), and add a test to enforce it

* Make sure Uploader tests cleanup after themselves (#401)

- Add an upload tag to uploads done through specHelper

* Fix test description in `uploader.create_archive` (#400)

* Add missing types for sign-request (#398)

* Version 1.22.0

* Move tests to specific folders based on area (#399)

* Move tests to specific folders based on area

* Fix typo in destroy method (resource_types should be resource_type (#402)

`resource_types` should be `resource_type` as documented in https://cloudinary.com/documentation/image_upload_api_reference#optional_parameters-2

* Organize authToken tests into distinct areas (#409)

- One describe per file
- Tests for Image Tag
- Tests for URL generation
- Tests for genearting tokens using utils

* Add a command to run unit tests(without integration tests) (#408)

* Allow external PRs to pass Travis tests (#407)

- Exclude account API tests when tests are running in Travis from a Fork
- Fix string assignment to process.env variable (would incorrectly get the value of "undefined" (as a string))

* added download_folder method (#404)

* added download_folder method

* updated rule for comma spacing (#412)

* Add test for context metadata as user variables (#406)

* add test for context metadata as user variables

* Added rule for arrow-spacing (#413)

* Added comma dangle rule (#414)

* Support `max_results` and `next_cursor` in `root_folders` and `sub_folders` (#411)

* added download backedup asset function (#415)

* added download backedup asset function

* Increase wait time for image upload for search api tests (#416)

* Detect data URLs with suffix in mime type (#418)

* Update nodeJS to use cloudinary-core for analytics (#419)

* Update nodeJS to use cloudinary-core for analytics

* Test refactoring and cleanup (#420)

- Clean up all of mocha CLI arguments (they were redundant)
- Add a single test entrypoint (setup.js) where we load all the global dependencies
- Remove all require('expect) from all files (they were not needed now)
- Remove all dotEnv require (not needed anymore, initiated through setup.js)
- Organize remaining tests to integration/unit folders respectfuly.
- Separated some tests into their own files

* Prepare using setup.js as the main entrypoint for tests (#426)

* Prepare using setup.js as the main entrypoint for tests

* Fixed test for manual moderation status (#424)

* Rename mockPromise to provideMockObjects (#428)

- Align all usages to be identical (name-wise)

* Create our custom Describe() to always reset the config in every before (#427)

* Create our custom Describe() to always reset the config in every before()

- Reset config per describe()
- Refactor tests that abused nested describe for no reason, and failed after this change.

* Add tests for new OCR features (#385)

* Refactor sharedExamples where possible. (#430)

- api_spec
- image_spec

The only remaining work is in util_spec, but that file has proven to be far
too complex to refactor at this time

* Refactor `pickOnlyExistingValues()` function (#432)

* Add support for metadata array value (#433)

* Add support for metadata array value

* Fix and improve docstring for download_folder() (#434)

* Fix and improve docstring for `download_folder()`

* Add support for pending, prefix, and sub_account_id to users method (#417)

* Add support for `pending`, `prefix`, and `sub_account_id` to `users` method
* Add tests for pending, prefix, and sub_account_id parameters to users method

* Create pull_request_template.md (#435)

* Update pull_request_template.md (#437)

* Test: Ignore URL in AuthToken generation if ACL is provided (#431)

* Fix invalid detection failing test (#439)

* Fix docstring for pending parameter of the users method (#436)

* Added wait to versions test (#422)

* Added linter rules (#423)

* Fix/turn off no else return (#441)

* turned of no-else-return linter rule

* Skip dtslint in older node versions (#444)

* Casing fixes in pull_request_template.md (#442)

* Update pull_request_template.md

* Fix inconsistent casing

* Add eval parameter to upload params (#438)

* Add eval parameter support to upload

* Version 1.23.0

* Generate url-safe base64 strings in remote custom functions (#446)

* Fix the build docs command (#445)

* Fixed grammar misakes (repeated words) in a few tests (#450)

* Encode all URI components when building a URL in base_api_url() (#447)

* Set the provisioning API config as optional (#451)

* Fix type of generate_auth_token options (#448)

* Add url option to AuthTokenApiOptions

* Add a mention to the supported node version (#455)

* Remove unused parameter from archive_params (#454)

* secure_distribution has wrong type (#462)

If I am following https://cloudinary.com/documentation/cloudinary_sdks#configuration_parameters correctly the type should be string. But it seems that a boolean is acceptable. Changing to string to see if someone can take a look if this is accurate. I should be able to place my cname in here.

* Add support for date parameter in usage API (#467)

* Update docstring for normalize_expression (#461)

* Add `accessibility_analysis` parameter support (#463)

* Change test for `eval` upload parameter (#468)

* add missing esy compiled files (#470)

* Version 1.24.0

* added filename override param (#471)

* Add validation to genreate_auth_token to enforce url or acl (#472)

* Add validation to genreate_auth_token to enforce url or acl in the object

* Add missing types to create/delete_folder and private_download_url (#473)

* updated docstring (#475)

* added sort by metadata field (#474)

* Fix config backup in sign requests test (#476)

* updated safe base64 to all url generation (#477)

* Version 1.25.0

* Add SHA-256 support for auth signatures (#479)

* Fix return type of api_url function(return String instead of Promise) (#483)

* Fix/unhandled promise rejection call api (#481)

* Add an additional option to disable promises in call_api -- disable_promises:bool

* Version 1.25.1

* #486 Fix upload_prefix configuration retrieval (#487)

* Add support for complex variable names (aheight) (#488)

* Remove file extensions from require statements (#490)

* Version 1.25.2

* Add support for oauth authorization (#489)

* Version 1.26.0

* Update analyze_api branch to master

Co-authored-by: Asi Sayag <42962333+asisayag2@users.noreply.github.com>
Co-authored-by: RTLcoil <RTLcoil@users.noreply.github.com>
Co-authored-by: Jenkins <jenkins@cloudinary.com>
Co-authored-by: strausr <Raya@cloudinary.com>
Co-authored-by: Patricia Sanes <psanesl93@gmail.com>
Co-authored-by: Hirofumi Wakasugi <baenej@gmail.com>
Co-authored-by: Alex Patterson <alex@ajonp.com>
Co-authored-by: cloudinary-bot <sdkintegration@cloudinary.com>
Co-authored-by: Vlad Chermenev <chermenevv@gmail.com>
Co-authored-by: Marco Spengler <marco.spengler@skriptfabrik.com>
@patrick-tolosa patrick-tolosa deleted the feature/sha256-auth-signature branch August 31, 2021 08:08
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