oxide-auth icon indicating copy to clipboard operation
oxide-auth copied to clipboard

Explicitly declare which values can be trusted

Open edward-shen opened this issue 5 months ago • 0 comments

Project Improvement

Hey! I've been going through implementing my own (async) implementation with this, and I've been collecting thoughts that I think are actionable to better the crate.

Notably, I'm very thankful for the primitives: once you understand what most pieces do it makes it a lot easier to implement things correctly. I've certainly

However, there's a bunch of things that I felt I was lost on, and had to read source code (and play around with what was being provided) to various traits:

By far the biggest concern is the documentation on the individual traits, specifically on Registrar, Authorizer, and Issuer. The documentation on all these traits currently do not inform a trait implementation whether or not an input can be trusted or not. For example:

  • [ ] In Registrar::negotiate, is the Option<Scope> raw user input? Can we directly trust BoundClient to have a server validated redirect uri, or must we validate it before generating a PreGrant?
  • [ ] In Issuer::issue, can we trust the Grant provided to us? Where is the Grant.until value being populated from?
  • [ ] In Issuer::refresh, can we trust the Grant provided to us?

This probably expands to all traits, but I think these are the most important ones when it comes to getting up and running in a secure manner.

There's also a couple clarifying questions I can't seem to figure out either that I think documentation could be extremely helpful on:

  • [ ] In Issuer::recover_refresh for an Issuer that supports refresh tokens, are we to return a Grant with an until value of the refresh token expiration or the session token expiration?
  • [ ] In the Issuer's recover flows, do we need to populate all fields of the Grant? As far as I can tell I was able to populate dummy values in the redirect_uri and extensions fields without any issues, though I suspect that's only because I currently am only doing the authorization grant flow.
  • [ ] As far as I can tell, a "solicitor" isn't part of the OAuth spec. You can figure it out as the "render-the-oauth-grant/deny-page", but it took a while for me to digest everything.

Finally, I think it would be really, really helpful embedding some of the expectations of the Flow structs. It took me quite a bit of time to figure out that you need to use HTTP Basic authorization to exchange an authorization code into bearer tokens. Suggestions on how long those bearer tokens should last would also be appreciated

Other context

I'm very appreciative of the work done already. It's definitely helped clear things up with OAuth on the server implementation side.

Tracking pull request

  • [ ] A pull request does not yet exist

edward-shen avatar Feb 14 '24 18:02 edward-shen