GEOLYTIX / xyz

An open source javascript framework for spatial data and application interfaces.
MIT License
88 stars 26 forks source link

API Sign: Change the S3 Provider into a signer #1698

Closed AlexanderGeere closed 1 week ago

AlexanderGeere commented 2 weeks ago

Description

Provides an api endpoint api/sign/s3 Which returns a signed URL.

GitHub Issue

#1607

Type of Change

How have you tested this?

Tested with bugs_testing/sign/s3_workspace.json

Testing Checklist

Code Quality Checklist

dbauszus-glx commented 2 weeks ago

How are requests handled where signing fails? eg. missing resource, missing credentials.

AlexanderGeere commented 2 weeks ago

@dbauszus-glx

How are requests handled where signing fails? eg. missing resource, missing credentials.

Signing will always succeed even if the credentials are wrong, calling the signed url will return a 403.

dbauszus-glx commented 2 weeks ago

I don't understand this bit. Can you comment on this for me to test.

  if (provider[req.params.provider]?.deprecated){
    return await provider[req.params.provider].fn(req, res)
  }
dbauszus-glx commented 2 weeks ago

I added S3 to requires of the sign module and updated the docs in regards to exports null from signer modules. https://github.com/GEOLYTIX/xyz/pull/1698/commits/90bc15e2b23d727d8e110b00af4b4f74022414cc

dbauszus-glx commented 2 weeks ago

I have updated the S3 module docs with reference to the credentials required from AWS and optional dependencies.

image

AlexanderGeere commented 2 weeks ago

I don't understand this bit. Can you comment on this for me to test.

  if (provider[req.params.provider]?.deprecated){
    return await provider[req.params.provider].fn(req, res)
  }

@dbauszus-glx Turns out this is no longer necessary

dbauszus-glx commented 2 weeks ago

@AlexanderGeere

The s3_provider doesn't have to be async or deprecated. We can just require the s3_signer and exports null or s3_provider dependent on the avilability of the s3 signer.

https://github.com/GEOLYTIX/xyz/pull/1698/commits/2a8523161ca95e38cef83d25c99dc22f05e9a32d

The idea is that eventually the s3 provider will sign a request and return the payload as this might be used inside the process. ie the workspace could be stored in an s3 bucket.

For now the s3_provider works exactly as the s3 signer.

AlexanderGeere commented 2 weeks ago

@RobAndrewHurst After a quick chat with @dbauszus-glx public url are not signed so these should just be handled in the plugin. Could Maybe do a public: true or similar

RobAndrewHurst commented 2 weeks ago

Happy with that! :)

dbauszus-glx commented 2 weeks ago

I bumped the optional dependencies to their current versions 3.691.0

AWS recommends to use ListObjectsV2Command in development. The ListObjectsCommand is only kept for legacy purposes.

I also changed the s3 signer to return the signed URL as a text which it is.

dbauszus-glx commented 2 weeks ago

Rather than providing command maps, and action dictionaries to translate commands and properties back and forth it will be much easier to talk directly with the SDK using the commands as documented in the S3 documentation.

https://github.com/GEOLYTIX/xyz/pull/1698/commits/9cce4f17d1e651693a0ff978a04a02bb25db2baf

sonarcloud[bot] commented 2 weeks ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

simon-leech commented 2 weeks ago

Don't feel like I'm best placed to review this one so removed my request. If you two @dbauszus-glx and @RobAndrewHurst are happy, that's great with me!