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

Implement UserInfo for URIs #2671

Merged
merged 3 commits into from Jul 6, 2019
Merged

Implement UserInfo for URIs #2671

merged 3 commits into from Jul 6, 2019

Conversation

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Jun 25, 2019

Model UserInfo as a case class, with instances and principled encoding and decoding.

Copy link
Member Author

@rossabaker rossabaker left a comment

Since this is the beginning of URI reform, self-reviewed a bunch of things that could set precedent.

Another one: since this supports supports an apply method, is it worth a userInfo string context? Or should we reserve those for parsing literals only for domains more restrictive than String?

Loading

*
* @see https://www.ietf.org/rfc/rfc3986.txt#section-3.21.
*/
final case class UserInfo private (username: String, password: Option[String])
Copy link
Member Author

@rossabaker rossabaker Jun 25, 2019

Choose a reason for hiding this comment

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

There are going to be more of these. Should they continue to be nested in Uri or move to top level?

Loading

fromStringWithCharset(s, StandardCharsets.UTF_8)

/** Parses a userInfo from a string percent-encoded in a specific charset. */
def fromStringWithCharset(s: String, cs: NioCharset): ParseResult[UserInfo] =
Copy link
Member Author

@rossabaker rossabaker Jun 25, 2019

Choose a reason for hiding this comment

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

i don't know how we'd ever use this in practice without UTF-8, or why anyone would want to, but the spec allows it. If we allow this, we should probably provide rendering to specific charsets, but that would be a partial operation.

Loading

(
username: String,
password: Option[String]) => UserInfo(decode(username), password.map(decode)))
}
Copy link
Member Author

@rossabaker rossabaker Jun 25, 2019

Choose a reason for hiding this comment

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

I really don't like what scalafmt does to our parsers. Maybe we should start exempting them.

Loading

def charset = cs
}.parse

private[http4s] trait Parser { self: Rfc3986Parser =>
Copy link
Member Author

@rossabaker rossabaker Jun 25, 2019

Choose a reason for hiding this comment

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

Following pattern of putting parsers in companion object, because they may call a private constructor after validation. It does lead to some quirky self-typing.

Loading

case Authority(Some(u), h, None) => u + "@" + renderHost(h)
case Authority(Some(u), h, Some(p)) => u + "@" + renderHost(h) + ":" + p
case Authority(Some(u), h, None) => s"${renderUserInfo(u)}@${renderHost(h)}"
case Authority(Some(u), h, Some(p)) => s"${renderUserInfo(u)}@${renderHost(h)}:${p}"
Copy link
Member Author

@rossabaker rossabaker Jun 25, 2019

Choose a reason for hiding this comment

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

I wonder whether anybody is using this feature. I'm dragging it along for now.

Loading

Arbitrary(
for {
username <- getArbitrary[String]
password <- getArbitrary[Option[String]]
Copy link
Member Author

@rossabaker rossabaker Jun 25, 2019

Choose a reason for hiding this comment

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

Maybe worth altering the mix since passwords are deprecated.

Loading

@ritschwumm
Copy link

@ritschwumm ritschwumm commented Jun 25, 2019

is this still useful nowadays? iirc modern browsers are removing or have removed support for this to a varying degree for security reasons. it looks like this removal is becoming standardized, too - see https://tools.ietf.org/html/rfc3986#section-7.5 . here's a (slightly dated) blog post about the state in various browsers: https://medium.com/@lmakarov/say-goodbye-to-urls-with-embedded-credentials-b051f6c7b6a3

Loading

@rossabaker
Copy link
Member Author

@rossabaker rossabaker commented Jun 25, 2019

I did this one early because I'm going through the RFC top-to-bottom. It raises some interesting questions that will pertain to other URI components, but it's a poor choice in its own relevance. I would strongly argue against its use in any API I control, but I've also seen a lot of our users with APIs they don't control, and it remains part of the spec. I would argue it belongs for compliance's sake, but I hope nobody uses it.

Subsequent PRs with paths and queries will be far more interesting and relevant.

Loading

@rossabaker rossabaker merged commit e4d9873 into http4s:master Jul 6, 2019
1 check passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants