Exposing callable parameters for DigestAuth.challenge

[blast-hardcheese]

NonceKeeper is package-private, so there's no way to construct one,
meaning there's no way to call challenge.

Mirror the same parameters from apply, and maintain the previous
challenge method for bincompat.

I'm happy to make changes if desired.

Thank you

[armanbilge]

Thanks for digging into this! Since you've done the work to preserve bincompat (appreciated!) would you be able to retarget this at a stable branch? series/0.23 unless you are on series/0.22.

[blast-hardcheese]

@armanbilge It would be helpful to have this on 0.22, I'm happy to open a PR against 0.23 as well if that's how things work around here 🙂

[blast-hardcheese]

Oh, I see what you mean now.

We still do have a usability complication though, as someone not utilizing typeclasses (perhaps a newcomer to the cats ecosystem), attempting to call DigestAuth.apply[IO, A] directly would run into an issue with disambiguation:

scala> val x = DigestAuth.apply[IO, String]("foo", store, nonceCleanupInterval, nonceStaleTime, nonceBits)
                                                                           ^
       error: ambiguous reference to overloaded definition,
       both method apply in object DigestAuth of type (realm: String, store: AuthenticationStore[IO,String], nonceCleanupInterval: Duration, nonceStaleTime: Duration, nonceBits: Int)(implicit evidence$2: Concurrent[IO]): IO[AuthMiddleware[IO,String]]
       and  method apply in object DigestAuth of type (realm: String, store: AuthenticationStore[IO,String], nonceCleanupInterval: Duration, nonceStaleTime: Duration, nonceBits: Int)(implicit evidence$1: Sync[IO]): AuthMiddleware[IO,String]
       match argument types (String,AuthenticationStore[IO,String],Duration,Duration,Int)

not to mention the "overloaded methods define default parameters".

I'm still leaning towards applyF for usability reasons, but open to guidance

[armanbilge]

Oh, good point about IO and the default parameters issue. This is clearly a source-compatibility problem more than a bin-compatibility problem 🤔

I'm still leaning towards applyF for usability reasons, but open to guidance

👍 for sure, let's move forward with that for now.

[blast-hardcheese]

@armanbilge Alright. server / mimaReportBinaryIssues is clear, applyF uses F[_]: Concurrent to permit future Semaphore[F] usage, I think this is good to go.

[blast-hardcheese]

Oops. Was relying on server / Test / compile, got bit by incremental compilation fail.

[blast-hardcheese]

OK. Rebased, clean ; test passed, scalafmtAll'd, mimaReportBinaryIssues, and we're ready to go again.

[armanbilge]

Nice work getting it green!

Personally I'd feel a bit funny shipping this without a matching implementation, since without one it's hard to know for sure if we got the constraints right and/or missed anything else. E.g. the note I made below.

[armanbilge]

I think this should also become a Concurrent constraint.

[blast-hardcheese]

Personally I'd feel a bit funny shipping this without a matching implementation

So, given this, are we back at where we started, just exposing a new challenge method with callable parameters, pending the NonceKeeper rewrite, or are we blocked until that gets done?

[armanbilge]

We should let another maintainer weigh in.

series/0.22 is approaching EOL so maybe it's okay to just eat the caveats and go for the minimum-viable-fix. But I think we should see this fixed properly on series/0.23 up, at least.

FWIW I don't think it should be too challenging to fix. The only change really is gating the NonceKeeper with a semaphore instead of relying on a ConcurrentMap.

[blast-hardcheese]

To clarify, I actually agree with your objection. I started trying to rework NonceKeeper, since that needs to be done regardless. If we could get a callable version of challenge in 0.22 that'd be grand, but not if y'all are uncomfortable with it.

[blast-hardcheese]

Alright. Rolling this one back to pre applyF, as the future commits are in the other draft PR. Presumably the tests will all still pass and we can make some progress here.l Bincompat should still pass, and we don't have the confusion apply vs applyF to deal with, it's just the surgical exposure of a method that was previously uncallable.

[armanbilge]

I'm a bit confused about the relationship between this PR and #6138. #6138 is looking promising so I expect you can fix #6068 in that PR :)

[blast-hardcheese]

The intent of this PR is to just get the challenge method merged into 0.22 before it gets closed, and the NonceKeeper changes would move on to 0.23+

This might be too much to ask, but I figured since we've got direction on the future work, we could get this one moving.

[armanbilge]

Gotcha! I think we should be able to land #6138 in 0.22 :)

[blast-hardcheese]

Gotcha! I think we should be able to land #6138 in 0.22 :)

Got it, I misunderstood the direction of the conversation, no worries

[blast-hardcheese]

#6138 looks like it's in the home stretch, closing this one out.