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

Fixing AzureCLI / CloudShell authentication #169

Merged
merged 5 commits into from
Sep 13, 2017
Merged

Fixing AzureCLI / CloudShell authentication #169

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a04d94c
Fixing AzureCLI / CloudShell authentication
tombuildsstuff Sep 8, 2017
575eb82
Changes requested in review
tombuildsstuff Sep 12, 2017
ede361e
Refactoring the CLI stuff into it's own package
tombuildsstuff Sep 12, 2017
a32ff8d
Refactoring a little
marstr Sep 13, 2017
954fd09
Even more refactoring.
marstr Sep 13, 2017
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
44 changes: 38 additions & 6 deletions autorest/adal/cli.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,19 @@
package adal
Copy link
Member

Choose a reason for hiding this comment

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

I thought about this a little this morning, and think that this code would be better off in a cli package. Particularly, I think it makes sense as a child of the azure package. In general, I'd like to have as few Azure specific references in the adal package as possible, that applies doubly to CLI specific code :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated :)


import (
"github.com/mitchellh/go-homedir"
"fmt"
"log"
"strconv"
"time"

"github.com/mitchellh/go-homedir"
)

// AzureCLIToken represents an AccessToken from the Azure CLI
type AzureCLIToken struct {
AccessToken string `json:"accessToken"`
Authority string `json:"_authority"`
ClientID string `json:"_clientId"`
ExpiresIn int `json:"expiresIn"`
ExpiresOn string `json:"expiresOn"`
IdentityProvider string `json:"identityProvider"`
IsMRRT bool `json:"isMRRT"`
Expand Down Expand Up @@ -47,13 +50,42 @@ func AzureCLIProfilePath() (string, error) {
}

// ToToken converts an AzureCLIToken to a Token
func (t AzureCLIToken) ToToken() Token {
return Token{
func (t AzureCLIToken) ToToken() (*Token, error) {
tokenExpirationDate, err := ParseAzureCLIExpirationDate(t.ExpiresOn)
if err != nil {
return nil, fmt.Errorf("Error parsing Token Expiration Date %q: %+v", t.ExpiresOn, err)
}

difference := tokenExpirationDate.Sub(expirationBase)
seconds := difference.Seconds()

token := Token{
AccessToken: t.AccessToken,
Type: t.TokenType,
ExpiresIn: strconv.Itoa(t.ExpiresIn),
ExpiresOn: t.ExpiresOn,
ExpiresIn: "3600",
ExpiresOn: strconv.Itoa(int(seconds)),
Copy link
Member

Choose a reason for hiding this comment

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

[nit] I think this would be more readable as difference.Seconds() and skipping the declaration of seconds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated :)

RefreshToken: t.RefreshToken,
Resource: t.Resource,
}
return &token, nil
}

// ParseAzureCLIExpirationDate parses either a Azure CLI or CloudShell date into a time object
func ParseAzureCLIExpirationDate(input string) (*time.Time, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: why do you return pointer to time.
time package uses value semantics by default, returning pointer causes one more allocation, moves object from stack to heap and creates work for garbage collector. as it is used only within a calling method it should return value.
this way it would stay on stack and get disposed when method finishes. no additional work for GC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ve opted for this approach since we’re returning nil for an invalid/unparseable date; given returning an empty object in addition to an error in the failure case feels misleading as a consumer of the API

Copy link
Member

@marstr marstr Sep 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It seems like you use that pattern pretty religiously, @tombuildsstuff. I'm not really convinced either way, other than sorta general intuitiveness, can you build a case for why it's better to return a pointer? Personally, when I'm writing Go, I just treat retvals that accompany an error as garbage, it doesn't matter if the value is the default or memory with random bits. As you can imagine, in my workflow, changing the return type to a pointer has almost no consequence. What scenario am I missing where it's totally important? :)

edit: this isn't important enough for me to hold up a PR. Don't feel like this comment is holding you hostage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not particularly - I've only opted for this approach to force the consumer to handle the error case, rather than accidental comparing the values returned by the default object (it's more habit than anything else) - but totally happy to switch this out :)

log.Printf("[DEBUG] Token Date: %s", input)
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 remove this line before we commit this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated :)


// CloudShell (and potentially the Azure CLI in future)
expirationDate, cloudShellErr := time.Parse(time.RFC3339, input)
if cloudShellErr != nil {
// Azure CLI (Python) e.g. 2017-08-31 19:48:57.998857 (plus the local timezone)
cliFormat := "2006-01-02 15:04:05.999999"
expirationDate, cliErr := time.ParseInLocation(cliFormat, input, time.Local)
if cliErr == nil {
return &expirationDate, nil
}

return nil, fmt.Errorf("Error parsing expiration date %q.\n\nCloudShell Error: \n%+v\n\nCLI Error:\n%+v", input, cloudShellErr, cliErr)
}

return &expirationDate, nil
}