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.

Sign up for GitHub

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

Jump to bottom

BYO creds related issue #66

Merged
merged 10 commits into from
Oct 5, 2020
Merged

BYO creds related issue #66

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
45d86b3
Merge pull request #4 from adobe/master
sandeep-paliwal Sep 24, 2020
7fed333
Code refactoring to abstract TVM behaviour from Files impl
sandeep-paliwal Sep 29, 2020
b2131a4
Made changes as per review comments
sandeep-paliwal Oct 5, 2020
48128e4
added tests
sandeep-paliwal Oct 5, 2020
b406e00
support hostName in BYO credentials
sandeep-paliwal Oct 5, 2020
3b23711
Throw error in case of sas byo creds
sandeep-paliwal Oct 5, 2020
1b7053d
minor fix for byo sas creds
sandeep-paliwal Oct 5, 2020
17de909
closed review comments
sandeep-paliwal Oct 5, 2020
ff2e6b8
removed unnecessary tests
sandeep-paliwal Oct 5, 2020
12ebe86
added jsdoc comment
sandeep-paliwal Oct 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,6 +539,7 @@ Azure account credentials. Must have the permission to create containers.
| storageAccount | <code>string</code> | name of azure storage account |
| storageAccessKey | <code>string</code> | access key for azure storage account |
| containerName | <code>string</code> | name of container to store files. Another `${containerName}-public` will also be used for public files. |
| [hostName] | <code>string</code> | custom domain for returned URLs |

<a name="RemotePathString"></a>

Expand Down
1 change: 1 addition & 0 deletions lib/FilesError.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Copy link
Member

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


module.exports = {
codes,
Expand Down
5 changes: 3 additions & 2 deletions lib/impl/AzureBlobFiles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ class AzureBlobFiles extends Files {
constructor (credentials, tvm) {
super()
this.tvm = tvm
moritzraho marked this conversation as resolved.
Show resolved Hide resolved
this.hasOwnCredentials = false
this.hasOwnCredentials = (tvm === null)
moritzraho marked this conversation as resolved.
Show resolved Hide resolved
const cloned = utils.withHiddenFields(credentials, ['storageAccessKey', 'sasURLPrivate', 'sasURLPublic'])
logger.debug(`init AzureBlobFiles with config ${JSON.stringify(cloned, null, 2)}`)

Expand Down Expand Up @@ -110,7 +110,6 @@ class AzureBlobFiles extends Files {
this._azure.containerURLPublic = azure.ContainerURL.fromServiceURL(serviceURL, credentials.containerName + '-public')
this._azure.sasCreds = false
this.credentials = utils.clone(credentials)
this.hasOwnCredentials = true
}
}

Expand Down Expand Up @@ -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 })) }
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok 👍

Copy link
Member

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']
?


const sharedKeyCredential = new azure.SharedKeyCredential(this.credentials.storageAccount, this.credentials.storageAccessKey)
const containerName = this.credentials.containerName
// generate SAS token
Expand Down
1 change: 1 addition & 0 deletions lib/types.jsdoc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ governing permissions and limitations under the License.
* @property {string} storageAccessKey access key for azure storage account
* @property {string} containerName name of container to store files.
* Another `${containerName}-public` will also be used for public files.
* @property {string} [hostName] custom domain for returned URLs
*/
/**
* @typedef RemotePathString
Expand Down
12 changes: 10 additions & 2 deletions test/impl/AzureBlobFiles.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,7 @@ describe('_getUrl', () => {
})

test('test _getUrl custom host', async () => {
files = new AzureBlobFiles({ ...fakeUserCredentials, hostName: 'fakeHost' })
files = new AzureBlobFiles({ ...fakeUserCredentials, hostName: 'fakeHost' }, null)
const cleanUrl = 'https://fake.blob.core.windows.net/fake/fakesub/afile'
setMockBlobUrl(cleanUrl)
const expectedUrl = 'https://fakeHost/fake/fakesub/afile'
Expand Down Expand Up @@ -610,6 +610,7 @@ describe('_getPresignUrl', () => {
test('_getPresignUrl with correct options default permission own credentials', async () => {
Copy link
Member

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.hasOwnCredentials = true
files.credentials = fakeUserCredentials
files._azure.sasCreds = false
const cleanUrl = 'https://fake.blob.core.windows.net/fake/fakesub/afile'
setMockBlobUrl(cleanUrl)
const expectedUrl = 'https://fake.blob.core.windows.net/fake/fakesub/afile?fakeSAS'
Expand All @@ -620,16 +621,23 @@ describe('_getPresignUrl', () => {
test('_getPresignUrl with correct options explicit permission own credentials', async () => {
files.hasOwnCredentials = true
files.credentials = fakeUserCredentials
files._azure.sasCreds = false
const cleanUrl = 'https://fake.blob.core.windows.net/fake/fakesub/afile'
setMockBlobUrl(cleanUrl)
const expectedUrl = 'https://fake.blob.core.windows.net/fake/fakesub/afile?fakeSAS'
const url = await files._getPresignUrl('fakesub/afile', { expiryInSeconds: 60, permissions: 'fake' })
expect(url).toEqual(expectedUrl)
})

test('_getAzureBlobPresignCredentials with correct options', async () => {
test('_getAzureBlobPresignCredentials with sas creds', async () => {
files.hasOwnCredentials = true
await expect(files._getAzureBlobPresignCredentials('fakesub/afile', { expiryInSeconds: 60 })).rejects.toThrow('[FilesLib:ERROR_UNSUPPORTED_OPERATION] Operation generatePresignURL not supported for SAS credentails')
})

test('_getAzureBlobPresignCredentials with account creds', async () => {
files.hasOwnCredentials = true
files.credentials = fakeUserCredentials
files._azure.sasCreds = false
const ret = await files._getAzureBlobPresignCredentials('fakesub/afile', { expiryInSeconds: 60 })
expect(ret).toEqual({ signature: 'fakeSAS' })
})
Copy link
Member

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()

Copy link
Contributor Author

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

Copy link
Member

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

Expand Down
2 changes: 2 additions & 0 deletions types.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,11 +311,13 @@ export type AzureCredentialsSAS = {
* @property storageAccessKey - access key for azure storage account
* @property containerName - name of container to store files.
* Another `${containerName}-public` will also be used for public files.
* @property [hostName] - custom domain for returned URLs
*/
export type AzureCredentialsAccount = {
storageAccount: string;
storageAccessKey: string;
containerName: string;
hostName?: string;
};

/**
Expand Down