From 9adc4bff96671bfa62aed5621097a69e272b4143 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Karol=20S=C3=B3jko?= Date: Wed, 13 Mar 2024 14:22:39 +0100 Subject: [PATCH 1/5] chore: add e2e test for refresh tokens cooldown period --- packages/api/src/Domain/Http/HttpService.ts | 15 +++++--- packages/snjs/mocha/session.test.js | 42 +++++++++++++++++++++ 2 files changed, 51 insertions(+), 6 deletions(-) diff --git a/packages/api/src/Domain/Http/HttpService.ts b/packages/api/src/Domain/Http/HttpService.ts index 24d0e2be6b8..8f0a070d761 100644 --- a/packages/api/src/Domain/Http/HttpService.ts +++ b/packages/api/src/Domain/Http/HttpService.ts @@ -173,12 +173,15 @@ export class HttpService implements HttpServiceInterface { if (this.inProgressRefreshSessionPromise) { await this.inProgressRefreshSessionPromise } else { - this.inProgressRefreshSessionPromise = this.refreshSession() - const isSessionRefreshed = await this.inProgressRefreshSessionPromise - this.inProgressRefreshSessionPromise = undefined - - if (!isSessionRefreshed) { - return response + const hasSessionTokenRenewedInBetweenOurRequest = httpRequest.authentication !== this.getSessionAccessToken() + if (!hasSessionTokenRenewedInBetweenOurRequest) { + this.inProgressRefreshSessionPromise = this.refreshSession() + const isSessionRefreshed = await this.inProgressRefreshSessionPromise + this.inProgressRefreshSessionPromise = undefined + + if (!isSessionRefreshed) { + return response + } } } diff --git a/packages/snjs/mocha/session.test.js b/packages/snjs/mocha/session.test.js index ce9bd2ce430..b2d3684f749 100644 --- a/packages/snjs/mocha/session.test.js +++ b/packages/snjs/mocha/session.test.js @@ -481,6 +481,48 @@ describe('server session', function () { await refreshPromise }) + it('should tell the client to refresh the token if one is used during the cooldown period after a refresh', async function () { + await Factory.registerUserToApplication({ + application: application, + email: email, + password: password, + }) + + const previousSession = application.legacyApi.session + + const refreshPromise = application.legacyApi.refreshSession() + + const setSessionFn = application.legacyApi.setSession + application.legacyApi.setSession = () => {} + const processSuccessResponseForMetaBodyFn = application.legacyApi.processSuccessResponseForMetaBody + application.legacyApi.processSuccessResponseForMetaBody = () => {} + const notifyEventFn = application.legacyApi.notifyEventSync + application.legacyApi.notifyEventSync = () => {} + + await refreshPromise + + const currentSession = application.legacyApi.session + + expect(previousSession.props.accessToken.value).to.not.equal(currentSession.props.accessToken.value) + expect(previousSession.props.refreshToken.value).to.not.equal(currentSession.props.refreshToken.value) + + application.legacyApi.session = previousSession + + const syncResponse = await application.legacyApi.sync([]) + expect(syncResponse.status).to.equal(200) + + const sessionRefreshedFromThePreviousSessionThatIsNewerThanCurrentSession = application.legacyApi.session + expect(sessionRefreshedFromThePreviousSessionThatIsNewerThanCurrentSession.props.accessToken.value).to.not.equal(previousSession.props.accessToken.value) + expect(sessionRefreshedFromThePreviousSessionThatIsNewerThanCurrentSession.props.refreshToken.value).to.not.equal(previousSession.props.refreshToken.value) + expect(sessionRefreshedFromThePreviousSessionThatIsNewerThanCurrentSession.props.accessToken.value).to.not.equal(currentSession.props.accessToken.value) + expect(sessionRefreshedFromThePreviousSessionThatIsNewerThanCurrentSession.props.refreshToken.value).to.not.equal(currentSession.props.refreshToken.value) + + + application.legacyApi.setSession = setSessionFn + application.legacyApi.processSuccessResponseForMetaBody = processSuccessResponseForMetaBodyFn + application.legacyApi.notifyEventSync = notifyEventFn + }) + it('notes should be synced as expected after refreshing a session', async function () { await Factory.registerUserToApplication({ application: application, From 4d4cabf1caed3898834e8a69fd0757b06476f60c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Karol=20S=C3=B3jko?= Date: Wed, 13 Mar 2024 15:51:40 +0100 Subject: [PATCH 2/5] chore: fix refreshing session in e2e --- packages/snjs/lib/Services/Api/ApiService.ts | 7 ++++++- packages/snjs/mocha/session.test.js | 12 ++++++------ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/packages/snjs/lib/Services/Api/ApiService.ts b/packages/snjs/lib/Services/Api/ApiService.ts index 2e2bd4dd489..d127e68f5ba 100644 --- a/packages/snjs/lib/Services/Api/ApiService.ts +++ b/packages/snjs/lib/Services/Api/ApiService.ts @@ -408,7 +408,12 @@ export class LegacyApiService } } - async refreshSession(): Promise> { + /** + * @deprecated + * + * This function should be replaced with @standardnotes/api's `HttpService::refreshSession` function. + */ + async deprecatedRefreshSessionOnlyUsedInE2eTests(): Promise> { const preprocessingError = this.preprocessingError() if (preprocessingError) { return preprocessingError diff --git a/packages/snjs/mocha/session.test.js b/packages/snjs/mocha/session.test.js index b2d3684f749..d0e4fbe5a2d 100644 --- a/packages/snjs/mocha/session.test.js +++ b/packages/snjs/mocha/session.test.js @@ -68,7 +68,7 @@ describe('server session', function () { password: password, }) - const response = await application.legacyApi.refreshSession() + const response = await application.legacyApi.deprecatedRefreshSessionOnlyUsedInE2eTests() expect(response.status).to.equal(200) expect(response.data.session.access_token).to.be.a('string') @@ -178,7 +178,7 @@ describe('server session', function () { expect(sessionFromStorage.refreshExpiration).to.equal(sessionFromApiService.refreshToken.expiresAt) expect(sessionFromStorage.readonlyAccess).to.equal(sessionFromApiService.isReadOnly()) - await application.legacyApi.refreshSession() + await application.http.refreshSession() const updatedSessionFromStorage = await getSessionFromStorage(application) const updatedSessionFromApiService = application.legacyApi.getSession() @@ -407,7 +407,7 @@ describe('server session', function () { await sleepUntilSessionExpires(application, false) - const refreshSessionResponse = await application.legacyApi.refreshSession() + const refreshSessionResponse = await application.legacyApi.deprecatedRefreshSessionOnlyUsedInE2eTests() expect(refreshSessionResponse.status).to.equal(400) /** @@ -452,7 +452,7 @@ describe('server session', function () { }) application.sessions.initializeFromDisk() - const refreshSessionResponse = await application.legacyApi.refreshSession() + const refreshSessionResponse = await application.legacyApi.deprecatedRefreshSessionOnlyUsedInE2eTests() expect(refreshSessionResponse.status).to.equal(400) expect(refreshSessionResponse.data.error.tag).to.equal('invalid-refresh-token') @@ -470,7 +470,7 @@ describe('server session', function () { password: password, }) - const refreshPromise = application.legacyApi.refreshSession() + const refreshPromise = application.legacyApi.deprecatedRefreshSessionOnlyUsedInE2eTests() const syncResponse = await application.legacyApi.sync([]) expect(syncResponse.data.error).to.be.ok @@ -490,7 +490,7 @@ describe('server session', function () { const previousSession = application.legacyApi.session - const refreshPromise = application.legacyApi.refreshSession() + const refreshPromise = application.legacyApi.deprecatedRefreshSessionOnlyUsedInE2eTests() const setSessionFn = application.legacyApi.setSession application.legacyApi.setSession = () => {} From 429f42003d605980ac7544cbc2dd646a55f882e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Karol=20S=C3=B3jko?= Date: Wed, 13 Mar 2024 16:15:35 +0100 Subject: [PATCH 3/5] chore: fix session refresh cooldown test --- packages/snjs/mocha/session.test.js | 41 ++++++++--------------------- 1 file changed, 11 insertions(+), 30 deletions(-) diff --git a/packages/snjs/mocha/session.test.js b/packages/snjs/mocha/session.test.js index d0e4fbe5a2d..0e9e25e0ec3 100644 --- a/packages/snjs/mocha/session.test.js +++ b/packages/snjs/mocha/session.test.js @@ -178,7 +178,7 @@ describe('server session', function () { expect(sessionFromStorage.refreshExpiration).to.equal(sessionFromApiService.refreshToken.expiresAt) expect(sessionFromStorage.readonlyAccess).to.equal(sessionFromApiService.isReadOnly()) - await application.http.refreshSession() + await application.legacyApi.deprecatedRefreshSessionOnlyUsedInE2eTests() const updatedSessionFromStorage = await getSessionFromStorage(application) const updatedSessionFromApiService = application.legacyApi.getSession() @@ -488,39 +488,20 @@ describe('server session', function () { password: password, }) - const previousSession = application.legacyApi.session - - const refreshPromise = application.legacyApi.deprecatedRefreshSessionOnlyUsedInE2eTests() - - const setSessionFn = application.legacyApi.setSession - application.legacyApi.setSession = () => {} - const processSuccessResponseForMetaBodyFn = application.legacyApi.processSuccessResponseForMetaBody - application.legacyApi.processSuccessResponseForMetaBody = () => {} - const notifyEventFn = application.legacyApi.notifyEventSync - application.legacyApi.notifyEventSync = () => {} - - await refreshPromise - - const currentSession = application.legacyApi.session - - expect(previousSession.props.accessToken.value).to.not.equal(currentSession.props.accessToken.value) - expect(previousSession.props.refreshToken.value).to.not.equal(currentSession.props.refreshToken.value) + const mimickApplyingSessionFromTheServerUnsuccessfully = () => {} + const originalSetSessionFn = application.http.setSession + const originalRefreshSessionCallbackFn = application.http.refreshSessionCallback + application.http.setSession = mimickApplyingSessionFromTheServerUnsuccessfully + application.http.refreshSessionCallback = mimickApplyingSessionFromTheServerUnsuccessfully - application.legacyApi.session = previousSession - - const syncResponse = await application.legacyApi.sync([]) - expect(syncResponse.status).to.equal(200) + await application.http.refreshSession() - const sessionRefreshedFromThePreviousSessionThatIsNewerThanCurrentSession = application.legacyApi.session - expect(sessionRefreshedFromThePreviousSessionThatIsNewerThanCurrentSession.props.accessToken.value).to.not.equal(previousSession.props.accessToken.value) - expect(sessionRefreshedFromThePreviousSessionThatIsNewerThanCurrentSession.props.refreshToken.value).to.not.equal(previousSession.props.refreshToken.value) - expect(sessionRefreshedFromThePreviousSessionThatIsNewerThanCurrentSession.props.accessToken.value).to.not.equal(currentSession.props.accessToken.value) - expect(sessionRefreshedFromThePreviousSessionThatIsNewerThanCurrentSession.props.refreshToken.value).to.not.equal(currentSession.props.refreshToken.value) + application.http.setSession = originalSetSessionFn + application.http.refreshSessionCallback = originalRefreshSessionCallbackFn + await application.sync.sync() - application.legacyApi.setSession = setSessionFn - application.legacyApi.processSuccessResponseForMetaBody = processSuccessResponseForMetaBodyFn - application.legacyApi.notifyEventSync = notifyEventFn + expect(application.sync.isOutOfSync()).to.equal(false) }) it('notes should be synced as expected after refreshing a session', async function () { From 89bb61dbcd97280de1448612712793b13065841f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Karol=20S=C3=B3jko?= Date: Wed, 13 Mar 2024 17:12:54 +0100 Subject: [PATCH 4/5] chore: fix e2e test --- packages/api/src/Domain/Http/HttpService.ts | 26 +++++++++++-------- .../src/Domain/Http/HttpServiceInterface.ts | 5 ++-- .../lib/Services/Session/SessionManager.ts | 9 ++++++- packages/snjs/mocha/session.test.js | 16 ++++++++---- 4 files changed, 37 insertions(+), 19 deletions(-) diff --git a/packages/api/src/Domain/Http/HttpService.ts b/packages/api/src/Domain/Http/HttpService.ts index 8f0a070d761..69601de0634 100644 --- a/packages/api/src/Domain/Http/HttpService.ts +++ b/packages/api/src/Domain/Http/HttpService.ts @@ -1,6 +1,6 @@ import { LoggerInterface, joinPaths, sleep } from '@standardnotes/utils' import { Environment } from '@standardnotes/models' -import { LegacySession, Session, SessionToken } from '@standardnotes/domain-core' +import { LegacySession, Result, Session, SessionToken } from '@standardnotes/domain-core' import { HttpStatusCode, HttpRequestParams, @@ -26,7 +26,7 @@ export class HttpService implements HttpServiceInterface { private declare host: string loggingEnabled = false - private inProgressRefreshSessionPromise?: Promise + private inProgressRefreshSessionPromise?: Promise>> private updateMetaCallback!: (meta: HttpResponseMeta) => void private refreshSessionCallback!: (session: Session) => void @@ -176,7 +176,11 @@ export class HttpService implements HttpServiceInterface { const hasSessionTokenRenewedInBetweenOurRequest = httpRequest.authentication !== this.getSessionAccessToken() if (!hasSessionTokenRenewedInBetweenOurRequest) { this.inProgressRefreshSessionPromise = this.refreshSession() - const isSessionRefreshed = await this.inProgressRefreshSessionPromise + const isSessionRefreshedResultOrError = await this.inProgressRefreshSessionPromise + let isSessionRefreshed = false + if (!isSessionRefreshedResultOrError.isFailed()) { + isSessionRefreshed = !isErrorResponse(isSessionRefreshedResultOrError.getValue()) + } this.inProgressRefreshSessionPromise = undefined if (!isSessionRefreshed) { @@ -193,13 +197,13 @@ export class HttpService implements HttpServiceInterface { return response } - async refreshSession(): Promise { + async refreshSession(): Promise>> { if (!this.session) { - return false + return Result.fail('No session to refresh') } if (this.session instanceof LegacySession) { - return false + return Result.fail('Cannot refresh legacy session') } const response = await this.post(Paths.v1.refreshSession, { @@ -208,7 +212,7 @@ export class HttpService implements HttpServiceInterface { }) if (isErrorResponse(response)) { - return false + return Result.ok(response) } if (response.meta) { @@ -220,7 +224,7 @@ export class HttpService implements HttpServiceInterface { response.data.session.access_expiration, ) if (accessTokenOrError.isFailed()) { - return false + return Result.fail(accessTokenOrError.getError()) } const accessToken = accessTokenOrError.getValue() @@ -230,21 +234,21 @@ export class HttpService implements HttpServiceInterface { response.data.session.refresh_expiration, ) if (refreshTokenOrError.isFailed()) { - return false + return Result.fail(refreshTokenOrError.getError()) } const refreshToken = refreshTokenOrError.getValue() const sessionOrError = Session.create(accessToken, refreshToken, response.data.session.readonly_access) if (sessionOrError.isFailed()) { - return false + return Result.fail(sessionOrError.getError()) } this.setSession(sessionOrError.getValue()) this.refreshSessionCallback(this.session) - return true + return Result.ok(response) } private params(inParams: Record): HttpRequestParams { diff --git a/packages/api/src/Domain/Http/HttpServiceInterface.ts b/packages/api/src/Domain/Http/HttpServiceInterface.ts index 421fa347c50..5736b75aa50 100644 --- a/packages/api/src/Domain/Http/HttpServiceInterface.ts +++ b/packages/api/src/Domain/Http/HttpServiceInterface.ts @@ -1,7 +1,8 @@ -import { LegacySession, Session } from '@standardnotes/domain-core' +import { LegacySession, Result, Session } from '@standardnotes/domain-core' import { HttpRequest, HttpRequestParams, HttpResponse, HttpResponseMeta } from '@standardnotes/responses' import { HttpRequestOptions } from './HttpRequestOptions' +import { SessionRefreshResponseBody } from '../Response' export interface HttpServiceInterface { setHost(host: string): void @@ -16,7 +17,7 @@ export interface HttpServiceInterface { runHttp(httpRequest: HttpRequest): Promise> setSession(session: Session | LegacySession): void - refreshSession(): Promise + refreshSession(): Promise>> setCallbacks( updateMetaCallback: (meta: HttpResponseMeta) => void, refreshSessionCallback: (session: Session) => void, diff --git a/packages/snjs/lib/Services/Session/SessionManager.ts b/packages/snjs/lib/Services/Session/SessionManager.ts index 72cbcb4da53..920e6a104cc 100644 --- a/packages/snjs/lib/Services/Session/SessionManager.ts +++ b/packages/snjs/lib/Services/Session/SessionManager.ts @@ -874,7 +874,14 @@ export class SessionManager const willRefreshTokenExpireSoon = refreshTokenExpiration.getTime() - Date.now() < ThirtyMinutes if (willAccessTokenExpireSoon || willRefreshTokenExpireSoon) { - return this.httpService.refreshSession() + const refreshSessionResultOrError = await this.httpService.refreshSession() + if (refreshSessionResultOrError.isFailed()) { + return false + } + + const refreshSessionResult = refreshSessionResultOrError.getValue() + + return isErrorResponse(refreshSessionResult) } return false diff --git a/packages/snjs/mocha/session.test.js b/packages/snjs/mocha/session.test.js index 0e9e25e0ec3..7e8c9880e83 100644 --- a/packages/snjs/mocha/session.test.js +++ b/packages/snjs/mocha/session.test.js @@ -494,14 +494,20 @@ describe('server session', function () { application.http.setSession = mimickApplyingSessionFromTheServerUnsuccessfully application.http.refreshSessionCallback = mimickApplyingSessionFromTheServerUnsuccessfully - await application.http.refreshSession() + const refreshResultOrError = await application.http.refreshSession() + expect(refreshResultOrError.isFailed()).to.equal(false) - application.http.setSession = originalSetSessionFn - application.http.refreshSessionCallback = originalRefreshSessionCallbackFn + const refreshResult = refreshResultOrError.getValue() + expect(isErrorResponse(refreshResult)).to.equal(false) - await application.sync.sync() + const secondRefreshResultOrErrorWithNotAppliedSession = await application.http.refreshSession() + expect(secondRefreshResultOrErrorWithNotAppliedSession.isFailed()).to.equal(false) - expect(application.sync.isOutOfSync()).to.equal(false) + const secondRefreshResultWithNotAppliedSession = secondRefreshResultOrErrorWithNotAppliedSession.getValue() + expect(isErrorResponse(secondRefreshResultWithNotAppliedSession)).to.equal(false) + + application.http.setSession = originalSetSessionFn + application.http.refreshSessionCallback = originalRefreshSessionCallbackFn }) it('notes should be synced as expected after refreshing a session', async function () { From 97a8d12e80282022c7bf88e426a537a431c7cae1 Mon Sep 17 00:00:00 2001 From: moughxyz Date: Wed, 13 Mar 2024 13:14:52 -0500 Subject: [PATCH 5/5] Add dropped response simulation test --- packages/api/src/Domain/Http/HttpService.ts | 6 ++++++ packages/snjs/mocha/session.test.js | 22 +++++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/packages/api/src/Domain/Http/HttpService.ts b/packages/api/src/Domain/Http/HttpService.ts index 69601de0634..3d10bdddd29 100644 --- a/packages/api/src/Domain/Http/HttpService.ts +++ b/packages/api/src/Domain/Http/HttpService.ts @@ -23,6 +23,7 @@ import { HttpRequestOptions } from './HttpRequestOptions' export class HttpService implements HttpServiceInterface { private session?: Session | LegacySession private __latencySimulatorMs?: number + private __simulateNextSessionRefreshResponseDrop = false private declare host: string loggingEnabled = false @@ -211,6 +212,11 @@ export class HttpService implements HttpServiceInterface { refresh_token: this.session.refreshToken.value, }) + if (this.__simulateNextSessionRefreshResponseDrop) { + this.__simulateNextSessionRefreshResponseDrop = false + return Result.fail('Simulating a dropped response') + } + if (isErrorResponse(response)) { return Result.ok(response) } diff --git a/packages/snjs/mocha/session.test.js b/packages/snjs/mocha/session.test.js index 7e8c9880e83..3ca733e193d 100644 --- a/packages/snjs/mocha/session.test.js +++ b/packages/snjs/mocha/session.test.js @@ -510,6 +510,28 @@ describe('server session', function () { application.http.refreshSessionCallback = originalRefreshSessionCallbackFn }) + it('if session renewal response is dropped, next sync with server should return a 498 and successfully renew the session', async function () { + await Factory.registerUserToApplication({ + application: application, + email: email, + password: password, + }) + + await sleepUntilSessionExpires(application) + + const refreshSpy = sinon.spy(application.http, 'refreshSession') + + /** + * With this sync, we expect refreshSession to be called twice, once where the response is dropped, + * and the other time where the request succeeds + */ + application.http.__simulateNextSessionRefreshResponseDrop = true + await application.sync.sync(syncOptions) + await application.sync.sync(syncOptions) + + expect(refreshSpy.callCount).to.equal(2) + }) + it('notes should be synced as expected after refreshing a session', async function () { await Factory.registerUserToApplication({ application: application,