-
Notifications
You must be signed in to change notification settings - Fork 14
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
BYO creds related issue #66
Conversation
Sync with Master
Fixed issue with BYO creds Fixed issue with presign URL feature not working with BYO creds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first of all, I see two issues in the current implementation:
- there is one case which is not covered for presignURLs which is when the user brings his own storage via SAS credentials: i.e. when
await files.init({ azure: { sasURLPrivate: 'xxx', sasURLPublic: 'xxx' } })
, this behavior is defined in the API https://github.com/adobe/aio-lib-files/blob/master/doc/api.md#initconfig--promisefiles.files.generatePresignURL
should throw an error with a meaningful error message explaining that the user should instead provide account credentials to use the presign feature- An alternative is to remove support for init with SAS credentials, BUT it will be a breaking change and a lost in functionality
- in AuthManager.init the TVM is initialized even if the user brought his own storage credentials, which we shouldn't do, as the TVM is not needed in that case.
now, in my opinion the added abstraction is premature and adds complexity to the code base. I prefer the following approach:
- keep all the credential generation (tvm) logic in init, in pseudocode this is what I mean:
init (config) {
if (config.azure) {
// user brought his own credentials
AzureBlobFiles.init(config.azure, null)
} else {
// do init tvm
// do retrieve credentials via TVM
AzureBlobFiles.init(credentials, tvm) // pass tvm instance to be able to call tvm.getAzureBlobPresignCredentials
}
}
- in AzureBlobFiles keep things mostly as before, just change the
_getPresignUrl
function as follows:
_getPresignUrl () {
// case 1: init was done via OW credentials
if (this.tvm) {
// do generate presign via tvm
return presignURL
}
// case 2: bring your own account credentials
if (this.hasAccountCredentials) { // set this variable in init to true in init if credentials.storageAccount is set
// copy logic of AzureTVMWrapper.getAzureBlobPresignCredentials in here
}
// case 3: bring you own SAS credentials, not supported for presign url
logAndThrow(new codes.ERROR_XXX('<nice error message in here>'))
}
(Note: as an alternative AzureBlobFiles.init() could take a simple lambda function (tvm.getAzureBlobPresignCredentials.bind(tvm)
) as argument instead of the whole tvm instance)
Why I prefer the direct approach:
- With the proposed solution in this PR, contributors bringing a new Storage implementation will need to know about and implement 2 abstractions instead of one.
- the proposed abstraction is very Azure specific, it requires every child class to implement
getAzureBlobCredentials
andgetAzureBlobPresignCredentials
which wouldn't make sense for another storage provider. - Also the generation of presign urls in other providers, e.g. AWS could maybe work without doing an extra request to the TVM. In those cases the abstraction would not be very useful.
Ultimately, I don't think it's a bad idea to wrap the credential handling in an abstraction, but I would wait for another cloud storage to be supported first, to give us a better idea of what needs to be included and what not.
Such an abstraction could eventually be even reused by state lib.
But maybe we need a third eye on this 🙂, @purplecabbage wdyt?
Codecov Report
@@ Coverage Diff @@
## master #66 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 5 5
Lines 423 445 +22
Branches 81 85 +4
=========================================
+ Hits 423 445 +22
Continue to review full report at Codecov.
|
Added tests
Updated doc fixed tests for above change
lib/impl/AzureBlobFiles.js
Outdated
@@ -383,6 +382,8 @@ class AzureBlobFiles extends Files { | |||
* @private | |||
*/ | |||
async _getAzureBlobPresignCredentials (params) { | |||
if (this._azure.sasCreds) { logAndThrow(new codes.ERROR_UNSUPPORTED_OPERATION({ messageValues: ['generatePresignURL'], sdkDetails: params })) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the condition should be this.hasOwnCredentials && this._azure.sasCreds
because tvm credentials ARE SAS credentials
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we already check for this.hasOwnCredentials
within _getPresignUrl call, so we don't need to repeat that check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok 👍
lib/FilesError.js
Outdated
@@ -45,6 +45,7 @@ E('ERROR_FILE_NOT_EXISTS', 'file `%s` does not exist') | |||
E('ERROR_BAD_FILE_TYPE', '%s') | |||
E('ERROR_OUT_OF_RANGE', '%s') | |||
E('ERROR_MISSING_OPTION', '%s') | |||
E('ERROR_UNSUPPORTED_OPERATION', 'Operation %s not supported for SAS credentails') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's make this more generic: use simply %s
lib/impl/AzureBlobFiles.js
Outdated
* @private | ||
*/ | ||
async _getAzureBlobPresignCredentials (params) { | ||
if (this._azure.sasCreds) { logAndThrow(new codes.ERROR_UNSUPPORTED_OPERATION({ messageValues: ['generatePresignURL'], sdkDetails: params })) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we make the messageValues
a bit more precise : ['generatePresignURL are not supported with Azure Container SAS credentials, please initialize the SDK with Azure storage account credentials instead']
?
@@ -571,6 +606,41 @@ describe('_getPresignUrl', () => { | |||
expect(url).toEqual(expectedUrl) | |||
expect(tvm.getAzureBlobPresignCredentials).toHaveBeenCalledWith({ blobName: 'fakesub/afile', expiryInSeconds: 60, permissions: 'fake' }) | |||
}) | |||
|
|||
test('_getPresignUrl with correct options default permission own credentials', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the 4 tests should not simulate an init by setting private control variables. Instead those variable should be set by the constructor depending on the init input.
for example instead of setting files.hasOwnCredentials=true
and files.credentials=fakeUserCredentials
and files._azure.sasCreds=false
you could do files = await AzureBlobFiles.init(fakeUserCredentials, null)
=> this will do the same but doesn't test internal control variable names which could change in the future
files._azure.sasCreds = false | ||
const ret = await files._getAzureBlobPresignCredentials('fakesub/afile', { expiryInSeconds: 60 }) | ||
expect(ret).toEqual({ signature: 'fakeSAS' }) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing test with tvm credentials ? you could pass a tvm mock to the init =>
mockFn = jest.fn()
mockfiles = await AzureBlobFiles.init(fakeSASCredentials, { getAzureBlobPresignCredentials: mockFn })
then you can test
expect(mockFn.getAzureBlobPresignCredentials).toHaveBeenCalled()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is specific unit test for the private function _getAzureBlobPresignCredentials
, there are other tests which mock tvm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I see, I think we should test against _getPresignUrl'
directly with different tvm, byo account and byo sas credential. I don't think we should test against _getAzureBlobPresignCredentials as it is not exposed by the Files interface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks for doing the changes @sandeep-paliwal
Relates to #52, #62 and #55
Description
Fixed issue with using BYO azure creds
Fixed issue with presign URL not working with BYO creds
Code refactoring to manage above changes
Fix test failure because of code refactoring
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: