octokit / source-generator

6 stars 3 forks source link

feat: Adds a prototype for token based auth #34

Closed nickfloyd closed 8 months ago

nickfloyd commented 8 months ago

What

This is a quick take on implementing a token auth provider. Other providers will need to be implemented. It does 3 fundamental things to help the generated SDK call the API

  1. Creates a provider to store auth info in as requests are being made
  2. Adds a version header - should be abstracted or at least configured outside of the provider
  3. Adds the auth header for each request thought the instantiation of the HttpClientRequestAdapter.

Why

The provider pattern a standard approach to providing extended functionality for things like this, it is also what is recommended by our Kiota friends.

Implementation details

cc: @baywet | if you have time look just for a gut check on if we're headed the right direction.

baywet commented 8 months ago

Didn't get to this on time, but I'll provide my feedback anyway. I think this is a great start, the only thing is it mixes concerns.

http headers

It'd be better if the user agent/API version work was in its own middleware handler like the retry handler for instance, you could then build a "GitHub client factory", which would rely on the kiota client factory to add this middleware. And derive from the request adapter to call this factory instead of the default one. This would allow for reusability of the version/user agent behavior, regardless of the authentication method.

other authentication schemes

Looking at the TODO comments:

All in all I'm not sure what the goal of this repository is, if it's to demo the feasibility, I think the code is good enough as is. If it's to provide production level libraries, I think you should seriously consider the http headers recommendations.

nickfloyd commented 8 months ago

@baywet This is so so good! Thanks for the heads up on all of this.... it seems like we have several spring boards and approaches that I need to try.

RE: The header work, I totally agree, I was thinking about it last night and knew that it needs to be broken out. I'm going to go round 2 on this and see what I can hammer out. Thank you for the orientation on this!