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

Jump to bottom

fix(@aws-amplify/auth): correctly throw the error when the refresh token is expired #1507

Merged
merged 3 commits into from
Aug 29, 2018
Merged

fix(@aws-amplify/auth): correctly throw the error when the refresh token is expired #1507

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c61505a
fix(@aws-amplify/auth): correctly throw the error when the refresh to…
powerful23 Aug 24, 2018
d034e19
add unit tests
powerful23 Aug 27, 2018
7adcca4
Merge branch 'master' into fix-issue-1468
powerful23 Aug 29, 2018
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
7 changes: 5 additions & 2 deletions packages/auth/__tests__/auth-unit-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1965,15 +1965,19 @@ describe('auth unit test', () => {
return callback('err', null);
});

expect.assertions(1);
const spyon3 = jest.spyOn(CognitoUser.prototype, 'getUserData');

expect.assertions(2);
try {
await auth.currentUserPoolUser();
} catch (e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test should be re-written to use expect().toThrow. The tests are more readable with this change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test will fail when using toThrow to catch a rejection. I followed the suggestion in this thread: jestjs/jest#1377 (comment)

expect(e).toBe('err');
expect(spyon3).not.toBeCalled();
}

spyon.mockClear();
spyon2.mockClear();
spyon3.mockClear();
});

test('get user data error because of user is deleted or disabled', async () => {
Expand Down Expand Up @@ -2042,7 +2046,6 @@ describe('auth unit test', () => {
spyon3.mockClear();
});
});
});

describe('sendCustomChallengeAnswer', () => {
test('happy case', async () => {
Expand Down
58 changes: 29 additions & 29 deletions packages/auth/src/Auth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -858,43 +858,43 @@ export default class AuthClass {
}

// refresh the session if the session expired.
user.getSession(function(err, session) {
user.getSession((err, session) => {
if (err) {
logger.debug('Failed to get the user session', err);
rej(err);
return;
}
});

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 unit test that ensures the these functions are not called concurrently

// get user data from Cognito
user.getUserData((err, data) => {
if (err) {
logger.debug('getting user data failed', err);
// Make sure the user is still valid
if (err.message === 'User is disabled' || err.message === 'User does not exist.') {
rej(err);
} else {
// the error may also be thrown when lack of permissions to get user info etc
// in that case we just bypass the error
res(user);
}
return;
}
const preferredMFA = data.PreferredMfaSetting || 'NOMFA';
const attributeList = [];

// get user data from Cognito
user.getUserData((err, data) => {
if (err) {
logger.debug('getting user data failed', err);
// Make sure the user is still valid
if (err.message === 'User is disabled' || err.message === 'User does not exist.') {
rej(err);
} else {
// the error may also be thrown when lack of permissions to get user info etc
// in that case we just bypass the error
res(user);
for (let i = 0; i < data.UserAttributes.length; i++) {
const attribute = {
Name: data.UserAttributes[i].Name,
Value: data.UserAttributes[i].Value,
};
const userAttribute = new CognitoUserAttribute(attribute);
attributeList.push(userAttribute);
}
return;
}
const preferredMFA = data.PreferredMfaSetting || 'NOMFA';
const attributeList = [];

for (let i = 0; i < data.UserAttributes.length; i++) {
const attribute = {
Name: data.UserAttributes[i].Name,
Value: data.UserAttributes[i].Value,
};
const userAttribute = new CognitoUserAttribute(attribute);
attributeList.push(userAttribute);
}

const attributes = this.attributesToObject(attributeList);
Object.assign(user, {attributes, preferredMFA});
res(user);
const attributes = that.attributesToObject(attributeList);
Object.assign(user, {attributes, preferredMFA});
res(user);
});
});
});
});
Expand Down