Allowing parameters and headers to be reused across providers

[SD10]

Background

This is inspired by issue #1273 and its matching pull request #1279.

Basically, #1273 discusses extending the behavior of Moya's current AccessTokenPlugin to support adding an access_token as a URL parameter to a request.

While this parameter can be added directly to the parameters of a TargetType, the fact that it needs to be applied to every request makes it a good candidate for improving the existing AccessTokenPlugin.

AccessTokenPlugin Current Behavior

Currently, the AccessTokenPlugin only supports authentication using an "Authorization" header with the values of "Basic" or "Bearer". Supporting auth via parameters would be a new concept for this plugin.

Proposed Solutions

1.) Adding another case to the AuthorizationType enum:

public enum AuthorizationType: String {
    case basic = "Basic"
    case bearer = "Bearer"
    case urlParamter = "access_token"
    case none
}

This raises the question: What if the API has different parameter names? Ex) api_key or api_token

2.) Using associated values over raw values:

public enum AuthorizationType {
    case header(name: String)
    case urlParameter(name: String)
    case none
}

3. Specifying the parameter name or header name in the AccessTokenPlugin initializer:

init(tokenClosure: ..., parameterName: ..., headerName: ...) { ... }

enum AuthorizationType {
    case basicInHeaders
    case bearerInHeaders
    case basicInParameters
    case bearerInParameters
    case none
}

4. Splitting option 3 into two enums and requesting optional AuthorizationType in the target, where nil will mean no authorization will be appended:

enum AuthorizationType {
    case basic
    case bearer
}

enum AuthorizationTypePlacement {
    case header
    case parameter
}

5. Providing a HeaderPlugin and ParameterPlugin:

protocol HeaderApplicable {
    var applyPluginHeaders: Bool
}
public struct HeaderPlugin: PluginType {
    var headerValue: String
    var headerName: String
}
protocol ParameterApplicable {
    var applyPluginParameters: Bool
}
public struct ParameterPlugin: PluginType {
   var parameterValue: Any
   var parameterName: String
   var encoding: ParameterEncoding
}

Approach 5 moves away from the domain of Authentication and looks at the problem as being we need to reuse a specific parameter or specific header on requests across multiple providers or targets.

Sorry for the essay 😅

[sunshinejr]

Note: I've added one option (4.) which is another version of option 3.

First of all thanks @SD10 for writing this up! 👏 What I think that we need to do is to agree on what we want to achieve. By gathering info from the discussion, right now we need (feel free to correct me if I'm wrong):

1. Availability to add basic/bearer token in headers
2. Availability to add basic/bearer token in parameters
3. Availability to not adding token (when plugin is on)
4. Availability to mix all of these in one TargetType implementation
5. Availability to change name of the header/parameter - now I'm not sure if we can achieve this on a flexible enough level, so we may just start with making the name global for the plugin (which is probably the most popular use-case)
6. Easy to use (e.g. typing the name of the parameter/header in each TargetType might be cumbersome)

If these are correct, we can make a quick rundown and check which option meets the criteria and if there are more than one, add additional criteria or see pros/cons.

Edit: I've also added the new label - discussion - as I've felt we were lacking this one and it fits here and with sample data perfectly for me. Let me know if we should reconsider that, though!

[SD10]

@sunshinejr I think the discussion tag is a great idea because I couldn't find a fitting label for this.

I'm still having trouble wrapping my head around having a plugin that's responsible for headers and url parameters. What happens if we want both of them on a single request?

Looking at option (4.) we can only return a single case of .header or .parameter.

I realize (5.) is a little out of scope for what started as a simple issue, I would be fine going with an easy/temporary fix to move things forward.

But I do think this issue starts to address an underlying issue with plugins. They seem to be brittle and not easy to combine/extend. I'd like to spend some time looking at #1123 too.

[stale]

This issue has been marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale]

This issue has been auto-closed because there hasn't been any activity for at least 37 days. However, we really appreciate your contribution, so thank you for that! 🙏 Also, feel free to open a new issue if you still experience this problem 👍.

[stale]

This issue has been auto-closed because there hasn't been any activity for at least 37 days. However, we really appreciate your contribution, so thank you for that! 🙏 Also, feel free to open a new issue if you still experience this problem 👍.

[SD10]

I don't know what's happening here Mr. Stalebot but I'm going to trigger this issue to be re-opened.

[sunshinejr]

@SD10 looking back at your comment right now, I'm not sure if we want to support sending both token in parameters and in headers in one request. In comparison, I would love to use token in headers and token in parameters in two endpoints, using one plugin, though.

[AndrewSB]

Great write-up. My two cents: I haven't ever used the AccessTokenPlugin since my subroutine for getting an access token is usually async, where I return an Observable that checks disk, make sure the token is still valid, and if not hits the network.

Maybe making Rx versions of the plugins would be a nice alternative? I'm not sure that our plugins right now are encouraging the best, most correct use of our APIs

[SD10]

I have to review my own thoughts here because I lost the the "deep" understanding I had regarding the problem.

@AndrewSB I'm not sure if supporting reactive plugins makes sense if we don't also provide a non-reactive component in the core library.

However, I do agree with you about the current plugins not encouraging the best use of Moya's API. Personally, I have never used the AccessTokenPlugin either -- which may further demonstrate the point above.

[ffittschen]

Similar to @AndrewSB, after working on #1393 I also ended up creating an async version with RxSwift by using an EndpointClosure etc. and ended up not using the AccessTokenPlugin.

But I'd really like to use it! My solution kinda feels hacky and I'd like to abstract the authentication out into some piece of code, as the AccessTokenPlugin, because my EndpointClosure violates the single responsibility and the interface segregation, because it basically is a general purpose solution.

So looking at the proposed solutions above, I think that 1 and 3 are not scalable. On the one hand I like proposal 2, because it enables complete freedom, on the other hand I think that complete freedom again enables a general purpose solution. When I created #1393 I looked for "official" resources or best practices of header authentication. For named authentication methods we should not change the current implementation, as the RFCs of Basic Authentication or Bearer Authentication are quite explicit, that it should look like the current implementation. But as those two are not the only ones 1 2, we should create a structure that allows to conform to custom authentication methods too. Especially, because not every API (as in my case in #1393) has correctly implemented authentication and might look for arbitrary headers.

Apart from that, using access tokens as parameter is considered as NOT OK by owasp: https://www.owasp.org/index.php/REST_Security_Cheat_Sheet#Sensitive_information_in_HTTP_requests

Regarding proposal 5: I don't think that we should create a general header / parameter plugin, because those should be specified in the TargetType extension, right?

[SD10]

@ffittschen Thanks for this feedback 💯 I agree 2 is more scalable than 1 and 3. The reason I suggested 5 is because I tried to look at things from one level above the AccessTokenPlugin.

I wasn't active in Moya when this plugin was created but I'm going to make the assumption that it was designed to reduce repetitive code among TargetTypes for requests the require some form of authentication.

This makes me believe that a core underlying issue that plugins should aim to solve is code re-use among the target types.

[stale]

This issue has been marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[sunshinejr]

@ffittschen Thank you so much for the detailed answer! You make very good points and I agree with most of them. From yours and @SD10's feedback I've came with an improved idea:

enum AuthorizationType {
    case basic
    case bearer
    case custom(prefix: String)

    var prefix: String {
        switch self {
            case .bearer: return "Bearer"
            case .basic: return "Authorization"
            case let .custom(prefix): return prefix
        }
    }
}

enum AuthorizationTypePlacement {
    case header
    case parameter
}

public protocol AccessTokenAuthorizable {
    var authorizationType: (AuthorizationType, AuthorizationTypePlacemenet)? { get }
}

This one allows Bearer/Basic authorization, custom prefix in case you need one, headers/parameters placement and opting in/out of the plugin depending on the endpoint.

The authorizationType is an optional tuple of two types, because if you don't need authorization for an endpoint, you will return .none, but if you want an authorization, you need to pass both type and placement. We could do a typealias for authorizationType, but ran short on names 😄

I'd love to get your feedback on that one 👍

[SD10]

@sunshinejr I think we can move forward with that approach. Trying to re-evaluate the plugin system isn't really getting us anywhere (and I've been less active recently). I think we've delayed solving this long enough as we've already had ~3 PRs requesting different prefixes.

I think the idea is solid, not too fond of the tuple to be honest. I think a typealias or wrapping it in a struct may be useful.

Would like to hear @ffittschen 's thoughts or any others

[stale]

This issue has been marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[sunshinejr]

I'm gonna work on this one in the next week as Moya 11 should be released ASAP (ReactiveSwift guys probably want to play with a new toy 😉)

[ffittschen]

Sorry for the late response..
I like the idea, but I wouldn't remove the .none case in favor of an optional. And in your prefix computed property, you are mixing the header field and the token prefix. The RFC conform header is always Authorization, and the RFC conform token prefix is Bearer, Basic etc. based on the HTTP authentication scheme. I would say, that we should make the RFC conform usage as easy as possible and if customizations are necessary, it is ok to require some additional parameters.

Since there is no RFC (that I know of) that describes how parameter based authentication should look like, there are a lot of different ways: GitHub and Bitbucket require a access_token=TOKEN parameter, and GitLab requires a private_token=TOKEN parameter, so the parameter name should be customizable as well.

Looking at the naming of the security scheme of the OpenAPI specification by swagger, I came up with the following idea in which the http case would be the RFC conform implementation and the apiKey is a fully customizable option:

/// HTTP authentication scheme (https://www.iana.org/assignments/http-authschemes/http-authschemes.xhtml)
enum AuthenticationScheme {
    case basic
    case bearer

    var tokenPrefix: String {
        switch self {
        case .basic:
            return "Basic"
        case .bearer:
            return "Bearer"
        }
    }
}

enum AuthenticationPlacement {
    case header
    case queryParameter
}

enum AuthenticationType {
    case none
    case http(scheme: AuthenticationScheme)
    case apiKey(name: String, placement: AuthenticationPlacement)

    var placement: AuthenticationPlacement? {
        switch self {
        case .none:
            return nil
        case .http:
            return .header
        case .apiKey(_, let placement):
            return placement
        }
    }

    var header: String? {
        switch self {
        case .http:
            return "Authorization"
        case .apiKey(let name, let placement) where placement == .header:
            return name
        default:
            return nil
        }
    }

    var queryParameter: String? {
        guard case .apiKey(let name, let placement) = self, placement == .queryParameter else {
            return nil
        }

        return name
    }
}

protocol Authenticatable {
    var authenticationType: AuthenticationType { get }
}

Example usage:

enum YourAPI: Authenticatable {
    case targetDoesNotNeedAuth
    case targetThatNeedsBearerAuth
    case targetThatNeedsBasicAuth
    case targetThatNeedsCustomParameterAuth
    case targetThatNeedsCustomHeaderAuth

    var authenticationType: AuthenticationType {
        switch self {
        case .targetDoesNotNeedAuth:
            return .none
        case .targetThatNeedsBearerAuth:
            return .http(scheme: .bearer)
        case .targetThatNeedsBasicAuth:
            return .http(scheme: .basic)
        case .targetThatNeedsCustomParameterAuth:
            return .apiKey(name: "access_token", placement: .queryParameter)
        case .targetThatNeedsCustomHeaderAuth:
            return .apiKey(name: "X-Access-Token", placement: .header)
        }
    }
}

The only thing that didn't make it into this proposal is the the case: Authentication with custom header and token prefix. But by adding the prefix to the tokenClosure this case would be feasible as well

[ffittschen]

Additionally, every time I'm giving the AccessTokenPlugin a new chance and I use RxSwift for my networking code, I get stuck when I need to provide a tokenClosure, so I fall back to using a plain endpoint closure again. Since the project board says Improving AccessTokenPlugin / plugins in general do you think this should be addressed as well?

[SD10]

@ffittschen What about the tokenClosure causes a roadblock for you and do you have any ideas as to how we can resolve that issue?

[sunshinejr]

Sorry for the delay guys. Your implementation @ffittschen looks pretty good. Would you be up for implementing it in a PR? If not, maybe @SD10 would be up for doing it - I would like to focus on CI right now.

Also about the tokenClosure, I think the autoclosure might be a blocker. It seemed like a good idea at first, but it's a mediocre solution for me (from experience in setting up stacks with this plugin in it).

[ffittschen]

@SD10 @sunshinejr Yes I also think that the @autoclosure might be the problem.
But ultimately, what I do at the moment is: For every request, I use an endpoint closure, that will look if there is a token stored in the keychain and inject a new request to refresh the token in case it is already expired or there is no token present.

This somehow feels hacky, because the tokenClosure again checks for a token in the keychain and returns "" if there is no token present. We should handle this case a bit better, because at the moment that empty string will just be used as a token and result in a 401. I guess my problem is mostly, that I'd like to have a way to specify a request that will fetch a new token additionally to specifying the tokenClosure.

I'll create a PR with my current version but I'll also think about a way to handle the case, where an empty token would be added to a target that has not .none as AuthenticationType.

[SD10]

@sunshinejr Can we agree that this should not be a blocker for Moya 11.0? I think we have enough breaking changes in development to move forward with a beta release. I can work on this in the next major version.

[sunshinejr]

@SD10 I agree 👍

[ffittschen]

@sunshinejr I started working on the PR you mentioned in #1283 (comment) during the holidays.
Shall I finish it today or do you want to do a proper "Improve plugins in general" as mentioned on the project board after releasing 11.0?
/cc @SD10

[SD10]

I'm not sure if "improving plugins in general" is necessary @ffittschen. It's hard to get an active discussion on the topic and we shouldn't break things on a whim. Part of naming the issue this way was I was looking at this issue from a higher level. I think the underlying problem is not that people want to authorize requests, but rather that they want to reuse headers and or parameters across multiple providers.

[SD10]

I'm going to close this because I don't think introducing this level of complexity via plugins is a good fit for Moya. I may write up some examples of how to achieve some of the things discussed above. If anyone wants to continue the discussion feel free to reopen.

[ethi1989]

@SD10 so,where is the examples? I need help to archive add "token" to parameters, thank you!