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.

Sign up for GitHub

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

Keep refresh token if the server does not return a new one on token refresh #83

Closed
wants to merge 1 commit into from
Closed

Keep refresh token if the server does not return a new one on token refresh #83

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions Heimdallr.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,8 @@
E258A16C1CC6BC8600649F5A /* OAuthAccessTokenDefaultParser.swift in Sources */ = {isa = PBXBuildFile; fileRef = E258A1651CC6B66900649F5A /* OAuthAccessTokenDefaultParser.swift */; };
E258A16D1CC6BC8700649F5A /* OAuthAccessTokenDefaultParser.swift in Sources */ = {isa = PBXBuildFile; fileRef = E258A1651CC6B66900649F5A /* OAuthAccessTokenDefaultParser.swift */; };
E258A16E1CC6BC8800649F5A /* OAuthAccessTokenDefaultParser.swift in Sources */ = {isa = PBXBuildFile; fileRef = E258A1651CC6B66900649F5A /* OAuthAccessTokenDefaultParser.swift */; };
E2A5A5031CF4D46A0087840B /* request-valid-norefresh.json in Resources */ = {isa = PBXBuildFile; fileRef = E2A5A5011CF4D4620087840B /* request-valid-norefresh.json */; };
E2A5A5041CF4D46B0087840B /* request-valid-norefresh.json in Resources */ = {isa = PBXBuildFile; fileRef = E2A5A5011CF4D4620087840B /* request-valid-norefresh.json */; };
/* End PBXBuildFile section */

/* Begin PBXContainerItemProxy section */
Expand Down Expand Up @@ -237,6 +239,7 @@
DC712A5E1C85AD77009860A5 /* watchOS-StaticLibrary.xcconfig */ = {isa = PBXFileReference; lastKnownFileType = text.xcconfig; path = "watchOS-StaticLibrary.xcconfig"; sourceTree = "<group>"; };
E258A1651CC6B66900649F5A /* OAuthAccessTokenDefaultParser.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = OAuthAccessTokenDefaultParser.swift; sourceTree = "<group>"; };
E258A1671CC6B8C500649F5A /* OAuthAccessTokenParser.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = OAuthAccessTokenParser.swift; sourceTree = "<group>"; };
E2A5A5011CF4D4620087840B /* request-valid-norefresh.json */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.json; path = "request-valid-norefresh.json"; sourceTree = "<group>"; };
/* End PBXFileReference section */

/* Begin PBXFrameworksBuildPhase section */
Expand Down Expand Up @@ -438,6 +441,7 @@
DC5220AD1BEA32A000F37F2A /* authorize-valid.json */,
DC5220AE1BEA32A000F37F2A /* request-invalid-norefresh.json */,
DC5220AF1BEA32A000F37F2A /* request-invalid.json */,
E2A5A5011CF4D4620087840B /* request-valid-norefresh.json */,
DC5220B01BEA32A000F37F2A /* request-valid.json */,
);
path = "JSON Responses";
Expand Down Expand Up @@ -813,6 +817,7 @@
isa = PBXResourcesBuildPhase;
buildActionMask = 2147483647;
files = (
E2A5A5041CF4D46B0087840B /* request-valid-norefresh.json in Resources */,
DC5220BF1BEA32A000F37F2A /* request-valid.json in Resources */,
DC5220B11BEA32A000F37F2A /* authorize-error.json in Resources */,
DC5220B71BEA32A000F37F2A /* authorize-invalid.json in Resources */,
Expand All @@ -835,6 +840,7 @@
isa = PBXResourcesBuildPhase;
buildActionMask = 2147483647;
files = (
E2A5A5031CF4D46A0087840B /* request-valid-norefresh.json in Resources */,
DC5220C01BEA32A000F37F2A /* request-valid.json in Resources */,
DC5220B21BEA32A000F37F2A /* authorize-error.json in Resources */,
DC5220B81BEA32A000F37F2A /* authorize-invalid.json in Resources */,
Expand Down
27 changes: 21 additions & 6 deletions Heimdallr/Core/Heimdallr.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,6 @@ public let HeimdallrErrorNotAuthorized = 2
get {
return accessTokenStore.retrieveAccessToken()
}
set {
accessTokenStore.storeAccessToken(newValue)
}
}
private let accessTokenParser: OAuthAccessTokenParser
private let httpClient: HeimdallrHTTPClient
Expand Down Expand Up @@ -76,7 +73,7 @@ public let HeimdallrErrorNotAuthorized = 2
/// **Note:** Sets the access token's expiration date to
/// 1 January 1970, GMT.
public func invalidateAccessToken() {
accessToken = accessToken?.copy(expiresAt: NSDate(timeIntervalSince1970: 0))
accessTokenStore.storeAccessToken(accessToken?.copy(expiresAt: NSDate(timeIntervalSince1970: 0)))
}

/// Clears the currently stored access token, if any.
Expand Down Expand Up @@ -143,8 +140,8 @@ public let HeimdallrErrorNotAuthorized = 2
} else if (response as! NSHTTPURLResponse).statusCode == 200 {
switch self.accessTokenParser.parse(data!) {
case let .Success(accessToken):
self.accessToken = accessToken
completion(.Success(accessToken))
let updatedAccessToken = self.updateAccessToken(accessToken)
completion(.Success(updatedAccessToken))
default:
let userInfo = [
NSLocalizedDescriptionKey: NSLocalizedString("Could not authorize grant", comment: ""),
Expand All @@ -170,6 +167,24 @@ public let HeimdallrErrorNotAuthorized = 2
}
}
}

/// Updates the stored access token with a new one.
///
/// - parameter accessToken: The new access token.
///
/// - returns: The updated access token.
private func updateAccessToken(accessToken: OAuthAccessToken) -> OAuthAccessToken {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would prefer this being part of the set part of accessToken just so that we always get the expected behaviour when setting a new token directly. What do you think?

var updatedAccessToken = accessToken

if accessToken.refreshToken == nil {
if let storedRefreshToken = self.accessToken?.refreshToken {
updatedAccessToken = accessToken.copy(refreshToken: storedRefreshToken)
}
}

accessTokenStore.storeAccessToken(updatedAccessToken)
return updatedAccessToken
}

/// Alters the given request by adding authentication with an access token.
///
Expand Down
34 changes: 34 additions & 0 deletions HeimdallrTests/Core/HeimdallrSpec.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -667,6 +667,36 @@ class HeimdallrSpec: QuickSpec {
}

context("when refreshing the access token succeeds") {
beforeEach {
OHHTTPStubs.stubRequestsPassingTest({ request in
return (request.URL!.absoluteString == "http://rheinfabrik.de")
}, withStubResponse: { request in
return OHHTTPStubsResponse(data: NSData(contentsOfFile: self.bundle.pathForResource("request-valid-norefresh", ofType: "json")!)!, statusCode: 200, headers: [ "Content-Type": "application/json" ])
})

waitUntil { done in
heimdallr.authenticateRequest(request) { result = $0; done() }
}
}

it("succeeds") {
expect(result?.value).toNot(beNil())
}

it("attempts to parse the fresh token") {
expect(accessTokenParser.timesCalled).to(equal(2))
}

it("authenticates the request using the resource request authenticator") {
expect(result?.value?.valueForHTTPHeaderField("MockAuthorized")).to(equal("totally"))
}

it("keeps the original refresh token") {
expect(accessTokenStore.retrieveAccessToken()?.refreshToken).to(equal("ZmVhMThjYzUyZDM3MmIzNDcyMDMyMzc2MzhmYTg4YWM0MWYyYmQxZmFlMTE2Mzk0MWY5YTk1YWQ4ZDBmYzIxZA"))
}
}

context("when refreshing the access token succeeds and overrides the refresh token") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the contexts can be rearranged so that this one and the one above read a bit nicer. I would propose something like this:

context("when refreshing the access token succeeds") {
  context("and the response does not contain a refresh token") {

  }

  context("and the response does contain a refresh token") {

  }
}

beforeEach {
OHHTTPStubs.stubRequestsPassingTest({ request in
return (request.URL!.absoluteString == "http://rheinfabrik.de")
Expand All @@ -690,6 +720,10 @@ class HeimdallrSpec: QuickSpec {
it("authenticates the request using the resource request authenticator") {
expect(result?.value?.valueForHTTPHeaderField("MockAuthorized")).to(equal("totally"))
}

it("overrides the refresh token") {
expect(accessTokenStore.retrieveAccessToken()?.refreshToken).to(equal("Vs25754VHY5CSovX2GRgLCLbKKZk8BQl7vmAoRav2agcY75uz338NyassLQCM8MeF04clqvtBeXG5o2wiuiTay"))
}
}

context("when refreshing the access token fails") {
Expand Down
1 change: 1 addition & 0 deletions HeimdallrTests/Core/JSON Responses/request-valid-norefresh.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"access_token":"MTQzM2U3YTI3YmQyOWQ5YzQ0NjY4YTZkYjM0MjczYmZhNWI1M2YxM2Y1MjgwYTg3NDk3ZDc4ZGUzM2YxZmJjZQ","expires_in":65536,"token_type":"bearer","scope":"user"}
2 changes: 1 addition & 1 deletion HeimdallrTests/Core/JSON Responses/request-valid.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"access_token":"MTQzM2U3YTI3YmQyOWQ5YzQ0NjY4YTZkYjM0MjczYmZhNWI1M2YxM2Y1MjgwYTg3NDk3ZDc4ZGUzM2YxZmJjZQ","expires_in":65536,"token_type":"bearer","scope":"user","refresh_token":"ZmVhMThjYzUyZDM3MmIzNDcyMDMyMzc2MzhmYTg4YWM0MWYyYmQxZmFlMTE2Mzk0MWY5YTk1YWQ4ZDBmYzIxZA"}
{"access_token":"MTQzM2U3YTI3YmQyOWQ5YzQ0NjY4YTZkYjM0MjczYmZhNWI1M2YxM2Y1MjgwYTg3NDk3ZDc4ZGUzM2YxZmJjZQ","expires_in":65536,"token_type":"bearer","scope":"user","refresh_token":"Vs25754VHY5CSovX2GRgLCLbKKZk8BQl7vmAoRav2agcY75uz338NyassLQCM8MeF04clqvtBeXG5o2wiuiTay"}