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

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

Merged
merged 7 commits into from
Jul 20, 2023
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
709c4a5
fix(core): make Signer.sign side-effect-ful
AllanZhengYP Jun 14, 2023
91d263f
Revert "fix(core): make Signer.sign side-effect-ful"
AllanZhengYP Jun 14, 2023
a14ac21
fix(api-rest): refactor ajax method to not relying on side effects
AllanZhengYP Jun 14, 2023
0302220
Merge branch 'main' into fix-graphql-skew
AllanZhengYP Jun 14, 2023
cb62a75
Merge branch 'main' into fix-graphql-skew
AllanZhengYP Jun 14, 2023
ea0094d
Merge branch 'main' into fix-graphql-skew
AllanZhengYP Jun 29, 2023
2d94b84
Merge branch 'main' into fix-graphql-skew
AllanZhengYP Jul 20, 2023
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
79 changes: 29 additions & 50 deletions packages/api-rest/__tests__/RestAPI-test.ts
View file
Open in desktop
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' },

Check failure

Code scanning / CodeQL

Hard-coded credentials

The hard-coded value "signed" is used as [authorization header](1).
});
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,39 +180,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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we need to call axios here and not this.ajax?

Copy link
Member Author

Choose a reason for hiding this comment

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

@elorzafe It's from the original implementation. My understanding it this prevents repeated call to this.Credentials.get() and reduce the duplicated debug messages caused by starting from beginning(this.ajax)

return isAllResponse ? response : response.data;
} catch (error) {
logger.debug(error);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a little bit of context to this log message?

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 @@ -365,7 +368,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 @@ -400,12 +403,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