KTOR-7644 Make re-auth status codes configurable

[wkornewald]

Some services use 403 instead of 401. Changing them might be impossible. With this change Ktor can flexibly work with any broken service.

KTOR-7644 Auth: Make re-auth/refresh status codes configurable

[osipxd]

Thank you for the pull request! Seems useful to me
Let's discuss an alternative approach before we merge this.

[osipxd]

Speaking of broken services, I've seen some APIs returning 200 OK with actual status code in a body. We could provide a lambda instead of a list of statuses:

var isUnauthorized: (HttpResponse) -> Boolean = { it.status == HttpStatusCode.Unauthorized }

@e5l, @bjhham, @marychatte, what do you think?

[wkornewald]

This sounds much better than the list. I've pushed that change.

[marychatte]

@osipxd
I like the idea with lambda!

returning 200 OK with actual status code in a body

Do you mean it could be used like this?
isUnauthorized = { it.bodyAsText() == "401" }
If yes, then isUnauthorized function should be suspend

[osipxd]

LGTM. Thank you!
@bjhham, @marychatte, could you also take a look?

[marychatte]

Thanks for the PR! Can we also please add tests for it?

[wkornewald]

Thanks for the PR! Can we also please add tests for it?

Sure, added one and switched to a suspend lambda.

[osipxd]

Let's refine the API a bit to make it more consistent to the existing ones.

Suggested change

	public var isUnauthorized: suspend (HttpResponse) -> Boolean = { it.status == HttpStatusCode.Unauthorized }
	internal var isUnauthorized: suspend (HttpResponse) -> Boolean = { it.status == HttpStatusCode.Unauthorized }
	/** ... */
	public fun isUnauthorized(block: suspend (HttpResponse) -> Boolean) {
	isUnauthorized = block
	}

Though, I'm not sure about naming. Probably, isUnauthorizedResponse would be better. @marychatte, what do you think?

[wkornewald]

But then you couldn't build on top of existing values:

val original = isUnauthorized
isUnauthorized = { original(it) || ... }

[osipxd]

We could make only set() private then, giving the possibility to get the configured isAuthorized:

public var isUnauthorized: suspend (HttpResponse) -> Boolean = { it.status == HttpStatusCode.Unauthorized }
    private set()

The point is to specify this field without an assignment operator.

[wkornewald]

Why is a getter + a differently named setter function better than a single var unifying those two?

[marychatte]

@osipxd isUnauthorizedResponse sounds valid to me, it adds more clarity 👍

[wkornewald]

Pushed the renamed version.

[osipxd]

Why is a getter + a differently named setter function better than a single var unifying those two?

The main concern for me is the inconsistency with the existing APIs. Let's break my proposal into two points:

1. Use a function with a lambda parameter instead of a setter
   The reason is that syntax doSomething { ... } is widespread across all plugins’ configurations, while fields are usually used for primitive types or enums. Though such functions usually contain a verb in the name, so checkResponseUnauthorized or checkResponseIsUnautorized fit better in this case.
2. Hide the field from public API
   Making the field internal makes it simpler for us to change implementation in the future without breaking changes. To be honest, I can't come up with any use case when I want to build check on top of an existing value. The default value is trivial, so I'd say it is clearer to copy it instead of reuse not to be affected by possible further changes. So it feels safe to make this field internal.

The combination of internal field + function lets us introduce shortcuts for common use cases. For example, we could introduce such an API in the future:

fun reAuthorizeOnStatuses(vararg statusCodes: HttpStatusCode) {
    isUnauthorizedResponse = { it.status in statusCodes }
}

[wkornewald]

Please take a look at the latest commit. I've changed it. Though, this solution seems less direct and understandable, especially when there's a public getter (which probably is missing in many of the other APIs?). The reAuthorizeOnStatuses extra function can also still be provided with a var, too, so I don't see that argument.

[osipxd]

Thank you for keeping this branch up-to-date!

Could you allow editing by maintainers? I want to mark isUnauthorizedResponse as @InternalAPI.

[wkornewald]

Oh right I forgot the InternalAPI markers. You should have access now.

[wkornewald]

Why is this an internal API? My intention was to allow access to the current value, so you can extend an existing rule.

[osipxd]

We want to preserve the right to introduce breaking changes to this field. Keeping it public for those who’re ready to further breaking changes.