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.

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

Fixed failing unit test - invalidate call revoke endpoint twice. Unit test did not validate it correctly. #2349

Merged
merged 1 commit into from
Dec 10, 2021

Conversation

candunaj
Copy link
Contributor

@candunaj candunaj commented Dec 2, 2021

Issue

The unit test was testing the authenticator.invalidate method. It is calling revoke endpoint twice. One time revoke refresh_token and the second time revoke access_token. Unit test expected only one request for refresh_token. Call for revoke access_token was not expected and was failing quietly.

Fix

When the fake revoke endpoint is called, only the refresh token is validated.

@marcoow
Copy link
Member

marcoow commented Dec 2, 2021

Isn't this working correctly on master though, e.g. https://github.com/simplabs/ember-simple-auth/actions/runs/1530141659 ?

@candunaj
Copy link
Contributor Author

candunaj commented Dec 2, 2021

It is failing on master also. Revoke endpoint is called twice and it fails on the first request and succeeds on the second. It is not caught by mocha, because it is not correctly implemented. I can prove it by debugging.

Comment on lines 349 to 352
expect(revokeRequests.find(body => body.token_type_hint === 'access_token')).to.eql({
'token_type_hint': 'access_token',
'token': 'access token!'
});
Copy link
Member

Choose a reason for hiding this comment

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

Given the messages of the describe and test this is in, I would assume there's a dedicated test for asserting the access token is invalidated?

expect(revokeRequests.find(body => body.token_type_hint === 'refresh_token')).to.eql({
'token_type_hint': 'refresh_token',
'token': 'refresh token!'
});
Copy link
Member

Choose a reason for hiding this comment

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

I assume it would be simpler to just wrap the expect in line 340 in an if body.token_type_hint === 'refresh_token'.

Sorry, I think that's what you had originally but I didn't fully understand the issue before…

@marcoow marcoow merged commit ae6ca55 into master Dec 10, 2021
@delete-merged-branch delete-merged-branch bot deleted the fix-authenticator-invalidate-test branch December 10, 2021 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants