Expose login api

[andig]

Fix #22.

This is still too convoluted but it should give an idea how it could look:

• WithMFAHandler will set the device handler for MFA, if omitted MFA is not supported
• the WithCredentials option will trigger the login and must be the last option

I'm using it like this:

ctx := context.WithValue(context.Background(), oauth2.HTTPClient, request.NewHelper(log).Client)
client, err := tesla.NewClient(
    ctx,
    tesla.WithMFAHandler(codePrompt),
    tesla.WithCredentials(username, password),
)
if err != nil {
    log.FATAL.Fatalln(err)
}

token := client.Token()

Comments welcome.

/cc @uhthomas

[uhthomas]

I think we should keep these components separate. They do two very different things and the client actually depends on having a token from its creation.

[andig]

I wanted to be able to: reuse the context (and the contained http client, not yet implemented).

Internally, auth this is a separate component. From user perspective though it shouldn‘t matter if I create the client with token oder with u/p.

Happy to follow any other path though 👍🏻

[bogosj]

I'm not opposed to this but I have a few observations:

• WithCredentials needing to be the last optional argument seems brittle.
• The options this provides a user are interesting: users of the lib could accept user/pass/code from end users and offer to store the token generated from first login.

[andig]

WithCredentials needing to be the last optional argument seems brittle.

Fixed by moving the initialisation to the client creation.

[uhthomas]

I'm concerned that the options aren't really optional. It seems you must have both "options" for it to work, and in some circumstances may not be applicable at all. What if there's also a WithToken thrown in there? What happens?

[bogosj]

This mirrors https://pkg.go.dev/google.golang.org/api/option fairly well, aside from the complexity of the MFA login bit. What happens if you give Google APIs WithAPIKey, WithCredentials, and WithServiceAccountFile all in the same call?

// Without MFA
NewClient(ctx, WithCredentials(username, password))

// With MFA
NewClient(ctx, WithCredentials(username, password), WithMFAHandler(promptUI))
NewClient(ctx, WithCredentials(username, password, WithMFAHandler(promptUI)))

Is what is at debate now, correct? I'd rather have two ClientOptions than one ClientOption and one CredentialOption that needs to be passed.

[andig]

What if there's also a WithToken thrown in there? What happens?

I had made this an error: https://github.com/bogosj/tesla/pull/37/files#diff-4b667feae66c9d46b21b9ecc19e8958cf4472d162ce0a47ac3e8386af8bbd8cfR54

I'm concerned that the options aren't really optional. It seems you must have both "options" for it to work, and in some circumstances may not be applicable at all.

Setting an MFA device handler is optional. If none is set and it turns out user has MFA enabled it will result in an error: https://github.com/bogosj/tesla/pull/37/files#diff-6b347a9043969a3ccadca7c26c658d5630226316770afecb2d6c9ac8355a9fbaR86

I agree it doesn't feel 200% nice but I couldn't find a better approach.

[andig]

Is what is at debate now, correct?

It's an error currently which makes it very explicit.

[bogosj]

Is there additional review needed here? We don't have an established norm for how / when / whom does merging. I added the other three contributors as reviewers but didn't really think through whether all or some or one of the reviewers approving was the appropriate signal for "go ahead and merge" :)

@michaelharo do you have any opinions on the PR?

@uhthomas any additional thoughts?

[andig]

I‘d be happy with any solution, doesn‘t need to be this one

[uhthomas]

I'd like coherence check on the potential panic when not providing a device selector for an MFA account.

[uhthomas]

To be clear, this line.

[andig]

It‘s here in https://github.com/bogosj/tesla/pull/37/files#diff-6b347a9043969a3ccadca7c26c658d5630226316770afecb2d6c9ac8355a9fbaR68 so wont happen.

[uhthomas]

Perfect, thank you.

[andig]

Comments fixed. Feel free to squash and merge.

[andig]

I've merged, given the approvals above. We have not agreed on a process, should we follow that? How many approvals to we need?