MBL-1214: Send OAuth token only in headers, not in request parameters #1954
Conversation
| @@ -41,6 +41,26 @@ extension Service { | |||
| ).handle_combine_dataResponse(service: self) | |||
| } | |||
|
|
|||
| func requestWithAuthentication<Model: Decodable>(_ route: Route, | |||
There was a problem hiding this comment.
The amount of copy-paste here makes me unhappy. However, I poked around with a few ideas for refactoring, and realized it was going to take significantly more refactoring to make a dent in this. I decided that (for the moment) this was an OK evil.
Because this is only used by precisely one route, I would much rather have one ugly copy-pasted method than plumb an extra parameter throughout our whole networking stack. And I would rather ship this now than embark in a larger refactoring project for Route.
There was a problem hiding this comment.
Just to confirm: this is necessary because we're calling this as soon as we have the token; before the token has been set (since we set the token when we have the user as well)? If I'm understanding this correctly, I support this as an OK evil, especially if we still think we may get the user object returned with the token in the future.
There was a problem hiding this comment.
Correct. It's a catch-22 - our login method requires both a user and a token; but our new token exchange endpoint (for throttling/speed reasons) only returns the token, not the user. So we needed a way to fetch the user before we've actually completed login and saved the token to the apiService.
| @@ -490,37 +490,6 @@ extension ServiceType { | |||
| return self.preparedRequest(forRequest: request, query: query) | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
These methods are unused; deleting for clarity.
| @@ -605,7 +599,6 @@ extension ServiceType { | |||
| var query: [String: String] = [:] | |||
| query["client_id"] = self.serverConfig.apiClientAuth.clientId | |||
| query["currency"] = self.currency | |||
| query["oauth_token"] = self.oauthToken?.token | |||
There was a problem hiding this comment.
👋 Goodbye, token in request params.
| @@ -248,9 +247,6 @@ internal enum Route { | |||
| case .userSelf: | |||
| return (.GET, "/v1/users/self", [:], nil) | |||
|
|
|||
| case let .userSelfWithToken(token): | |||
There was a problem hiding this comment.
Unnecessary; handling this at the Service level.
| } | ||
| .receive(on: RunLoop.main) |
There was a problem hiding this comment.
Whoops, this wasn't applying to the fetchUserSelf_combine call because it happened before. 🤦♀️
amy-at-kickstarter
changed the title
amy-at-kickstarter
requested review from
a team and
ifosli
and removed request for
a team
SwiftFormat found issues:
Generated by 🚫 Danger |
API V1 also uses basic auth in the Authorization header; adding this extra header lets the server resolve the conflict between the two.
… in a header, not in request params
amy-at-kickstarter
force-pushed
the
feat/adyer/MBL-1214/auth-token-as-header
branch
from
4cf77e8
to
763799d
Compare
| @@ -41,6 +41,26 @@ extension Service { | |||
| ).handle_combine_dataResponse(service: self) | |||
| } | |||
|
|
|||
| func requestWithAuthentication<Model: Decodable>(_ route: Route, | |||
There was a problem hiding this comment.
Just to confirm: this is necessary because we're calling this as soon as we have the token; before the token has been set (since we set the token when we have the user as well)? If I'm understanding this correctly, I support this as an OK evil, especially if we still think we may get the user object returned with the token in the future.
amy-at-kickstarter
deleted the
feat/adyer/MBL-1214/auth-token-as-header
branch
📲 What
Send OAuth tokens only in the request headers, not in the request parameters.
🤔 Why
This is a security concern which we wanted to address with the MVP of OAuth. Moving the authorization parameters into headers is a best practice.
See: https://github.com/kickstarter/kickstarter/pull/26425 and https://github.com/kickstarter/kickstarter/pull/26432
🛠 How
Note that this isn't behind any flags. The server change to accept the OAuth token in the headers was made globally, for all V1 requests, and already applied for GraphQL. We decided a ramp-up would be difficult, due to how deep the headers are buried in our networking stack, and of limited use, compared to testing this change manually.
Also note that we are sometimes sending redundant headers, like both
AuthorizationandX-Authwith the OAuth token. To the best of my knowledge, both V1 and GraphQL discard extra headers, so there's no harm in this - and it makes our code significantly simpler.