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

Fix memory leak issue when using fetch in oidc authentication #2640

Merged
merged 13 commits into from
Jan 3, 2025
Merged
7 changes: 7 additions & 0 deletions addons/auth/addon/authenticators/base.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ export default class BaseAuthenticator extends SimpleAuthBaseAuthenticator {
*/
async validateToken(token, tokenID) {
const tokenValidationURL = this.buildTokenValidationEndpointURL(tokenID);

// Note: waitForPromise is needed to provide the necessary integration with @ember/test-helpers
// visit https://www.npmjs.com/package/@ember/test-waiters for more info.
const response = await waitForPromise(
Expand All @@ -66,6 +67,12 @@ export default class BaseAuthenticator extends SimpleAuthBaseAuthenticator {
headers: { Authorization: `Bearer ${token}` },
}),
);
// Note: Always consume response body in order to avoid memory leaks
// visit https://undici.nodejs.org/#/?id=garbage-collection for more info.
lisbet-alvarez marked this conversation as resolved.
Show resolved Hide resolved
// We do not use the undici package but the link informs us that garbage
// collection is undefined when response body is not consumed.
await response.json();

// 401 and 404 responses mean the token is invalid, whereas other types of
// error responses do not tell us about the validity of the token.
if (response.status === 401 || response.status === 404) return reject();
Expand Down
10 changes: 8 additions & 2 deletions addons/auth/addon/authenticators/oidc.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { reject } from 'rsvp';
import { waitForPromise } from '@ember/test-waiters';

/**
* The OIDC base authenticator encapsulates the multistep OIDC flow.
* The OIDC base authenticator encapsulates the multi-step OIDC flow.
*
* 1. Start authentication flow: this step is actually a combination of two
* sub steps:
Expand Down Expand Up @@ -97,13 +97,19 @@ export default class OIDCAuthenticator extends BaseAuthenticator {
});
// Fetch the endpoint and get the response JSON
const response = await waitForPromise(fetch(url, { method: 'post', body }));

// Note: Always consume response body in order to avoid memory leaks
// visit https://undici.nodejs.org/#/?id=garbage-collection for more info.
// We do not use the undici package but the link informs us that garbage
// collection is undefined when response body is not consumed.
const json = await response.json();

if (response.status === 202) {
// The token isn't ready yet, keep trying.
return false;
} else if (response.status < 400) {
// Response was successful, meaning a token was obtained.
// Authenticate with the session service using the response JSON.
const json = await response.json();
await this.session.authenticate('authenticator:oidc', json);
return true;
} else {
Expand Down
6 changes: 3 additions & 3 deletions addons/auth/tests/unit/authenticators/base-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ module('Unit | Authenticator | base', function (hooks) {
const authenticator = this.owner.lookup('authenticator:base');
server.get(authenticator.buildTokenValidationEndpointURL(id), () => {
assert.ok(true, 'token validation was requested');
return [200];
return [200, {}, '{}'];
});
await authenticator.restore(mockData);
});
Expand All @@ -64,7 +64,7 @@ module('Unit | Authenticator | base', function (hooks) {
const authenticator = this.owner.lookup('authenticator:base');
server.get(authenticator.buildTokenValidationEndpointURL(id), () => {
assert.ok(true, 'token validation was requested');
return [401];
return [401, {}, '{}'];
});
try {
await authenticator.restore(mockData);
Expand All @@ -80,7 +80,7 @@ module('Unit | Authenticator | base', function (hooks) {
const authenticator = this.owner.lookup('authenticator:base');
server.get(authenticator.buildTokenValidationEndpointURL(id), () => {
assert.ok(true, 'token validation was requested');
return [404];
return [404, {}, '{}'];
});
try {
await authenticator.restore(mockData);
Expand Down
Loading