Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide a pattern for dynamic reconfiguration of network transport / auth headers #37

Closed
timbotnik opened this issue Jan 5, 2017 · 55 comments
Milestone

Comments

@timbotnik
Copy link
Contributor

@timbotnik timbotnik commented Jan 5, 2017

It's not clear how to use the API to implement login / logout, as it would require us to reconfigure headers. We’ll need a way to dynamically configure / reconfigure the client instance to provide different auth headers, perhaps other transport options as well.

@idris
Copy link

@idris idris commented Jan 6, 2017

FYI I just have a shared Apollo client instance, and any time the session changes (login/logout/expired, etc), I just re-configure a new Apollo client and replace it.

@xiekevin
Copy link

@xiekevin xiekevin commented Jan 27, 2017

For what it's worth, the solution @idris brought up is pretty much what we're doing as well

@fruitcoder
Copy link

@fruitcoder fruitcoder commented Feb 6, 2017

How do you handle queries that fail because of invalid/expired tokens?
Do you catch a 401 in the completion handler of the apollo client and refresh the token? Then the request would still have failed and just the next request might go through (if token refresh was successful).
I would prefer to have a delegate method with an incoming url request and a closure that takes another url request. Here I would refresh my token, update the header for the request and call the closure with the now authenticated header request which is consumed by the apollo client.

@MrAlek
Copy link
Contributor

@MrAlek MrAlek commented Feb 7, 2017

I'd prefer this to not be in the network transport as a delegate but rather maybe an authentication delegate on the client. Currently, the network transport is the only customization point (see discussion in #6) and if you'd implement your own transport, you would then have to re-implement authentication.

Some cases the authentication mechanism should be able to handle (long-term):

  • Inject means of authentication to the operation before it's sent to the network transport
  • Blocking an outgoing operation from being sent to the network transport (if unauthorized)
  • Re-authenticating before an operation is sent (if token, etc needs to refresh)
  • (optionally) Handle unauthorized responses by either failing or re-authenticating. (could be handled by the application)

@martijnwalraven
Copy link
Contributor

@martijnwalraven martijnwalraven commented Feb 7, 2017

@MrAlek: Thanks for writing these cases down! The more I think about this, the more I realize this will take some effort to get right. And we should probably think about this in conjunction with #6.

One idea would be to implement something akin to RequestAdapter and RequestRetrier in Alamofire. These are fairly low-level callbacks, but they do offer quite a bit of flexibility. You could then perform the authentication yourself, or plug in something like p2/OAuth. (Or perform exponential backoff on connection failures, etc.)

My first thought would be to put these on a HTTPNetworkTransportDelegate, because the callbacks are tied to URLRequest, and not every network transport will use this:

public protocol HTTPNetworkTransportDelegate: class {
  func networkTransport(_ networkTransport: HTTPNetworkTransport, shouldSend request: URLRequest) -> Bool
  func networkTransport(_ networkTransport: HTTPNetworkTransport, willSend request: inout URLRequest)
  func networkTransport(_ networkTransport: HTTPNetworkTransport, request: URLRequest, didFailWithError error: Error, retryHandler: (_ shouldRetry: Bool) -> Void)
}

I'm not entirely happy with this because URLRequest doesn't provide any information about the GraphQL operation, and you may want that to decide your authentication or retry policy. We could pass the operation in as well, although that requires making these methods generic (because GraphQLOperation has an associated type).

Also, as you mentioned, this ties it to one particular network transport. An alternative might be to consider HTTPNetworkTransport abstract and allow for implementations using different network stacks.

The main issue seems to be that the 'means of authentication' is actually network transport specific. So it is hard to do this in a generic way. A web socket transport for example, wouldn't allow you to perform authentication on a per-request basis, but only for the connection as a whole.

There is also some work to be done to still allow network operations to be Cancellable, if they are not tied to a specific network task. So we may want to introduce a GraphQLRequest subclass of NSOperation. At that point, I'm wondering if we can't somehow take advantage of operation dependencies and OperationQueue to simplify some of this.

What do you think? How does this compare to the network stack you're currently using?

@fruitcoder
Copy link

@fruitcoder fruitcoder commented Feb 16, 2017

Wow, I didn't know about the RequestAdapter and RequestRetrier in Alamofire but they are exactly how I would want the authentication process to work. Whether you put it in the Network or the Client doesn't really matter to me, both have pros and cons.

Using NSOperations always sounds promising at first, but for me it often felt too complicated wrapping URLSessionTasks in operations. Also, since UrlSession uses an operation queue under the hood anyways, it felt like duplicating and overcomplicating things. But the dependencies and limiting the number of concurrent operations are a huge benefit of using operations in general.

I think it makes sense to have generic callbacks so you can base your authentication/retry policy based on the queries/operations!

@fruitcoder
Copy link

@fruitcoder fruitcoder commented Feb 28, 2017

Could we - until a better solution is found - add another initializer to HTTPNetworkTransport to inject an NSURLSession instead of just the configuration? That way, one could handle authentication challenges in the url session delegate methods.

@fruitcoder
Copy link

@fruitcoder fruitcoder commented May 20, 2017

Any update? :)

@martijnwalraven
Copy link
Contributor

@martijnwalraven martijnwalraven commented May 21, 2017

@fruitcoder: Hey, sorry, I've been preoccupied with the new store and caching API, which has taken much longer than expected. Once this lands, we're in a better position to redesign the network layer, also thinking about the WebSocket transport and subscriptions.

@fruitcoder
Copy link

@fruitcoder fruitcoder commented May 22, 2017

Cool, then I'll stick with my fork for now. Nice work on the store and caching!

@sciutand
Copy link

@sciutand sciutand commented May 23, 2017

Hi, I just bumped in to this. Personally I think that the use of the NetworkTransport protocol gives enough customisation opportunities without making Apollo too opinionated in handling requests.

Here is an example of using Alamofire and SessionManager.

Once you are in control of the SessionManager you can add your RequestAdapter, RequestRetriers, background sessions etc etc

import Foundation
import Apollo
import Alamofire


enum GQLError: Error {
  case failedToParseResponseToJSONObject
}

class AlamofireNetworkTransport: NetworkTransport, URLConvertible {

  let sessionManager: SessionManager
  let url: URL

  init(url: URL, sessionManager: SessionManager = SessionManager.default) {
    self.sessionManager = sessionManager
    self.url = url
  }

  func send<Operation>(operation: Operation, completionHandler: @escaping (GraphQLResponse<Operation>?, Error?) -> Void) -> Cancellable where Operation : GraphQLOperation {

    let body: GraphQLMap = ["query": type(of: operation).queryDocument, "variables": operation.variables]
    return sessionManager
      .request(self, method: .post, parameters: body, encoding: JSONEncoding.default)
      .validate(statusCode: [200])
      .responseJSON { response in
        let gqlResult = response.result.flatMap{ value -> GraphQLResponse<Operation> in
          guard let value = value as? JSONObject else {
            throw AFError.responseSerializationFailed(reason: .jsonSerializationFailed(error: GQLError.failedToParseResponseToJSONObject))
          }
          return GraphQLResponse(operation: operation, body: value)
        }
        completionHandler(gqlResult.value, gqlResult.error)

    }.task!
  }

  func asURL() throws -> URL {
    return url
  }
}

@vishal-p
Copy link

@vishal-p vishal-p commented Jun 22, 2017

Hi @martijnwalraven

Good work on SQLLite persistence and I'm using 0.6.0 beta. We need network layer customization for updating Auth headers. When do you think it will be available?

@martijnwalraven
Copy link
Contributor

@martijnwalraven martijnwalraven commented Jun 22, 2017

@vishal-p: The network layer is already customizable by providing your own implementation of NetworkTransport, see the AlamofireNetworkTransport code above for example. I'd still like to spend to some time to make auth easier out of the box however, hopefully next week.

@vishal-p
Copy link

@vishal-p vishal-p commented Jun 22, 2017

Thanks for the reply @martijnwalraven, tried above code but got crash on return GraphQLResponse(operation: operation, body: value) will investigate more on this . Will look forward to out of box solution.

@vishal-p
Copy link

@vishal-p vishal-p commented Jul 6, 2017

hi @martijnwalraven Is this enhancement available now with 0.6.0 release ?

@jasonsilberman
Copy link
Contributor

@jasonsilberman jasonsilberman commented Aug 3, 2017

Just saw this thread, has anyone had a chance to work on this?

@MaximusMcCann
Copy link

@MaximusMcCann MaximusMcCann commented Aug 4, 2017

Same, this is quite important here :)

@ultrazhangster
Copy link

@ultrazhangster ultrazhangster commented Aug 22, 2017

Any update, folks?

@sergiog90
Copy link

@sergiog90 sergiog90 commented Sep 7, 2017

Hi,

my solution is based on original HTTPNetworkTransport form Apollo.
I make a custom HTTPNetworkTransport with dynamic custom headers injection.

AuthHTTPNetworkTransport

You only need to add your headers (line 37) based on your local settings, like Authorization header.

The client creation is like original
let client = ApolloClient(networkTransport: AuthHTTPNetworkTransport(url: url))

@matteinn
Copy link

@matteinn matteinn commented Oct 4, 2017

@sergiog90 so aren't you sharing the same ApolloClient instance for all your requests?

@sergiog90
Copy link

@sergiog90 sergiog90 commented Oct 12, 2017

@matteinn I'm sharing a single instance for all requests and adding dynamically headers for any performed request through the client.

@DarkDust
Copy link

@DarkDust DarkDust commented Oct 18, 2017

I'd like to share our use-case why we would really love to see support for this and how we'd like to use it:

We're using OAuth2 for authorization, via AppAuth. For each network request sent to the server, the token may need to be refreshed which is an asynchronous operation.

So Apollo needs to ask our app for the additional headers which would be an asynchronous operation. For example, Apollo could pass a completion block that our app then has to call once the tokens have been refreshed. We would then pass the additional headers as parameters to the completion block.

Right now we're using a modified version of Apollo and need to wrap the Apollo calls so the tokens get refreshed and can be passed to Apollo… not a good solution.

@DarkDust
Copy link

@DarkDust DarkDust commented Oct 23, 2017

So I was playing around with an implementation to allow a delegate of HTTPNetworkTransport to asynchronously modify a request before it's sent. The biggest "issue" would be that almost every function that returns a Cancellable would need to instead return a Promise<Cancellable> (likewise, some instance variables would need to changed accordingly. I'm not sure if that would be acceptable. Thoughts?

@DarkDust
Copy link

@DarkDust DarkDust commented Oct 24, 2017

I came up with a design that doesn't need the Promise<Cancellable>. Will make a pull request in a couple of days.

@martijnwalraven
Copy link
Contributor

@martijnwalraven martijnwalraven commented Oct 24, 2017

That would be great! Sorry, have been really busy with GraphQL Summit.

@MrAlek
Copy link
Contributor

@MrAlek MrAlek commented Oct 24, 2017

@martijnwalraven How about going for that middleware pattern Apollo Link uses? That would solve most of this.

@martijnwalraven
Copy link
Contributor

@martijnwalraven martijnwalraven commented Oct 24, 2017

@MrAlek Yes, I think that makes sense. Want to take a stab at a design so we can discuss here? I haven't had any time to think about this since our last conversation.

@MrAlek
Copy link
Contributor

@MrAlek MrAlek commented Oct 24, 2017

@martijnwalraven I don't have much time right now either I'm afraid :/ We've worked around this particular problem right now by not using ApolloClient in the few cases we need to manually insert authentication per-request.

@DarkDust have you looked at Apollo Link? I think that context-passing middleware concept would work in the iOS client too.

@MaxDesiatov
Copy link
Contributor

@MaxDesiatov MaxDesiatov commented Dec 14, 2018

This feature will be possible to be written by users if library will allow to provide custom NetworkTransport

Sorry @wtrocki, I'm a bit confused, could you please clarify what you mean by this? Apollo iOS library already allows providing a custom NetworkTransport, you can check an example of a custom implementation of that provided in ApolloAlamofire library.

@wtrocki
Copy link

@wtrocki wtrocki commented Dec 14, 2018

@MaxDesiatov This is something that I was looking for (and possibly all developers who wanted to extend core networking). This library should be added to docs for people who already use Alamofire.
Really good work!

@MaxDesiatov
Copy link
Contributor

@MaxDesiatov MaxDesiatov commented Dec 14, 2018

Thank you very much @wtrocki, that's great to hear! I've mentioned this library multiple times in related issues of this repository and also Apollo Slack, but it doesn't look there's any interest from the maintainers of Apollo to mention ApolloAlamofire anywhere in the docs. I actually don't mind that and hope that users can still discover it directly.

@wtrocki
Copy link

@wtrocki wtrocki commented Jan 17, 2019

@MaxDesiatov Library is amazing and it is been used quite extensively from what I see. Are you open for collaboration on this? As chances to get this functionality to core is fairly limited maybe we could move that to some aggregator where more developers can join and actively maintain it. This will be also great way to discover it (which is why I think now we still have so many PR's to the core trying to resolve it by duplicating functionality). We can put that into apollo-community organization and invite users that are actively using them.

WDYT?

@MaxDesiatov
Copy link
Contributor

@MaxDesiatov MaxDesiatov commented Jan 17, 2019

Sure, that's quite reasonable, I wouldn't mind having something like apollo-community and would greatly appreciate if it was somehow linked. Look forward to hearing more details on how this organization would be governed and if there are any additional criteria or requirements.

@D-Link13
Copy link

@D-Link13 D-Link13 commented Mar 27, 2019

@martijnwalraven your thoughts about using ApolloAlamofire?

@postmechanical
Copy link

@postmechanical postmechanical commented Apr 23, 2019

Came to this issue via https://www.apollographql.com/docs/ios/initialization#adding-headers. Will that doc ever be updated with the conclusions here? Will this issue actually ever turn into changes in the SDK?

@wtrocki
Copy link

@wtrocki wtrocki commented Apr 23, 2019

I think there is wider discussion now in the community about support more advanced use cases for IOS and getting things merged.

@postmechanical
Copy link

@postmechanical postmechanical commented Apr 23, 2019

@wtrocki What does that mean with respect to this particular issue? Which specific solution will be chosen?

@wtrocki
Copy link

@wtrocki wtrocki commented Apr 23, 2019

ApolloAlamofire mentioned couple times here resolves a lots of issues and IMHO should be listed in documentation as alternative to internal transports. If there is any use case that this lib is not resolving lets ping team or create issue in the repo.

EDIT: Library is now available under graphql-community umbrella (https://github.com/graphql-community/ApolloAlamofire) with the number of people tracking issues. @MaxDesiatov done amazing work on the library.

The way I see it is that core networking will be working for simple use cases but if app needs to work with OAuth, cert pinning, send metrics etc. ApolloAlamofire provides a great alternative, especially that most of the apps will already have Alamofire configured. Couple PR's created for Apollo-ios are targeting the problem that ApolloAlamofire resolves.

@postmechanical
Copy link

@postmechanical postmechanical commented Apr 23, 2019

There's plenty of us who don't want yet another dependency, particularly Alamofire. Not an acceptable solution at all. @wtrocki

@wtrocki
Copy link

@wtrocki wtrocki commented Apr 23, 2019

Yes. It is just alternative that has features that still did not landed in apollo-ios library.

@MaxDesiatov
Copy link
Contributor

@MaxDesiatov MaxDesiatov commented Apr 24, 2019

I'm wondering if anyone would be interested in using a new potential package similar to ApolloAlamofire without Alamofire as a dependency, which would use NSURLSession directly?

@RolandasRazma
Copy link
Contributor

@RolandasRazma RolandasRazma commented Apr 24, 2019

would be great if it would build in as any auth requires it. Don't want to reply on another "package"...

@designatednerd
Copy link
Contributor

@designatednerd designatednerd commented Jul 1, 2019

Very much still subject to change but a draft on cancel-before-flight and swap-out-info-on-the-request stuff is up as #602

@designatednerd designatednerd moved this from In Progress to Done in Developer Experience Improvement Jul 10, 2019
@designatednerd designatednerd added this to the 0.11.0 milestone Jul 10, 2019
@designatednerd
Copy link
Contributor

@designatednerd designatednerd commented Jul 10, 2019

Shipped with v0.11.0!

@Drusy
Copy link

@Drusy Drusy commented Jul 11, 2019

Hey @designatednerd, from what I understand, this will work only in case of malformed response or netwok error ?
The delegate HTTPNetworkTransportRetryDelegate won't be called in case of correct GraphQL response but functional errors filled in GraphQLResult.errors from body["errors"]?

@designatednerd
Copy link
Contributor

@designatednerd designatednerd commented Jul 11, 2019

Yes, that's correct - because the same request is retried, there's no reason to think that if you get errors back from the server, you won't get those same errors sending again.

The only exception I can think of is for Automatic Persisted Queries, which will be special cased. Right now #608 is looking like the most promising implementation for that.

@Drusy
Copy link

@Drusy Drusy commented Jul 11, 2019

In our case, the missing authentication error (401) is located in body["errors"] and the HTTPNetworkTransportRetryDelegate would allow us to refresh the JWT token and retry the request.

@designatednerd
Copy link
Contributor

@designatednerd designatednerd commented Jul 11, 2019

Dammit, I love that I wrote this code last week and I already forgot what it does 🙃.

Wherever the error is created when dealing with a response, it hits the handleErrorOrRetry method, which will hit the retrier if it exists. Only if the retrier determines a retry should not be done will it then actually return the original error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet