feat: accessibleAt v4 signed urls #1328

shaffeeullah · 2020-10-28T19:37:00Z

Resolves merge conflicts from #1079

Fixes #1041 🦕

…affeeullah/usableFromV4SignedURLs

codecov · 2020-10-29T23:16:24Z

Codecov Report

Merging #1328 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1328   +/-   ##
=======================================
  Coverage   99.00%   99.00%           
=======================================
  Files          14       14           
  Lines       11719    11775   +56     
  Branches      513      593   +80     
=======================================
+ Hits        11602    11658   +56     
  Misses        117      117

Impacted Files	Coverage Δ
src/file.ts	`99.94% <100.00%> (+<0.01%)`	⬆️
src/signer.ts	`99.77% <100.00%> (+0.01%)`	⬆️

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 7f3bb25...a989dce. Read the comment docs.

frankyn · 2020-10-30T23:02:56Z

src/file.ts

@@ -2723,6 +2724,10 @@ class File extends ServiceObject<File> {
   * @param {string} [config.responseDisposition] The
   *     [response-content-disposition parameter](http://goo.gl/yMWxQV) of the
   *     signed url.
+   * @param {*} [config.usableFrom=Date.now()] A timestamp when this link became usable. Any value


Is the name usableFrom readable to you or do you have recommendations for changing it?

It makes sense to me. The URL is usable from the date specified forward. I've been thinking about alternative names and haven't really come up with anything. I am open to suggestions though.

@frankyn what do you think of accessibleAt? (thanks @bcoe for the suggestion!)

frankyn · 2020-10-30T23:05:37Z

system-test/storage.ts

+        expires: expiresDate.setMinutes(expiresMinutes + 90),
+      });
+      const res = await fetch(signedReadUrl);
+      const body = await res.text();


Instead of validating the text. Validate the res status code which is less brittle.

frankyn · 2020-10-30T23:15:44Z

test/signer.ts

+        it('should accept strings', async () => {
+          const usableFrom = '12-12-2099';
+          const usableFromDate = new Date(usableFrom);
+          const expectedUsableFrom = '20991212T000000Z';


Could you update this to be provided from the Date object instead?

frankyn · 2020-10-30T23:16:46Z

test/signer.ts

+            version: 'v4',
+            method: 'GET',
+            usableFrom,
+            expires: usableFromDate.valueOf() + 5000,


Why use .valueOf() here instead of setX()?

setX modifies and returns the original object

frankyn · 2020-10-30T23:18:16Z

test/signer.ts

+            [string],
+            Promise<string>
+          > = sandbox.stub(authClient, 'sign').resolves('signature');
+          const usableFrom = new Date(1581984000000);


Nit: Make int constants named variables in a before() call so it can be reused.

frankyn · 2020-10-30T23:19:22Z

test/signer.ts

+        });
+
+        it('should throw if an expiration date from the before usableFrom date is given', () => {
+          const expires = Date.now() + 5;


Based on our pairing:

+ 5 is may not work as expected, could you update the line to increment based on the unit? e.g. second, minute, hour.

I have updated it to be 1 day difference. I am not incrementing by unit as to avoid converting to a date and then back to milliseconds. Do you think I should perform this conversion?

frankyn · 2020-10-30T23:20:43Z

src/signer.ts

@@ -408,6 +419,16 @@ export class URLSigner {

    return Math.round(expiresInMSeconds / 1000); // The API expects seconds.
  }
+
+  parseUsableFrom(usableFrom?: string | number | Date): number {


Did you try refactoring parseUsableFrom() into parseExpires() they look really similar?

parseExpires is ensuring that the expiration date is not in the past, while parseUsableFrom does not require this check. I could refactor the code so that it takes in a boolean parameter to determine whether this check is necessary (ie: is it being called for usableFrom or expires?). Do you think this makes sense? Further, the check on line 424 would have to be included in the joint function, as usableFrom is an optional parameter. While the false case will never trigger, it might be confusing to see that expires will default to the current date.

frankyn · 2020-10-30T23:23:07Z

src/signer.ts

@@ -140,6 +147,7 @@ export class URLSigner {
    const config: GetSignedUrlConfigInternal = Object.assign({}, cfg, {
      method,
      expiration: expiresInSeconds,
+      usableFrom: new Date(1000 * usableFromInSeconds),


Nit: not sure I understand 1000 * in this case, is it needed? (Comparing to expiration and it's not used in this way.

The expiration field is expecting number. usableFrom is expecting Date and the Date constructor accepts a number argument in milliseconds. The 1000 multiplier is to perform this conversion.

…leapis/nodejs-storage into shaffeeullah/usableFromV4SignedURLs

sofisl · 2020-11-02T18:35:42Z

@shaffeeullah, I reviewed this for readability, but will refer to @frankyn for library-specific code review.

frankyn

I saw you decided on accessibleAt sounds good and acking.

You have a linter issue and added a nit for the added example.

frankyn · 2020-11-02T23:20:57Z

src/file.ts

@@ -2747,7 +2746,8 @@ class File extends ServiceObject<File> {
   *
   * const config = {
   *   action: 'read',
-   *   expires: '03-17-2025'
+   *   expires: '03-17-2025',
+   *   accessibleAt: ''03-13-2025'


Double '.

Recommend adding another example separate from the simple Signed URL example.

frankyn · 2020-11-03T00:12:41Z

src/file.ts

+   * const config = {
+   *   action: 'read',
+   *   expires: '03-17-2025',
+   *   accessibleAt: '03-13-2025'


One potential issue with accessibleAt is that it may not fit well with using a Signed URL to upload data only downloading data.

WDYT?

From how I read it, 'accessible' refers to the link and not the file. The link to upload is only accessible from the date specified. I think it works.

Source-Link: googleapis/synthtool@89dd35d Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-nodejs:latest@sha256:89c5b2f3decec8ad64febbebea671076c119d1ab43700da380846a315600de8a

shaffeeullah added 7 commits October 22, 2020 18:53

feat: public url of a file

2b53bd7

removed log statement

9420a09

changed function to use apiEndpoint

9bd5de6

initial migration of usableFrom logic

007037a

Merge branch 'master' of github.com:googleapis/nodejs-storage into sh…

2ed384f

…affeeullah/usableFromV4SignedURLs

moved test

cf7239f

removed duplicated tests

31a3ed6

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 28, 2020

shaffeeullah added 4 commits October 29, 2020 16:28

moved tests to signer

7629b6e

changed test values

5644f2f

linted files

756436e

migrated test to signer

036eb7b

shaffeeullah added 4 commits October 30, 2020 20:17

updated tests

fe89954

removed undefined logic; linted files

dec152a

fixed failing test

d0a6db8

Merge branch 'master' into shaffeeullah/usableFromV4SignedURLs

0803aa6

shaffeeullah marked this pull request as ready for review October 30, 2020 21:42

shaffeeullah requested a review from a team as a code owner October 30, 2020 21:42

shaffeeullah requested a review from frankyn October 30, 2020 21:43

frankyn requested changes Oct 30, 2020

View reviewed changes

shaffeeullah added 2 commits November 2, 2020 18:12

refactored tests

6b06e63

Merge branch 'shaffeeullah/usableFromV4SignedURLs' of github.com:goog…

b2aaca9

…leapis/nodejs-storage into shaffeeullah/usableFromV4SignedURLs

shaffeeullah requested a review from frankyn November 2, 2020 18:14

sofisl approved these changes Nov 2, 2020

View reviewed changes

changed usableFrom to accessibleAt, added example, named 1000 constant

e062acc

frankyn requested changes Nov 2, 2020

View reviewed changes

added seperate example for accessibleAt; linted files

e84f7e1

shaffeeullah changed the title ~~feat: usableFrom v4 signed urls~~ feat: accessibleAt v4 signed urls Nov 3, 2020

shaffeeullah requested a review from frankyn November 3, 2020 00:09

frankyn reviewed Nov 3, 2020

View reviewed changes

frankyn approved these changes Nov 3, 2020

View reviewed changes

product-auto-label bot added the api: storage Issues related to the googleapis/nodejs-storage API. label Nov 3, 2020

Merge branch 'master' into shaffeeullah/usableFromV4SignedURLs

a989dce

shaffeeullah merged commit 1e0295e into master Nov 3, 2020

shaffeeullah deleted the shaffeeullah/usableFromV4SignedURLs branch November 3, 2020 22:23

shaffeeullah mentioned this pull request Nov 3, 2020

feat: allow date to be specified for v4 signed URLs #1079

Closed

4 tasks

gcf-merge-on-green bot pushed a commit that referenced this pull request Jan 19, 2022

build(node): switch back to keystore for publication (#1328) (#1759)

469056a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: accessibleAt v4 signed urls #1328

feat: accessibleAt v4 signed urls #1328

shaffeeullah commented Oct 28, 2020 •

edited

Loading

codecov bot commented Oct 29, 2020 •

edited

Loading

frankyn Oct 30, 2020

shaffeeullah Nov 2, 2020

shaffeeullah Nov 2, 2020

frankyn Oct 30, 2020

frankyn Oct 30, 2020

frankyn Oct 30, 2020

shaffeeullah Nov 2, 2020

frankyn Oct 30, 2020

frankyn Oct 30, 2020

shaffeeullah Nov 2, 2020

frankyn Oct 30, 2020

shaffeeullah Nov 2, 2020

frankyn Oct 30, 2020

shaffeeullah Nov 2, 2020

sofisl commented Nov 2, 2020

frankyn left a comment

frankyn Nov 2, 2020

frankyn Nov 3, 2020

shaffeeullah Nov 3, 2020

feat: accessibleAt v4 signed urls #1328

feat: accessibleAt v4 signed urls #1328

Conversation

shaffeeullah commented Oct 28, 2020 • edited Loading

codecov bot commented Oct 29, 2020 • edited Loading

Codecov Report

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

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

sofisl commented Nov 2, 2020

frankyn 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

shaffeeullah commented Oct 28, 2020 •

edited

Loading

codecov bot commented Oct 29, 2020 •

edited

Loading