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.

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

Merged
merged 10 commits into from
Oct 5, 2020
1 change: 1 addition & 0 deletions doc/api.md
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
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', '%s')

module.exports = {
codes,
Expand Down
54 changes: 51 additions & 3 deletions lib/impl/AzureBlobFiles.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,12 @@ class AzureBlobFiles extends Files {
* @memberof AzureBlobFiles
* @private
*/
constructor (credentials, tvm) {
constructor (credentials, tvm = null) {
super()
/** @private */
this.tvm = tvm
moritzraho marked this conversation as resolved.
Show resolved Hide resolved
/** @private */
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 All @@ -95,6 +98,7 @@ class AzureBlobFiles extends Files {
/** @private */
this._azure = {}
this._azure.aborter = azure.Aborter.none
this.credentials = utils.clone(credentials)
if (credentials.sasURLPrivate) {
const azureCreds = new azure.AnonymousCredential()
const pipeline = azure.StorageURL.newPipeline(azureCreds)
Expand Down Expand Up @@ -335,9 +339,17 @@ class AzureBlobFiles extends Files {
* @private
*/
_getUrl (filePath) {
let hostName = DEFAULT_CDN_STORAGE_HOST
const azureURL = this._propsForPath(filePath).blockBlobURL.url.split('?')[0]
if (this.hasOwnCredentials) {
if (this.credentials.hostName) {
hostName = this.credentials.hostName
} else {
return azureURL
}
}
const index = azureURL.indexOf(AZURE_STORAGE_DOMAIN)
return 'https://' + DEFAULT_CDN_STORAGE_HOST + azureURL.substring(index + AZURE_STORAGE_DOMAIN.length)
return 'https://' + hostName + azureURL.substring(index + AZURE_STORAGE_DOMAIN.length)
}

/**
Expand All @@ -355,10 +367,46 @@ class AzureBlobFiles extends Files {
}

const presignOptions = Object.assign({ blobName: filePath }, options)
const cred = await utils.wrapTVMRequest(this.tvm.getAzureBlobPresignCredentials(presignOptions))

let cred
if (this.hasOwnCredentials) {
// generate signature based on options and using own credentials
cred = await this._getAzureBlobPresignCredentials(presignOptions)
} else {
cred = await utils.wrapTVMRequest(this.tvm.getAzureBlobPresignCredentials(presignOptions))
}

return this._getUrl(filePath) + '?' + cred.signature
}

/**
* @memberof AzureBlobFiles
* @private
*/
async _getAzureBlobPresignCredentials (params) {
if (this._azure.sasCreds) {
const msg = 'generatePresignURL is not supported with Azure Container SAS credentials, please initialize the SDK with Azure storage account credentials instead'
logAndThrow(new codes.ERROR_UNSUPPORTED_OPERATION({ messageValues: [msg], sdkDetails: params }))
}

const sharedKeyCredential = new azure.SharedKeyCredential(this.credentials.storageAccount, this.credentials.storageAccessKey)
const containerName = this.credentials.containerName
// generate SAS token
const expiryTime = new Date(Date.now() + (1000 * params.expiryInSeconds))

const permissions = azure.BlobSASPermissions.parse(params.permissions)
const commonSasParams = {
permissions: permissions.toString(),
expiryTime: expiryTime,
blobName: params.blobName
}

const sasQueryParamsPrivate = azure.generateBlobSASQueryParameters({ ...commonSasParams, containerName: containerName }, sharedKeyCredential)
return {
signature: sasQueryParamsPrivate.toString()
}
}

/**
* @memberof AzureBlobFiles
* @override
Expand Down
22 changes: 9 additions & 13 deletions lib/init.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,24 +57,20 @@ async function init (config = {}) {
// 1. set provider
const provider = 'azure' // only azure is supported for now

// 2. instantiate tvm
let tvm
/* istanbul ignore else */
if (provider === 'azure') {
logger.debug('init with openwhisk credentials.')
// remember config.ow can be empty if env vars are set
const tvmArgs = { ow: config.ow, ...config.tvm }
tvm = await TvmClient.init(tvmArgs)
}

// 3. return state store based on provider
// 2. return state store based on provider
switch (provider) {
case 'azure':
if (config.azure) {
logger.debug('init with azure blob credentials.')
return AzureBlobFiles.init(config.azure, tvm)
return AzureBlobFiles.init(config.azure, null)
} else {
logger.debug('init with openwhisk credentials.')
// remember config.ow can be empty if env vars are set
const tvmArgs = { ow: config.ow, ...config.tvm }
const tvm = await TvmClient.init(tvmArgs)
return AzureBlobFiles.init(await utils.wrapTVMRequest(tvm.getAzureBlobCredentials()), tvm)
}
return AzureBlobFiles.init(await utils.wrapTVMRequest(tvm.getAzureBlobCredentials()), tvm)

// default:
// throw new FilesError(`provider '${provider}' is not supported.`, FilesError.codes.BadArgument)
}
Expand Down
1 change: 1 addition & 0 deletions lib/types.jsdoc.js
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
8 changes: 7 additions & 1 deletion lib/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,13 @@ function withHiddenFields (toCopy, fields) {
return copyConfig
}

// eslint-disable-next-line jsdoc/require-jsdoc
function clone (toCopy) {
return cloneDeep(toCopy)
}

module.exports = {
withHiddenFields,
wrapTVMRequest
wrapTVMRequest,
clone
}
86 changes: 76 additions & 10 deletions test/impl/AzureBlobFiles.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ const fakeSASCredentials = {
sasURLPrivate: 'https://fake.com/private?secret=abcd',
sasURLPublic: 'https://fake.com/public?secret=abcd'
}
const fakeUserCredentials = {
containerName: 'fake',
storageAccessKey: 'fakeKey',
storageAccount: 'fakeAccount'
}
const fakeAborter = 'fakeAborter'

const DEFAULT_CDN_STORAGE_HOST = 'https://firefly.azureedge.net'
Expand Down Expand Up @@ -52,11 +57,6 @@ const testWithProviderError = async (boundFunc, providerMock, errorDetails, file
describe('init', () => {
const fakeAzureAborter = 'fakeAborter'
const mockContainerCreate = jest.fn()
const fakeUserCredentials = {
containerName: 'fake',
storageAccessKey: 'fakeKey',
storageAccount: 'fakeAccount'
}

beforeEach(async () => {
mockContainerCreate.mockReset()
Expand Down Expand Up @@ -495,18 +495,35 @@ describe('_copyRemoteToRemoteFile', () => {
})

describe('_getUrl', () => {
const fakeAzureAborter = 'fakeAborter'
const mockContainerCreate = jest.fn()

const mockBlockBlob = jest.fn()
const setMockBlobUrl = url => {
azure.BlockBlobURL.fromContainerURL = mockBlockBlob.mockReturnValue({ url })
}

const tvm = jest.fn()

/** @type {AzureBlobFiles} */
let files

beforeEach(async () => {
mockBlockBlob.mockReset()

tvm.getAzureBlobPresignCredentials = jest.fn()
tvm.getAzureBlobPresignCredentials.mockResolvedValue({
signature: 'fakesign'
})

azure.ContainerURL = jest.fn()
files = await AzureBlobFiles.init(fakeSASCredentials)
files = await AzureBlobFiles.init(fakeSASCredentials, tvm)
files._azure.aborter = fakeAborter

mockContainerCreate.mockReset()
azure.ContainerURL = { fromServiceURL: jest.fn() }
azure.Aborter = { none: fakeAzureAborter }
azure.ContainerURL.fromServiceURL.mockReturnValue({ create: mockContainerCreate })
})

test('url with no query args', async () => {
Expand All @@ -524,28 +541,54 @@ describe('_getUrl', () => {
const url = files._getUrl('fakesub/afile')
expect(url).toEqual(expectedUrl)
})

test('test _getUrl custom host', async () => {
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'
const url = files._getUrl('fakesub/afile')
expect(url).toEqual(expectedUrl)
})
})

describe('_getPresignUrl', () => {
const fakeAzureAborter = 'fakeAborter'
const mockContainerCreate = jest.fn()

const mockBlockBlob = jest.fn()
const setMockBlobUrl = url => {
azure.BlockBlobURL.fromContainerURL = mockBlockBlob.mockReturnValue({ url })
}
const tvm = jest.fn()
/** @type {AzureBlobFiles} */
let files
azure.generateBlobSASQueryParameters = jest.fn()
azure.BlobSASPermissions.parse = jest.fn()

beforeEach(async () => {
tvm.mockReset()
azure.generateBlobSASQueryParameters.mockReset()
azure.BlobSASPermissions.parse.mockReset()

mockContainerCreate.mockReset()
mockBlockBlob.mockReset()
azure.ContainerURL = jest.fn()
azure.ContainerURL.fromServiceURL = jest.fn()
azure.ContainerURL.fromServiceURL.mockReturnValue({ create: mockContainerCreate })
azure.Aborter = { none: fakeAzureAborter }

// defaults that work
azure.generateBlobSASQueryParameters.mockReturnValue({ toString: () => 'fakeSAS' })
azure.BlobSASPermissions.parse.mockReturnValue({ toString: () => 'fakePermissionStr' })

tvm.getAzureBlobPresignCredentials = jest.fn()
tvm.getAzureBlobPresignCredentials.mockResolvedValue({
signature: 'fakesign'
})

mockBlockBlob.mockReset()
azure.ContainerURL = jest.fn()
files = await AzureBlobFiles.init(fakeSASCredentials)
files = await AzureBlobFiles.init(fakeSASCredentials, tvm)
files._azure.aborter = fakeAborter
files.tvm = tvm
})

test('_getPresignUrl with no options', async () => {
Expand All @@ -571,6 +614,29 @@ 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 () => {
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 = await AzureBlobFiles.init(fakeUserCredentials)
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 })
expect(url).toEqual(expectedUrl)
})

test('_getPresignUrl with correct options explicit permission own credentials', async () => {
files = await AzureBlobFiles.init(fakeUserCredentials)
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('_getPresignUrl with correct options explicit permission own sas credentials', async () => {
files = await AzureBlobFiles.init(fakeSASCredentials)
await expect(files._getAzureBlobPresignCredentials('fakesub/afile', { expiryInSeconds: 60 })).rejects.toThrow('[FilesLib:ERROR_UNSUPPORTED_OPERATION] generatePresignURL is not supported with Azure Container SAS credentials, please initialize the SDK with Azure storage account credentials instead')
})
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

})

describe('_statusFromProviderError', () => {
Expand Down
6 changes: 2 additions & 4 deletions test/init.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,8 @@ describe('init', () => {
test('with azure config', async () => {
await filesLib.init({ azure: fakeAzureBlobConfig })
expect(AzureBlobFiles.init).toHaveBeenCalledTimes(1)
expect(AzureBlobFiles.init).toHaveBeenCalledWith(fakeAzureBlobConfig, {
getAzureBlobCredentials: azureBlobTvmMock
})
expect(TvmClient.init).toHaveBeenCalledTimes(1)
expect(AzureBlobFiles.init).toHaveBeenCalledWith(fakeAzureBlobConfig, null)
expect(TvmClient.init).toHaveBeenCalledTimes(0)
expect(global.mockLogDebug).toHaveBeenCalledWith(expect.stringContaining('azure'))
checkInitDebugLogNoSecrets(fakeAzureBlobConfig.storageAccessKey)
checkInitDebugLogNoSecrets(fakeAzureBlobConfig.sasURLPrivate)
Expand Down
2 changes: 2 additions & 0 deletions types.d.ts
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