Skip to content

Commit

Permalink
fix(api-rest): refactor ajax method to not relying on side effects (#…
Browse files Browse the repository at this point in the history 
…11498)

* fix(api-rest): refactor ajax method to not relying on side effects

the previous Signer.sign() implementation not only returns signed
request, but also sets the signed headers to input request. The
recent refactor makes it side-effect-less.

However this change breaks RestClient.ajax() clock skew correction
which relies on the x-amz-date header set by signer to the input
request object to indicate the current client side time.

this fix resolves #11480
  • Loading branch information
@AllanZhengYP @david-mcafee
AllanZhengYP authored and david-mcafee committed Jul 24, 2023
1 parent 000820d commit a3c6f73
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 89 deletions.
79 changes: 29 additions & 50 deletions packages/api-rest/__tests__/RestAPI-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import { Signer, Credentials, DateUtils } from '@aws-amplify/core';

jest.mock('axios');

const mockAxios = jest.spyOn(axios as any, 'default');

axios.CancelToken = <CancelTokenStatic>{
source: () => ({ token: null, cancel: null }),
};
Expand All @@ -28,6 +30,7 @@ const config = {

afterEach(() => {
jest.restoreAllMocks();
mockAxios.mockClear();
});

describe('Rest API test', () => {
Expand Down Expand Up @@ -166,19 +169,11 @@ describe('Rest API test', () => {
api.configure(custom_config);
const spyon = jest
.spyOn(Credentials, 'get')
.mockImplementationOnce(() => {
return new Promise((res, rej) => {
res('cred');
});
});
.mockResolvedValueOnce('cred');

const spyonRequest = jest
.spyOn(RestClient.prototype as any, '_request')
.mockImplementationOnce(() => {
return new Promise((res, rej) => {
res({});
});
});
.mockResolvedValueOnce({});
await api.get('apiName', 'path', {});

expect(spyonRequest).toBeCalledWith(
Expand Down Expand Up @@ -221,25 +216,15 @@ describe('Rest API test', () => {
session_token: 'token',
};

const spyon = jest.spyOn(Credentials, 'get').mockImplementation(() => {
return new Promise((res, rej) => {
res(creds);
});
});
const spyon = jest.spyOn(Credentials, 'get').mockResolvedValue(creds);

const spyonSigner = jest
.spyOn(Signer, 'sign')
.mockImplementationOnce(() => {
return { headers: {} };
});

const spyAxios = jest
.spyOn(axios as any, 'default')
.mockImplementationOnce(() => {
return new Promise((res, rej) => {
res(resp);
});
});
mockAxios.mockResolvedValue(resp);

const init = {
timeout: 2500,
Expand Down Expand Up @@ -297,13 +282,7 @@ describe('Rest API test', () => {
return { headers: {} };
});

const spyAxios = jest
.spyOn(axios as any, 'default')
.mockImplementationOnce(() => {
return new Promise((res, rej) => {
res(resp);
});
});
mockAxios.mockResolvedValue(resp);

const init = {
queryStringParameters: {
Expand Down Expand Up @@ -363,13 +342,7 @@ describe('Rest API test', () => {
return { headers: {} };
});

const spyAxios = jest
.spyOn(axios as any, 'default')
.mockImplementationOnce(() => {
return new Promise((res, rej) => {
res(resp);
});
});
mockAxios.mockResolvedValue(resp);

const init = {
queryStringParameters: {
Expand Down Expand Up @@ -429,13 +402,7 @@ describe('Rest API test', () => {
return { headers: {} };
});

const spyAxios = jest
.spyOn(axios as any, 'default')
.mockImplementationOnce(() => {
return new Promise((res, rej) => {
res(resp);
});
});
mockAxios.mockResolvedValue(resp);

const init = {
queryStringParameters: {
Expand Down Expand Up @@ -557,19 +524,31 @@ describe('Rest API test', () => {
.mockImplementation(() => 'endpoint');

jest.spyOn(Credentials, 'get').mockResolvedValue('creds');

jest
.spyOn(RestClient.prototype as any, '_signed')
.mockRejectedValueOnce(normalError);
jest.spyOn(RestClient.prototype as any, '_sign').mockReturnValue({
...init,
headers: { ...init.headers, Authorization: 'signed' },
});
mockAxios.mockImplementationOnce(() => {
return new Promise((_, rej) => {
rej(normalError);
});
});

await expect(api.post('url', 'path', init)).rejects.toThrow(normalError);

// Clock should not be skewed from normal errors
expect(DateUtils.getClockOffset()).toBe(0);

jest
.spyOn(RestClient.prototype as any, '_signed')
.mockRejectedValueOnce(clockSkewError);
// mock clock skew error response and successful response after retry
mockAxios
.mockImplementationOnce(() => {
return new Promise((_, rej) => {
rej(clockSkewError);
});
})
.mockResolvedValue({
data: [{ name: 'Bob' }],
});

await expect(api.post('url', 'path', init)).resolves.toEqual([
{ name: 'Bob' },
Expand Down
76 changes: 37 additions & 39 deletions packages/api-rest/src/RestClient.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,39 +171,42 @@ export class RestClient {
return this._request(params, isAllResponse);
}

// Signing the request in case there credentials are available
return this.Credentials.get().then(
credentials => {
return this._signed({ ...params }, credentials, isAllResponse, {
region,
service,
}).catch(error => {
if (DateUtils.isClockSkewError(error)) {
const { headers } = error.response;
const dateHeader = headers && (headers.date || headers.Date);
const responseDate = new Date(dateHeader);
const requestDate = DateUtils.getDateFromHeaderString(
params.headers['x-amz-date']
);

// Compare local clock to the server clock
if (DateUtils.isClockSkewed(responseDate)) {
DateUtils.setClockOffset(
responseDate.getTime() - requestDate.getTime()
);

return this.ajax(urlOrApiInfo, method, init);
}
}

throw error;
});
},
err => {
logger.debug('No credentials available, the request will be unsigned');
return this._request(params, isAllResponse);
let credentials;
try {
credentials = await this.Credentials.get();
} catch (error) {
logger.debug('No credentials available, the request will be unsigned');
return this._request(params, isAllResponse);
}
let signedParams;
try {
signedParams = this._sign({ ...params }, credentials, {
region,
service,
});
const response = await axios(signedParams);
return isAllResponse ? response : response.data;
} catch (error) {
logger.debug(error);
if (DateUtils.isClockSkewError(error)) {
const { headers } = error.response;
const dateHeader = headers && (headers.date || headers.Date);
const responseDate = new Date(dateHeader);
const requestDate = DateUtils.getDateFromHeaderString(
signedParams.headers['x-amz-date']
);

// Compare local clock to the server clock
if (DateUtils.isClockSkewed(responseDate)) {
DateUtils.setClockOffset(
responseDate.getTime() - requestDate.getTime()
);

return this.ajax(urlOrApiInfo, method, init);
}
}
);
throw error;
}
}

/**
Expand Down Expand Up @@ -356,7 +359,7 @@ export class RestClient {

/** private methods **/

private _signed(params, credentials, isAllResponse, { service, region }) {
private _sign(params, credentials, { service, region }) {
const { signerServiceInfo: signerServiceInfoParams, ...otherParams } =
params;

Expand Down Expand Up @@ -391,12 +394,7 @@ export class RestClient {

delete signed_params.headers['host'];

return axios(signed_params)
.then(response => (isAllResponse ? response : response.data))
.catch(error => {
logger.debug(error);
throw error;
});
return signed_params;
}

private _request(params, isAllResponse = false) {
Expand Down

0 comments on commit a3c6f73

Please sign in to comment.