Allow configuration of URIs in JWT helpers

[andtie]

I am facing the problem that I need to specify another URI to get the JWKS for the Microsoft issuer.
It is currently hardcoded to "https://login.microsoftonline.com/common/discovery/keys", but I need the "v2.0"-version at "https://login.microsoftonline.com/common/discovery/v2.0/keys".

The only way that I found to work around this issue is to copy and rename all of the JWT-Microsoft helpers (in JWT+Micorsoft.swift) and change the URI in the copy.
I would prefer it, if I could only change the URI without having to need to copy the whole other logic.

Therefore, I propose to add another public property (e.g. named jwksEndpoint) similar to applicationIdentifier that can be changed via app.jwt.microsoft.jwksEndpoint.

The same could be done for the Apple and Google helpers.

[0xTim]

I think this could be a useful enhancement. If you're happy to do the PR feel free otherwise we can add it to the backlog

[andtie]

Thanks for your reply. I am happy to do the PR.

[andtie]

Of course, the problem is more complicated than I first thought 😉.
The JWT- and Microsoft-structs are recreated on every access and all data is stored in the respective Storage-class. However, the URI cannot be stored in the storage, because it is already needed in the initializer of the storage itself.

I see several possibilities to overcome this problem:

1. Store the URI in a static variable on the Mircosoft-struct such that it can be set like this: Application.JWT.Microsoft.jwksEndpoint = "...".
   This is simple, but afaik setting of the static variable is not thread-safe. In practice this probably isn't a problem, because setting the URI makes only sense once during the configuration of the app anyway.
2. We could add a second storage / configuration class, just to store the URI. This should be clean, but very verbose and adds a lot of duplicated code.
3. We could add a new additional initializer with the URI as a parameter to the extension. E.g.:

extension Application.JWT {
     public func microsoft(jwksEndpoint: URI) -> Microsoft { ... }
     ...
}

This way, we do not have to store the URI. But we would also have to parametrize the microsoft-extension on Request.JWT such that it passes the parameter when accessing the signers:

extension Request.JWT {
     public func microsoft(jwksEndpoint: URI) -> Microsoft { ... }
     ...
}

For (1) and (2) there is the additional constraint that a user must configure the URI before the applicationIdentifier. Otherwise it would have no effect. Perhaps we could log a warning if this happens, similar to how it is done in HTTPServer.Configuration.

If we would go down the road of (3) and parametrize the extensions on Request.JWT, one could probably go further and unify the Apple, Google and Microsoft-extensions as well. E.g., by passing a complete configuration struct.

Of course there are also other possibilities, such as requiring an explicit initialization of the storage before the first request. But this would then probably break API compatibility.

I would love to hear your thoughts on this.

[andtie]

On further reflection, I think the current implementation of setting the applicationIdentifier is also not thread-safe.
I turned on Thread Sanitizer and wrote a small test to verify this:

    func testDataRace() throws {
        let app = Application(.testing)
        defer { app.shutdown() }
        let exp = expectation(description: "data race")

        (0...10).forEach { _ in
            DispatchQueue.global().async { app.jwt.microsoft.applicationIdentifier = "" }
        }

        DispatchQueue.global().async { exp.fulfill() }
        waitForExpectations(timeout: 1, handler: nil)
    }

This produces the expected runtime-issue: runtime: Threading Issues: Data race in (extension in JWT):Vapor.Application.JWT.Apple.applicationIdentifier.setter : Swift.Optional<Swift.String> at 0x7b1000039300

However, as mentioned above, in practice this probably isn't a problem, because setting the applicationIdentifier (and Endpoint-URI) more than once wouldn't make sense anyway.
For the case of the jwksEndpoint however, it may be a little bit more dangerous because changing the URI would necessarily invalidate the EndpointCache.

This leaves us with the options to either (a) expose an jwksEndpoint similar to the applicationIdentifier and ignore potential threading problems, (b) wrap everything in locks or (c) do a bigger refactor of the whole thing (there is a lot of duplicated code anyway) with the current problems in mind.

I would be happy to help with the implementation if you have a preference.

[0xTim]

If you set the app identifier to use Applications storage, you can wrap it in a lock, similar to how the HTTP Client works https://github.com/vapor/vapor/blob/master/Sources/Vapor/HTTP/Client/Application%2BHTTP%2BClient.swift#L19-L38

[andtie]

I have created a pull request to solve this issue. It uses a separate Lock to reset the EndpointCache when the URI is modified.