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

Azure blob access #94

Merged
merged 12 commits into from Feb 28, 2017

Conversation

Projects
None yet
3 participants
@elenasolomon
Contributor

elenasolomon commented Feb 10, 2017

Adds an API endpoint for getting a SAS string for use with a specific Azure blob storage container

Please do not merge it, because the implementation of Blob service is not yet merged.

@elenasolomon

This comment has been minimized.

Show comment
Hide comment
@elenasolomon

elenasolomon Feb 20, 2017

Contributor

@jonasfj based on your last email, I changed the scope to:
auth:azure-blob: level : account / container

Contributor

elenasolomon commented Feb 20, 2017

@jonasfj based on your last email, I changed the scope to:
auth:azure-blob: level : account / container

@jonasfj jonasfj self-requested a review Feb 20, 2017

Show outdated Hide outdated src/azure.js
if (!this.azureAccounts[account]) {
return res.status(404).json({
message: "Account '" + account + "' not found, can't delegate access"

This comment has been minimized.

@jonasfj

jonasfj Feb 20, 2017

Contributor

Use res.reportError I think it's called... See tc-lib-api

@jonasfj

jonasfj Feb 20, 2017

Contributor

Use res.reportError I think it's called... See tc-lib-api

@jonasfj

Mostly just small things...

Show outdated Hide outdated src/azure.js
if (['read-write', 'read-only'].indexOf(level) < 0) {
return res.status(404).json({

This comment has been minimized.

@jonasfj

jonasfj Feb 20, 2017

Contributor

Use res.reportError

@jonasfj

jonasfj Feb 20, 2017

Contributor

Use res.reportError

Show outdated Hide outdated src/azure.js
write: perm,
delete: perm,
list: perm

This comment has been minimized.

@jonasfj

jonasfj Feb 20, 2017

Contributor

Is list not read only? I'm not sure what it covers...

@jonasfj

jonasfj Feb 20, 2017

Contributor

Is list not read only? I'm not sure what it covers...

This comment has been minimized.

@elenasolomon

elenasolomon Feb 21, 2017

Contributor

You are right. 'List' gives the permission to list blobs in the container.

@elenasolomon

elenasolomon Feb 21, 2017

Contributor

You are right. 'List' gives the permission to list blobs in the container.

@jhford

This comment has been minimized.

Show comment
Hide comment
@jhford

jhford Feb 22, 2017

Contributor

@elenasolomon can you please rebase your patch when you have a chance. You'll also need to update the package.json to match the new version of fast-azure-storage.

@jonasfj do the review addressing commits look good? Once we land fast-azure-storage with blob storage, would you be OK with this being merged?

Contributor

jhford commented Feb 22, 2017

@elenasolomon can you please rebase your patch when you have a chance. You'll also need to update the package.json to match the new version of fast-azure-storage.

@jonasfj do the review addressing commits look good? Once we land fast-azure-storage with blob storage, would you be OK with this being merged?

@elenasolomon

This comment has been minimized.

Show comment
Hide comment
@elenasolomon

elenasolomon Feb 22, 2017

Contributor

I made the rebase and I changed also the version of 'fast-azure-storage' library

Contributor

elenasolomon commented Feb 22, 2017

I made the rebase and I changed also the version of 'fast-azure-storage' library

Show outdated Hide outdated schemas/azure-blob-response.yml
@@ -0,0 +1,22 @@
$schema: http://json-schema.org/draft-04/schema#
title: "Azure Shared-Access-Signature Response"

This comment has been minimized.

@jonasfj

jonasfj Feb 22, 2017

Contributor

Included Blob and Table in the two titles for this schema and the table schema... Afaik golang client uses the titles to generate types :)

@jonasfj

jonasfj Feb 22, 2017

Contributor

Included Blob and Table in the two titles for this schema and the table schema... Afaik golang client uses the titles to generate types :)

Show outdated Hide outdated src/azure.js
api.declare({
method: 'get',
route: '/azure/:account/blob/:container/:level',

This comment has been minimized.

@jonasfj

jonasfj Feb 22, 2017

Contributor

How about '/azure/:account/containers/:container/:level',

The other one we use: /azure/<account>/table/<table>/<level>
And we probably should have used /azure/<account>/tables/<table>/<level>. So that matches with the route for listing.

@jonasfj

jonasfj Feb 22, 2017

Contributor

How about '/azure/:account/containers/:container/:level',

The other one we use: /azure/<account>/table/<table>/<level>
And we probably should have used /azure/<account>/tables/<table>/<level>. So that matches with the route for listing.

This comment has been minimized.

@jonasfj

jonasfj Feb 22, 2017

Contributor

Also we should define a pattern for the container parameter here:
https://github.com/elenasolomon/taskcluster-auth/blob/d1eb601a2b9b5f2709f4c0f672a7d060d99169d1/src/v1.js#L57

Perhaps we can even define level there too... Since level is always /^(read-write|read-only)$/

@jonasfj

jonasfj Feb 22, 2017

Contributor

Also we should define a pattern for the container parameter here:
https://github.com/elenasolomon/taskcluster-auth/blob/d1eb601a2b9b5f2709f4c0f672a7d060d99169d1/src/v1.js#L57

Perhaps we can even define level there too... Since level is always /^(read-write|read-only)$/

Show outdated Hide outdated src/azure.js
account: account,
container: containerName,
level: level,
})) {

This comment has been minimized.

@jonasfj

jonasfj Feb 22, 2017

Contributor

Please use the short hand: req.satisfies({account, container: containerName, level})
If you changed containerName to container it would work even better :)

@jonasfj

jonasfj Feb 22, 2017

Contributor

Please use the short hand: req.satisfies({account, container: containerName, level})
If you changed containerName to container it would work even better :)

This comment has been minimized.

@jonasfj

jonasfj Feb 22, 2017

Contributor

Actually, a better test is:
level === 'read-only' && req.satisfies({account, container: containerName, level: 'read-write'}, true) && req.satisfies({account, container: containerName, level})

That way, the 'read-write' scope grants 'read-only' too...

Credits @imbstack for remembering this for tables...

@jonasfj

jonasfj Feb 22, 2017

Contributor

Actually, a better test is:
level === 'read-only' && req.satisfies({account, container: containerName, level: 'read-write'}, true) && req.satisfies({account, container: containerName, level})

That way, the 'read-write' scope grants 'read-only' too...

Credits @imbstack for remembering this for tables...

@jonasfj

Fix the nits... Change title to not start "DOT NOT MERGE" :) and we'll ship this...

@@ -1,5 +1,5 @@
$schema: http://json-schema.org/draft-04/schema#
title: "Azure Shared-Access-Signature Response"
title: "Azure Table Shared-Access-Signature Response"

This comment has been minimized.

@jonasfj

jonasfj Feb 24, 2017

Contributor

Thanks :)

@jonasfj

jonasfj Feb 24, 2017

Contributor

Thanks :)

Show outdated Hide outdated src/azure.js
return res.status(404).json({
message: "Account '" + account + "' not found, can't delegate access"
});
return res.reportError('InvalidRequestArguments',

This comment has been minimized.

@jonasfj

jonasfj Feb 24, 2017

Contributor

ResourceNotFound produces 404

@jonasfj

jonasfj Feb 24, 2017

Contributor

ResourceNotFound produces 404

Show outdated Hide outdated src/azure.js
// Check that the account exists
if (!this.azureAccounts[account]) {
return res.reportError('InvalidRequestArguments',

This comment has been minimized.

@jonasfj

jonasfj Feb 24, 2017

Contributor

Again this is probably a not found error

@jonasfj

jonasfj Feb 24, 2017

Contributor

Again this is probably a not found error

Show outdated Hide outdated test/azure_test.js
let auth = new helper.Auth({
baseUrl: helper.baseUrl,
credentials: rootCredentials,
authorizedScopes: [

This comment has been minimized.

@jonasfj

jonasfj Feb 24, 2017

Contributor

Helper has a quick method to set authorised scopes...

@jonasfj

jonasfj Feb 24, 2017

Contributor

Helper has a quick method to set authorised scopes...

This comment has been minimized.

@elenasolomon

elenasolomon Feb 27, 2017

Contributor

Which method? Sorry, but I do not find a method that can set authorizedScopes

@elenasolomon

elenasolomon Feb 27, 2017

Contributor

Which method? Sorry, but I do not find a method that can set authorizedScopes

This comment has been minimized.

@jonasfj

jonasfj Feb 27, 2017

Contributor

oh, we don't have it here...
In queue we do:

helper.auth = new helper.Auth({
baseUrl: helper.baseUrl,
credentials: {
clientId: 'root',
accessToken: cfg.app.rootAccessToken,
}
});

We could do the same here:

helper.auth = new helper.Auth({
baseUrl: helper.baseUrl,
credentials: {
clientId: 'root',
accessToken: cfg.app.rootAccessToken,
}
});

Ie. basically wrap with a helper.scopes method...

@jonasfj

jonasfj Feb 27, 2017

Contributor

oh, we don't have it here...
In queue we do:

helper.auth = new helper.Auth({
baseUrl: helper.baseUrl,
credentials: {
clientId: 'root',
accessToken: cfg.app.rootAccessToken,
}
});

We could do the same here:

helper.auth = new helper.Auth({
baseUrl: helper.baseUrl,
credentials: {
clientId: 'root',
accessToken: cfg.app.rootAccessToken,
}
});

Ie. basically wrap with a helper.scopes method...

@jonasfj

Adding the helper.scopes(...) function as is done in queue would be nice... But this looks ready to ship.

@jonasfj

This comment has been minimized.

Show comment
Hide comment
@jonasfj

jonasfj Feb 27, 2017

Contributor

@elenasolomon let us know when this isn't "[DO-NOT-MERGE]" anymore...

Contributor

jonasfj commented Feb 27, 2017

@elenasolomon let us know when this isn't "[DO-NOT-MERGE]" anymore...

@elenasolomon elenasolomon changed the title from [DO-NOT-MERGE]Azure blob access to Azure blob access Feb 28, 2017

@elenasolomon

This comment has been minimized.

Show comment
Hide comment
@elenasolomon

elenasolomon Feb 28, 2017

Contributor

@jonasfj The new version of fast-azure-storage is published :) Now we can merge this PR

Contributor

elenasolomon commented Feb 28, 2017

@jonasfj The new version of fast-azure-storage is published :) Now we can merge this PR

@jonasfj jonasfj merged commit e4d5660 into taskcluster:master Feb 28, 2017

@jonasfj

This comment has been minimized.

Show comment
Hide comment
@jonasfj

jonasfj Feb 28, 2017

Contributor

I'll update npm-shrinkwrap.json and land this in production...

Contributor

jonasfj commented Feb 28, 2017

I'll update npm-shrinkwrap.json and land this in production...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment