Zaid-Ajaj / Fable.Remoting

Type-safe communication layer (RPC-style) for F# featuring Fable and .NET Apps
https://zaid-ajaj.github.io/Fable.Remoting/
MIT License
273 stars 55 forks source link

Question: Adding withAuthorizationHeaderBuilder #149

Closed Dzoukr closed 4 years ago

Dzoukr commented 4 years ago

Hello @Zaid-Ajaj!

Great article about Fable.Remoting from @JamesRandall (https://www.azurefromthetrenches.com/token-authentication-with-fsharp-safe-and-fable-remoting/) brought me to one idea. What about extend RemoteBuilderOptions to have something like:

type RemoteBuilderOptions = {
    //...all previous props are the same
    AuthorizationBuilder : (unit -> string) option
}

and use it like:

Remoting.withAuthorizationHeaderBuilder myFuncToProvideToken

and such function will be used in Proxy.fs when creating request? This one is to keep it as feature (no breaking change), but another way would be changing (breaking change here) RemoteBuilderOptions to:

type AuthorizationHeaderBuilder =
    | Static of token:string
    | Dynamic of (unit -> string)

type RemoteBuilderOptions = {
    CustomHeaders : (string * string) list
    BaseUrl  : string option
    Authorization : AuthorizationHeaderBuilder option
    RouteBuilder : (string -> string -> string)
}

The second one is a little bit cleaner, but still at least having the first option would be great and would fix many workarounds (fact is that Authorization header is almost never static in my cases).

What do you think friend? You know me - I feel no pain if you call it stupid and close the issue. Just need to know your opinion on this. 😄

Zaid-Ajaj commented 4 years ago

Hello Roman! This is actually a really good question. I have thought about this before and dicussed it at length with @mvsmal in Add resolve function for Authorization header

The problem with proposed solution is that it does not account for the many possible ways to acquire an access token: this resolve function (unit -> string) might have been

Then it gets more complicated as these functions themselves may be parameterized and require input like a refresh token or similar: (string -> Async<string>) which brings you right back to having to parameterize proxy creation.

Another underlying problem is this: how should the library handle failures in this resolve function? Implement a retry policy? throw exception? create a proxy with an empty authorization header?

Answers to these question should not be answered by the library but rather from application code that decides what to do in case of failures.

Because of the above issues, it makes it so much easier to parameterize the creation of the proxy itself rather then parameterize how the the Authorization header should be resolved. This way, it gives from freedom to the user with how to create proxy and from the perspective of the library, we don't have to assume how the users are acquiring the access tokens and what they should do in case of failures.

People were concerned about proxy creation being an expensive operation and wanted to create it just once. The fact is, proxy creation is very cheap since it is just creating an object literal based on record type metadata which is already available.

Dzoukr commented 4 years ago

Hello Zaid,

thanks for amazing response! I get your point about resolve function, but I think you care too much here. If someone put something failing in token builder function, it will just fail - it is responsibility of developer of such function to make it safe.

Regarding more ways ho to acquire token, I agree and you are right, there could more of them. Looking at Proxy.fs it seems that creation itself is async so making it unit -> Async<string> will be reasonable (and as you wrote - most common). You, as author of tens of amazing libraries, you know the best - you cannot always satisfy everyone.

Just to be clear: I don't want to persuade you no matter what. I understand you concern and as library author, it is you call indeed. I just want to pin out that adding one function (even imperfect for some less common cases) will resolve 99% of workarounds - cheap or not, creating whole proxy for each call seems heavy and kind of weird.

Zaid-Ajaj commented 4 years ago

Just to be clear: I don't want to persuade you no matter what. I understand you concern and as library author, it is you call indeed. I just want to pin out that adding one function (even imperfect for some less common cases) will resolve 99% of workarounds - cheap or not, creating whole proxy for each call seems heavy and kind of weird.

I really appreciate all the feedback. Whenever I build stuff I can never know how people are using them and what the pain points are unless you actually tell me.

For this proposal however, I still strongly believe that the current of the API should stay as is because even though it makes things look simpler, they are not. Basically we will be hiding the problem and not give a way to workaround possible issues. When we say it is up to the developer to make the function safe, the signature unit -> Async<string> doesn't suggest how to handle the possible errors.

Don't get me wrong, I am all in for simple defaults but in this case I would like to avoid introducing internal failure points such as this one and let the developer go with the more generic and explicit approach.

This is all in order to be able to build truly robust applications that know how to work handle errors properly. A proper proxy implementation would be something as follows. Given the type

type ServerResult<'T> = 
    | Data of 'T 
    | Unauthorized 
    | InternalServerError

Where Unauthorized can be returned either when acquiring an access token or when receiving response from the server. Building a proxy requires an auth token:

let buildProxy authToken =
    Remoting.createApi()
    |> Remoting.withRouteBuilder normalizeRoutes
    |> Remoting.withAuthorizationHeader authToken
    |> Remoting.buildProxy<IServerApi>

Then parameterize the proxy creation to handle the different kinds of possible errors:

module Server 

let acquireToken() : Async<Result<string, exn>> = (* do cool stuff here *)

let createAuthorizedApi() : Async<Result<IServerApi, exn>> = 
    async {
        match! acquireToken() with 
        | Ok token -> return Ok (buildProxy token) 
        | Error error -> return Error error
    }

let call (handler: IServerApi -> Async<'T>) : Async<ServerResult<'T>> = 
    async {
        let! proxy = createAuthorizedApi()
        try 
            match proxy with 
            | Ok serverApi -> 
                let! data = handler serverApi
                return Data data
            | Error exn -> 
                // token was not acquired
                return Unauthorized
        with error ->
            let proxyError = unbox<ProxyRequestException> error
            if proxyError.StatusCode = 401
            then return Unauthorized
            else return InternalServerError
    }

Now to using this is simple from the consumers of the Server API:

async {
  match! Server.call (fun api -> api.lessons()) with
  | Data lessons -> return Msg.Finished (Lessons lessons)
  | Unauthorized -> return Msg.RedirectToLogin
  | InternalServerError -> return ShowSadFace
}

Thanks again for the feedback, please do not let this deter you from giving me better suggestions for improvement :heart:

Dzoukr commented 4 years ago

Thank you anyway, really! It is kind of refreshing to sanely discuss, not getting into agreement and still have (hopefully mutual) good feeling about it. Something extraordinary in these days, I would say. ❤️ Have a great day and thanks for samples - it will help!