Add login and token delete

[nickvelloff]

• We won't use DeleteToken now because revoking it without being able to invalidate it on the client is not a good idea. Nice to have it in case we use it later.
• Supports the /api/auth/login endpoint
• Supports the /api/auth/token endpoint
• Test coverage included 💯

[nickvelloff]

@paulyoung ready for review

[paulyoung]

I think maybe the Login and Logout directories / groups belong under Authorization and not Resources/User.

[paulyoung]

All of the files / methods / interface so far have been verbs; is LoginIdentity supposed to be a verb?

Maybe it's meant as LogInIdentity but I'm not sure what it means to "log in" an identity.

[paulyoung]

I don't think this makes any sense. Could you explain?

[nickvelloff]

For the log in we don't have a bearer token at that stage, as opposed to the add and remove identity. Should we make it optional, for init with an empty string? Not sure how to address that.

[paulyoung]

I think making an access token required for all of these operations has a nice simplicity to it. We can provide ways to make it easy to get a token, like getAccessToken.

This pull request is already evidence of ways in which obtaining an access token can change, but that isn't a concern for this interface so it shouldn't require any changes in order to keep up.

[paulyoung]

My high-level feedback at this point is:

• I think the "log in identity" piece is really just "login", and doesn't belong on APIClient since that's about making authenticated requests
• the "delete token" part does make sense as part of APIClient, but as far as a consumer is concerned it's just "logout"

I have some more detailed feedback but not sure it's worth sharing yet.

[paulyoung]

After another look it seems like maybe loginIdentity is along the lines of the existing getAccessToken method, since they both/all attempt to retrieve an AccessToken. Is that accurate?

Based on that I'm thinking there could be some overloads for that on OAuthClient that take the relevant parameters.

[nickvelloff]

I don't completely understand the feedback. "Login" is a Passport endpoint to trade a Provider token for a bearer token essentially. It does not belong in APIClient?
Let's chat today I'm a bit lost.

[nickvelloff]

Perhaps my naming is bad/confusing.

[nickvelloff]

OK I think what Im hearing is

1. GetAccessTokenRequest is a better name and project location for what I called LoginIdentity
2. LoginIdentity is what meets the requirements of the API, so migrating the functionality to GetAccessTokenRequest may make more sense.

We are not actually using OAuthClient as we did not want to duplicate the functionality of the OAuth2 lib which is needed to do the dance, pull the bearer token etc.

[paulyoung]

It seems like in addition to the /login/authorize/token endpoint which is already supported via OAuthClient, /api/auth/login is also just a mechanism for obtaining an AccessToken which can then be then passed to APIClient.

Is that correct?

[paulyoung]

Current trail of thought is that we could do away with OAuthClient and move the functionality it provides to static methods on APIClient.

The params currently passed to init would need to be passed to the methods themselves instead which is a little bit repetitive but could be nicer overall in only having to deal with one type for everything.

The new login endpoint could be supported through a static method too, with code being shared privately with addIdentity.

I'm happy to work on this if need be.

[nickvelloff]

I don't understand this:

Current trail of thought is that we could do away with OAuthClient and move the functionality it provides to static methods on OAuthClient.

OK it sounds like may you have some specific changes and I don't think I'll properly extract them from what you have described. I had hoped to add this with your current pattern and not have to bother you with it, but it seems not needing a token has upped the scope of this change a bit.

[paulyoung]

Sorry, that was a typo – I meant APIClient. Edited my comment above.

[nickvelloff]

Cool. Go ahead and knock this out as you see fit @paulyoung. It'll be faster as I don't completely understand the approach I think.

[nickvelloff]

Sadly this was obolessed by other PRs.