search

benjreinhart / react-native-aws3

Pure JavaScript React Native library for uploading to AWS S3
MIT License
399 stars 151 forks source link

Add X-AMZ-SECURITY-TOKEN header w/ sessionToken. #9

Closed cgcardona closed 7 years ago

cgcardona commented 8 years ago

Thanks for the great React Native lib. I found that it needed to be slightly tweaked to work w/ Cognito credentials.

I added an optional sessionToken property to the options and then conditionally add the X-AMZ-SECURITY-HEADER based on it's presence.

Thanks again.

benjreinhart commented 8 years ago

@cgcardona Thanks for the contribution!

This looks good. On a high level, I'm a bit nervous about this given I have not used cognito myself and don't have a quick, easy way to test this. The code is simple enough that I feel fine about it, just worried about maintaining this in the long term. For instance, I am still planning on taking another pass at presigned URLs, see #2. When I make changes like that, nervous that I could break your change. Unfortunately, I'm not sure of a good way to automate tests since to test any of this is actually working we'd need to call AWS while running RN. I need to get some unit tests that can run in pure JS and then set up a test repo for end to end testing.

:shipit:

cgcardona commented 8 years ago

Here is the cleaned up README per your request.

Also, we realized that we don't want to return these creds down to our webapp via our webservice so we're also exploring the signed URL route. My intention is to hopefully still use this lib w/ that path as well so I'll push any changes I make your way.

And yes I agree about testing. We're still working through that flow as well. Currently I have some enzyme, mocha and chai test stubbed out but nothing major. I tried to get jest working w/ testutils but couldn't get it wired up.

Glad to contribute to the lib. I'm stoked to be working w/ RN and this lib seems like it's a huge win so glad to help in any way.

benjreinhart commented 8 years ago

Also, we realized that we don't want to return these creds down to our webapp via our webservice so we're also exploring the signed URL route.

@cgcardona I'd prefer to merge this only if at least one app is making use of this feature, so if you're not sure you're going to use this yet, mind if I hold off on merging this until you know you need it?

cgcardona commented 8 years ago

Of course not. I'll keep you posted if I make any meaningful changes regarding presigned urls.

benjreinhart commented 8 years ago

👍 I'll leave this here for now

benjreinhart commented 7 years ago

Going to close this for now. If others need it, we can reopen.

damathryx commented 7 years ago

Can we use this? I need to pass sessionToken.

JPTeasdale commented 7 years ago

Ditto, sessionToken is pretty important. It's not only used by cognito, any sort of assumeRole flow required this.

JPTeasdale commented 7 years ago

If anyone else finds this, you can install this pull request with

npm install --save react-native-aws3@benjreinhart/react-native-aws3#pull/9/head

benjreinhart commented 7 years ago

@JPTeasdale @damathryx I agree this should make it in soon. However, I'd be a bit careful about pointing to this PR as it is quite out of date with master. If someone wants to submit a clean & tested PR to master branch with this I will definitely consider merging it!

benjreinhart commented 7 years ago

Also, if you do submit a PR for this again, mind providing the steps required to test this with my S3 account and any other context? I haven't used this myself, so the easier it is for me to test it the more likely I'll have time to review/merge.

amanthegreatone commented 7 years ago

mybigday/react-native-s3 supports cognito. Maybe you can look at that repo and get this feature working.

benjreinhart commented 7 years ago

@amanthegreatone If you're interested in this functionality, you too could look at that repo and provide a thought out PR. I don't often have time to work on this as I am not actively developing in React Native land.

amanthegreatone commented 7 years ago

@benjreinhart I am not familiar with objective-c :( otherwise would have readily obliged to do it. Can try it out only for android but a half solution would not be acceptable I guess.

kevinresol commented 7 years ago

This works, can you simply reopen and merge it? @benjreinhart

yongzhi-chen commented 6 years ago

I think this deserves more attention because as far as I know it is not secure to put secretKey and accessKey in the react native javascript code as people can reverse engineering it (see https://github.com/facebook/react-native/issues/1093). So temporal session should be the way to go.