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

Avoid panics in NewSharedKeyCredential #63

Merged
merged 2 commits into from
Aug 18, 2018
Merged

Avoid panics in NewSharedKeyCredential #63

merged 2 commits into from
Aug 18, 2018

Conversation

zezha-msft
Copy link
Contributor

No description provided.

tombuildsstuff
tombuildsstuff suggested changes Aug 17, 2018
View reviewed changes
Copy link

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @zezha-msft

Thanks for pushing a PR to start fixing the issue raised in #59 😄

I took a look through and whilst I don't work on this library - I've spotted some errors which aren't being handled and crashes that are possible which I thought are worth raising (perhaps less so for the tests, but certainly for the examples).

Hope that's helpful - thanks!

2018-03-28/azblob/zt_test.go Outdated
@@ -1246,7 +1246,7 @@ func (s *aztestsSuite) TestContainerSetPermissionsPublicAccessContainer(c *chk.C

func (s *aztestsSuite) TestContainerSetPermissionsACLSinglePolicy(c *chk.C) {
bsu := getBSU()
credentials := azblob.NewSharedKeyCredential(os.Getenv("ACCOUNT_NAME"), os.Getenv("ACCOUNT_KEY"))
credential, _ := azblob.NewSharedKeyCredential(os.Getenv("ACCOUNT_NAME"), os.Getenv("ACCOUNT_KEY"))
Copy link

Choose a reason for hiding this comment

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

should this check to ensure there's no error returned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I can check the error and panic. The test would have failed anyway if the credential was invalid, but I see that a panic here makes debugging easier.

2018-03-28/azblob/zt_test.go Outdated
@@ -561,7 +561,7 @@ func (s *aztestsSuite) TestCreateBlobURLWithSnapshotAndSAS(c *chk.C) {
blobURL, blobName := getBlockBlobURL(c, containerURL)

currentTime := time.Now().UTC()
credential := azblob.NewSharedKeyCredential(os.Getenv("ACCOUNT_NAME"), os.Getenv("ACCOUNT_KEY"))
credential, _ := azblob.NewSharedKeyCredential(os.Getenv("ACCOUNT_NAME"), os.Getenv("ACCOUNT_KEY"))
Copy link

Choose a reason for hiding this comment

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

should this check to ensure there's no error returned?

2018-03-28/azblob/zt_examples_test.go Outdated
@@ -1151,7 +1156,7 @@ func ExampleListBlobsHierarchy() {
accountName, accountKey := accountInfo()

// Use your Storage account's name and key to create a credential object; this is used to access your account.
credential := azblob.NewSharedKeyCredential(accountName, accountKey)
credential, _ := azblob.NewSharedKeyCredential(accountName, accountKey)
Copy link

Choose a reason for hiding this comment

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

given this is an example (and thus a recommended way of using this library) - shouldn't this be checking that the error object returned is nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok!

2018-03-28/azblob/zt_examples_test.go Outdated
@@ -1082,7 +1087,7 @@ func ExampleLeaseContainer() {
accountName, accountKey := accountInfo()

// Use your Storage account's name and key to create a credential object; this is used to access your account.
credential := azblob.NewSharedKeyCredential(accountName, accountKey)
credential, _ := azblob.NewSharedKeyCredential(accountName, accountKey)
Copy link

Choose a reason for hiding this comment

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

given this is an example (and thus a recommended way of using this library) - shouldn't this be checking that the error object returned is nil?

2018-03-28/azblob/zt_examples_test.go Outdated
@@ -1052,7 +1056,8 @@ func ExampleUploadStreamToBlockBlob() {

// Create a BlockBlobURL object to a blob in the container (we assume the container already exists).
u, _ := url.Parse(fmt.Sprintf("https://%s.blob.core.windows.net/mycontainer/BigBlockBlob.bin", accountName))
blockBlobURL := azblob.NewBlockBlobURL(*u, azblob.NewPipeline(azblob.NewSharedKeyCredential(accountName, accountKey), azblob.PipelineOptions{}))
credential, _ := azblob.NewSharedKeyCredential(accountName, accountKey)
Copy link

Choose a reason for hiding this comment

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

given this is an example (and thus a recommended way of using this library) - shouldn't this be checking that the error object returned is nil?

2018-03-28/azblob/zt_examples_test.go Outdated
@@ -278,7 +278,7 @@ func ExampleAccountSASSignatureValues() {
accountName, accountKey := accountInfo()

// Use your Storage account's name and key to create a credential object; this is required to sign a SAS.
credential := azblob.NewSharedKeyCredential(accountName, accountKey)
credential, _ := azblob.NewSharedKeyCredential(accountName, accountKey)
Copy link

Choose a reason for hiding this comment

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

given this is an example (and thus a recommended way of working with this library) - shouldn't this error be handled?

2018-03-28/azblob/zt_examples_test.go Outdated
@@ -32,7 +32,7 @@ func Example() {
accountName, accountKey := accountInfo()

// Use your Storage account's name and key to create a credential object; this is used to access your account.
credential := azblob.NewSharedKeyCredential(accountName, accountKey)
credential, _ := azblob.NewSharedKeyCredential(accountName, accountKey)
Copy link

Choose a reason for hiding this comment

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

given this is an example (and thus a recommended way of working with this library) - shouldn't this error be handled?

2018-03-28/azblob/zt_test.go Outdated
@@ -1784,11 +1784,11 @@ func (s *aztestsSuite) TestBlobStartCopyUsingSASSrc(c *chk.C) {
blobURL, blobName := createNewBlockBlob(c, containerURL)

// Create sas values for the source blob
credentials := azblob.NewSharedKeyCredential(os.Getenv("ACCOUNT_NAME"), os.Getenv("ACCOUNT_KEY"))
credential, _ := azblob.NewSharedKeyCredential(os.Getenv("ACCOUNT_NAME"), os.Getenv("ACCOUNT_KEY"))
Copy link

Choose a reason for hiding this comment

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

should this check to ensure there's no error returned?

2018-03-28/azblob/zt_test.go Outdated
@@ -1830,8 +1830,8 @@ func (s *aztestsSuite) TestBlobStartCopyUsingSASDest(c *chk.C) {
// Generate SAS on the source
serviceSASValues := azblob.BlobSASSignatureValues{ExpiryTime: time.Now().Add(time.Hour).UTC(),
Permissions: azblob.BlobSASPermissions{Read: true, Write: true, Create: true}.String(), ContainerName: containerName, BlobName: blobName}
credentials := azblob.NewSharedKeyCredential(os.Getenv("ACCOUNT_NAME"), os.Getenv("ACCOUNT_KEY"))
queryParams := serviceSASValues.NewSASQueryParameters(credentials)
credential, _ := azblob.NewSharedKeyCredential(os.Getenv("ACCOUNT_NAME"), os.Getenv("ACCOUNT_KEY"))
Copy link

Choose a reason for hiding this comment

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

should this check to ensure there's no error returned?

2018-03-28/azblob/zt_test.go Outdated
@@ -1845,11 +1845,11 @@ func (s *aztestsSuite) TestBlobStartCopyUsingSASDest(c *chk.C) {
copyBlobURL, copyBlobName := getBlockBlobURL(c, copyContainerURL)

// Generate Sas for the destination
credentials = azblob.NewSharedKeyCredential(os.Getenv("SECONDARY_ACCOUNT_NAME"), os.Getenv("SECONDARY_ACCOUNT_KEY"))
credential, _ = azblob.NewSharedKeyCredential(os.Getenv("SECONDARY_ACCOUNT_NAME"), os.Getenv("SECONDARY_ACCOUNT_KEY"))
Copy link

Choose a reason for hiding this comment

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

should this check to ensure there's no error returned?

@tombuildsstuff tombuildsstuff mentioned this pull request Aug 17, 2018
Closed
@zezha-msft zezha-msft force-pushed the new-shared-key-return-error branch from 9805d4f to 0802cea Compare August 18, 2018 06:11
@zezha-msft zezha-msft force-pushed the new-shared-key-return-error branch from 0802cea to d795ea3 Compare August 18, 2018 06:21
@JeffreyRichter JeffreyRichter merged commit b13c8df into dev Aug 18, 2018
@zezha-msft zezha-msft deleted the new-shared-key-return-error branch August 28, 2018 01:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tombuildsstuff tombuildsstuff tombuildsstuff requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zezha-msft @tombuildsstuff @JeffreyRichter