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

Challenge shouldn't require realm #7308

Open
kapunga opened this issue Nov 2, 2023 · 3 comments
Open

Challenge shouldn't require realm #7308

kapunga opened this issue Nov 2, 2023 · 3 comments

Comments

@kapunga
Copy link

kapunga commented Nov 2, 2023

org.http4s.Challenge requires the realm parameter, however the WWW-Authenticate Directive doesn't appear to require it.

@kapunga kapunga changed the title Challenge shouldn't require _realm_ Challenge shouldn't require realm Nov 2, 2023
@kapunga
Copy link
Author

kapunga commented Nov 2, 2023

I'm going to open a PR to make realm optional.

@kapunga
Copy link
Author

kapunga commented Nov 2, 2023

Okay, so digging in a little further, it seems a bit more complicated. realm is required for the Basic challenge types, but not others. Additionally there are other parameters that are required on some, but not on others. Additionally the token68 token that can appear by itself on some challenges and it does not take a value, and therefore doesn't fit into a map. Not to mention, Challenge is very stringly typed.

I think the proper way to fix this would be to turn Challenge into a sealed trait with the different challenge schemes and parameters with an apply(scheme: String, realm: String, params: Map[String, String] = Map.empty) method on the companion object for backwards compatibility.

I'd love to hear other opinions.

@armanbilge
Copy link
Member

turn Challenge into a sealed trait with the different challenge schemes

This seems like a good direction to go in.

for backwards compatibility.

Just changing Challenge from a case class to a sealed trait is already breaking binary compatibility. So these changes would have to target the main branch and I don't think it's worth working too hard to preserve other forms of compatibility.

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

No branches or pull requests

2 participants