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.

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

Support realm in the OAuth1 header #2887

Closed
wants to merge 5 commits into from
Closed

Support realm in the OAuth1 header #2887

wants to merge 5 commits into from

Conversation

semenodm
Copy link

@semenodm semenodm commented Sep 26, 2019

Support realm as part of Auth header

Support realm as part of Auth header
@semenodm semenodm changed the title move headers into their own data classes Support realm in the OAuth1 header Sep 26, 2019
Copy link
Member

@rossabaker rossabaker left a comment

Thanks! I haven't worked with oauth1 much, but I'm in favor of making the experience better. Comments inline.

import cats.Show
import cats.kernel.Order
import cats.implicits._
sealed trait Header {
Copy link
Member

@rossabaker rossabaker Sep 27, 2019

Choose a reason for hiding this comment

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

Is there any reason not to model these as standard org.http4s.Header? We can still have the specific types, but these won't work quite the same as other headers in http4s.

Copy link
Member

@rossabaker rossabaker Sep 27, 2019

Choose a reason for hiding this comment

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

I see it now in takeSigHeaders. Yeah, maybe make this a marker trait, but still make these standard http4s headers?

Copy link
Author

@semenodm semenodm Sep 30, 2019

Choose a reason for hiding this comment

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

I'm not sure these are standard headers, these are part of.
http://docs.akana.com/cm/api_oauth/aaref/Ref_OAuth_AuthorizationHeader_10a.htm

Authorization: OAuth realm="Example",
  oauth_consumer_key="0685bd9184jfhq22",
  oauth_token="ad180jjd733klru7",
  oauth_signature_method="HMAC-SHA1",
  oauth_signature="wOJIO9A2W5mFwDgiDvZbTSMK%2FPY%3D",
  oauth_timestamp="137131200",
  oauth_nonce="4572616e48616d6d65724c61686176",
  oauth_version="1.0"

So i'm not sure if we want them to be Http header.

Copy link
Member

@rossabaker rossabaker Oct 5, 2019

Choose a reason for hiding this comment

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

You're right. I got fooled by the name. These are called "parameters" in most headers, and the spec refers to them as "protocol parameters". Maybe I'd call the trait ProtocolParameter to avoid ambiguity with Header.

import org.http4s.client.oauth1.Header.{Callback, Verifier}
import org.http4s.client.oauth1.{Header => OAuthHeader}

case class OAuthConfig(
Copy link
Member

@rossabaker rossabaker Sep 27, 2019

Choose a reason for hiding this comment

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

Are we sure more config won't be necessary? We've started to move away from case classes for configuration, so we can add more parameters later without breaking binary compatibility. This is not an issue if this is truly a closed set, but it makes me a bit nervous.

Copy link
Author

@semenodm semenodm Sep 30, 2019

Choose a reason for hiding this comment

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

my rule of thumb is usually if number of params > 3 i use case classes

Copy link
Author

@semenodm semenodm Sep 30, 2019

Choose a reason for hiding this comment

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

i never thought about your point

Copy link
Member

@rossabaker rossabaker Oct 5, 2019

Choose a reason for hiding this comment

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

It's tedious to write out binary compatible case classes by hand, but it does improve the compatibility story. If we think this will always be enough config, we can probably leave this alone.

An interesting project has emerged to help with this, but should probably be discussed separately: data-class.

rossabaker
rossabaker previously approved these changes Oct 5, 2019
Copy link
Member

@rossabaker rossabaker left a comment

Apologies for the delayed reply. I've had a tough week at home, but should be increasingly responsive again.

override val headerName: String = "oauth_verifier"
}

implicit val sortHeaders: Order[Header] = new Order[Header] {
Copy link
Member

@rossabaker rossabaker Oct 5, 2019

Choose a reason for hiding this comment

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

Nit we've started following the cats convention of naming with the package, type name, and typeclass name, to avoid any accidental shadowing. So, http4sClientOauth1SortForHeaders.

implicit val sortHeaders: Order[Header] = new Order[Header] {
override def compare(x: Header, y: Header): Int = x.headerName.compareTo(y.headerName)
}
val tupleShow: Show[(String, String)] = new Show[(String, String)] {
Copy link
Member

@rossabaker rossabaker Oct 5, 2019

Choose a reason for hiding this comment

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

I'm a little surprised cats doesn't have an inductive instance for this one. You could make it private so we can refactor it without a breaking change. This shouldn't be http4s' responsibility to expose to another project.

@rossabaker rossabaker dismissed their stale review Oct 5, 2019

Didn't mean to approve. Still discussing. But it's close!

@semenodm
Copy link
Author

@semenodm semenodm commented Oct 12, 2019

@rossabaker please take a look when you have time

Copy link
Member

@rossabaker rossabaker left a comment

Apologies for falling behind. This looks good, save for the two comments below. I'd like to get this released without further delay, so I'm cherry-picking back to series/0.20, and I'll append a commit with my proposed changes.

}

case class Timestamp(
override val headerValue: String = (System.currentTimeMillis() / 1000).toString)
Copy link
Member

@rossabaker rossabaker Nov 27, 2019

Choose a reason for hiding this comment

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

Should pass in a clock and create in an effect.

override val headerName: String = "oauth_timestamp"
}

case class Nonce(override val headerValue: String = System.nanoTime.toString)
Copy link
Member

@rossabaker rossabaker Nov 27, 2019

Choose a reason for hiding this comment

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

Should pass in a clock and create in an effect.

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Nov 27, 2019

Cherry-picked to #2992. Proposed fixes in 579f7c7.

@rossabaker rossabaker closed this Nov 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants