SDKS-4348: Implement token.revoke in OIDC client #376
Conversation
🦋 Changeset detectedLatest commit: c48d9c8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
View your CI Pipeline Execution ↗ for commit c48d9c8
☁️ Nx Cloud last updated this comment at |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #376 +/- ##
=======================================
Coverage 55.47% 55.47%
=======================================
Files 32 32
Lines 2044 2044
Branches 340 340
=======================================
Hits 1134 1134
Misses 910 910 🚀 New features to boost your workflow:
|
|
Deployed 218d5be to https://ForgeRock.github.io/ping-javascript-sdk/pr-376/218d5be77b793e1e5114e58f2494fa19e21258af branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis📊 Minor Changes📈 @forgerock/oidc-client - 23.1 KB (+0.7 KB) ➖ No Changes➖ @forgerock/davinci-client - 34.1 KB 11 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
| }, | ||
| }), | ||
| userInfo: builder.mutation<TokenExchangeResponse, { accessToken: string; endpoint: string }>({ | ||
| userInfo: builder.mutation<unknown, { accessToken: string; endpoint: string }>({ |
There was a problem hiding this comment.
Why are we making this change?
There was a problem hiding this comment.
I can revert this but I thought the TokenExchangeResponse type was a mistake. The user endpoint doesn't return tokens, it returns a user object which varies depending on scope. I wasn't sure if we were guaranteed a Record<string,string> so I followed what we did in the old SDK and typed it as unknown.
There was a problem hiding this comment.
Yes, the original type was an error, but I don't want to use unknown. The structure of the response is known, though much of it is optional. It's defined by the spec. Let's provide a more explicitly structured object, instead of unknown.
There was a problem hiding this comment.
I think this is the spec for PingOne server which only requires sub in the response. Is there a spec for PingAM server somewhere?
https://apidocs.pingidentity.com/pingone/auth/v1/api/#get-userinfo-get
| }), | ||
| // Delete local token and return combined results | ||
| Micro.flatMap((revokeResponse) => | ||
| Micro.promise(async () => { |
There was a problem hiding this comment.
Nothing wrong with this, but in my brain when I read this it feels semi-redundant to make the callback async, await a promise, and then return an object, in a micro.
I think of it more in sequences like so:
Micro.flatMap(revokeResponse =>
Micro.promise(() => storageClient.remove()),
Micro.map(deleteResponse => ({ deleteResponse, revokeResponse }))
)Like I said this is more of a nitpick so feel free to ignore.
There was a problem hiding this comment.
I updated it and added a pipe. Please take a look.
| }), | ||
| ), | ||
| ).pipe( | ||
| Micro.map(({ error }) => { |
There was a problem hiding this comment.
Another nitpick - but i'm a little against this pattern we somewhat have of handling errors in the map.
Because RTK doesn't explicitly error, it appears like these are successful sequences.
I think we already have the logic though to move these to the error channel.
if we have an error, we should fail the effect (micro).
if (error) {
return Micro.fail(error)
}
).pipe(
Micro.mapError(error => {
// error logic here
}),Now, because we don't use _tag to identify errors, I don't think we can use catchTags but thats okay for now I think.
Why would I suggest this?
The pattern itself allows us to handle errors differently or the same for a given effect. While in this effect it may not be perfectly represented here is the thought.
- build a reusable effect
- produce a failure when the effect fails
- allows the consumer of the effect (Micro) to handle the error how the consumer wants to.
- doesn't conflate successful sequences with errors.
Building in an error into the successful computations, in my eyes, is an anti-pattern.
One may argue, that because the error itself, is a value, its "successful" but this is what Micro/Effect already does for us. It branches these two channels, and still returns them as a value.
There was a problem hiding this comment.
Can we discuss this as a team at a later time? I think I understand your intention here, but I'd like to be sure we test this out together and ensure we are on the same page with the desired technical outcome. I'd encourage us to go ahead and merge this as it's consistent with the existing pattern.
After the discussion, we can update both to be consistent.
There was a problem hiding this comment.
This was challenging for me so I think I will leave this refactoring for another time unless you have a better suggestion. I agree that we should Micro.fail when we come across an error but I couldn't find a way to do this gracefully. Here's what I came up with which we can discuss another time:
const revokeNew = Micro.zip(
// Revoke the access token
Micro.promise(() =>
store.dispatch(
oidcApi.endpoints.revoke.initiate({
accessToken: tokens.accessToken,
clientId: config.clientId,
endpoint: wellknown.revocation_endpoint,
}),
),
),
// Delete local token
Micro.promise(() => storageClient.remove()),
).pipe(
Micro.flatMap(([revokeResponse, deleteResponse]) => {
const { error } = revokeResponse;
if (error) {
return Micro.fail(error);
}
return Micro.succeed({ revokeResponse: null, deleteResponse });
}),
);
const revokeResult = Micro.mapError(revokeNew, (error) => {
let message = 'An error occurred while revoking the token';
let status: number | string = 'unknown';
if ('message' in error && error.message) {
message = error.message;
}
if ('status' in error) {
status = error.status;
}
return {
revokeResponse: {
error: 'Token revocation failure',
message,
type: 'auth_error',
status,
} as GenericError,
deleteResponse: undefined,
};
});
const result = await Micro.runPromiseExit(revokeResult);
if (exitIsSuccess(result)) {
return result.value;
} else if (exitIsFail(result)) {
return result.cause.error;
} else {
return {
error: 'Token revocation failure',
message: result.cause.message,
type: 'auth_error',
};
}
There was a problem hiding this comment.
sure we can discuss later.
There was a problem hiding this comment.
for future reference, this should do what I am suggesting
const revokeµ = Micro.promise(() =>
store.dispatch(
oidcApi.endpoints.revoke.initiate({
accessToken: tokens.accessToken,
clientId: config.clientId,
endpoint: wellknown.revocation_endpoint,
}),
),
).pipe(
Micro.flatMap(({ error }) => {
if (error) {
return Micro.fail(error);
}
return Micro.succeed(null);
}),
Micro.mapError((error) => {
let message = 'An error occurred while revoking the token';
let status: number | string = 'unknown';
if ('message' in error && error.message) {
message = error.message;
}
if ('status' in error) {
status = error.status;
}
return {
error: 'Token revocation failure',
message,
type: 'auth_error',
status,
} as GenericError;
}),
// Delete local token and return combined results
Micro.flatMap((revokeResponse) =>
Micro.promise(() => storageClient.remove()).pipe(
Micro.map((deleteResponse) => ({ revokeResponse, deleteResponse })),
),
),
);There was a problem hiding this comment.
Cool, thanks, Ryan. Now that we have a complete sample of code, we can discuss the pros and cons. Appreciate the effort here :)
| return null; | ||
| }), | ||
| // Delete local token and return combined results | ||
| Micro.flatMap((revokeResponse) => |
ancheetah
force-pushed
the
SDKS-4348-token-revoke
branch
from
e6f746d to
c7490ee
Compare
e2e/oidc-app/src/utils/oidc-app.ts
Outdated
| const userInfo = await oidcClient.user.info(); | ||
|
|
||
| if ('error' in userInfo) { | ||
| if (typeof userInfo === 'object' && 'error' in userInfo) { |
There was a problem hiding this comment.
Is TypeScript communicating that userInfo might not be an object? If so, something is wrong as it should either be a user info object or a generic error.
There was a problem hiding this comment.
This was because I changed the user info return type to unknown. This will change though once I look up the spec and better define the return type
e2e/oidc-app/src/utils/oidc-app.ts
Outdated
| const response = await oidcClient.token.revoke(); | ||
| const isError = checkLogoutOrRevokeError(response); | ||
|
|
||
| if (isError) { |
There was a problem hiding this comment.
Should we have a root property on the object that signals an error within one of the responses?
There was a problem hiding this comment.
That would be helpful. Something like isError: boolean;. But I think it would be nicer if we returned an error type if error and a success type if success. Something like:
type RevokeResult = {
revokeResponse: null;
deleteResponse: void;
};
type RevokeError = {
error: {
revokeResponse: GenericError;
deleteResponse: void;
};
};
In which case RevokeResult seems silly and we can get rid of it altogether. However, these sort of types would require a refactor of revoke and logout.
e2e/oidc-suites/src/token.spec.ts
Outdated
| await expect(page.locator('#accessToken-0')).not.toBeEmpty(); | ||
| await expect(page.locator('#accessToken-0')).not.toHaveText('undefined'); |
There was a problem hiding this comment.
Do we need both of these? Could we improve how we handle the DOM so "undefined" is not injected.
e2e/oidc-suites/src/token.spec.ts
Outdated
| await clickButton('Login (Background)', 'https://openam-sdks.forgeblocks.com/'); | ||
|
|
||
| await page.getByLabel('User Name').fill(pingAmUsername); | ||
| await page.getByRole('textbox', { name: 'Password' }).fill(pingAmPassword); | ||
| await page.getByRole('button', { name: 'Next' }).click(); | ||
|
|
||
| await page.waitForURL('http://localhost:8443/ping-am/**', { waitUntil: 'networkidle' }); | ||
| expect(page.url()).toContain('code'); | ||
| expect(page.url()).toContain('state'); | ||
|
|
||
| await expect(page.locator('#accessToken-0')).not.toBeEmpty(); | ||
| await expect(page.locator('#accessToken-0')).not.toHaveText('undefined'); | ||
|
|
||
| await page.evaluate(() => window.localStorage.clear()); | ||
| const token = await page.evaluate(() => localStorage.getItem('pic-oidcTokens')); | ||
| await expect(token).toBeFalsy(); |
There was a problem hiding this comment.
Do we need to go through this whole login flow to then delete the token to test this error case? Could we do what's done just above?
e2e/oidc-suites/src/user.spec.ts
Outdated
| await clickButton('Login (Background)', 'https://openam-sdks.forgeblocks.com/'); | ||
|
|
||
| await page.getByLabel('User Name').fill(pingAmUsername); | ||
| await page.getByRole('textbox', { name: 'Password' }).fill(pingAmPassword); | ||
| await page.getByRole('button', { name: 'Next' }).click(); | ||
|
|
||
| await page.waitForURL('http://localhost:8443/ping-am/**'); | ||
| expect(page.url()).toContain('code'); | ||
| expect(page.url()).toContain('state'); | ||
| await expect(page.locator('#accessToken-0')).not.toBeEmpty(); | ||
| await expect(page.locator('#accessToken-0')).not.toHaveText('undefined'); | ||
|
|
||
| await page.evaluate(() => window.localStorage.clear()); |
There was a problem hiding this comment.
This too. Can we just call user info without logging in first to then clear the tokens?
| }), | ||
| ), | ||
| ).pipe( | ||
| Micro.map(({ error }) => { |
There was a problem hiding this comment.
Can we discuss this as a team at a later time? I think I understand your intention here, but I'd like to be sure we test this out together and ensure we are on the same page with the desired technical outcome. I'd encourage us to go ahead and merge this as it's consistent with the existing pattern.
After the discussion, we can update both to be consistent.
| }, | ||
| }), | ||
| userInfo: builder.mutation<TokenExchangeResponse, { accessToken: string; endpoint: string }>({ | ||
| userInfo: builder.mutation<unknown, { accessToken: string; endpoint: string }>({ |
There was a problem hiding this comment.
Yes, the original type was an error, but I don't want to use unknown. The structure of the response is known, though much of it is optional. It's defined by the spec. Let's provide a more explicitly structured object, instead of unknown.
| }), | ||
| // Delete local token and return combined results | ||
| Micro.flatMap((revokeResponse) => | ||
| Micro.promise(() => storageClient.remove()).pipe( |
There was a problem hiding this comment.
This is good.
Semantically, it may "feel" cleaner if we wrap both effects in a pipe, but not a big deal.
Alternative being:
pipe(
Micro.promise(() => storageClient.remove()),
Micro.map((deleteResponse) => ({ revokeResponse, deleteResponse }))
)This is entirely subjective so i'll leave it to you what you feel reads better they are functionally equivalent if i have my mental model correct.
ancheetah
force-pushed
the
SDKS-4348-token-revoke
branch
2 times, most recently
from
24ddb2e to
45c2862
Compare
| Micro.flatMap((revokeResponse) => | ||
| Micro.promise(() => storageClient.remove()).pipe( | ||
| Micro.map((deleteResponse) => ({ | ||
| isError: !!(revokeResponse && 'error' in revokeResponse), |
There was a problem hiding this comment.
I think I would personally prefer just using error to keep it consistent. It could contain a value of "Inner request error" or, even more, have an addition of errorKey with a value of the request that contains the error?
So, I'd suggest something like this:
{
error: "Inner request error",
errorKey: "revokeResponse",
revokeResponse: { /** inner response error **/ },
deleteResponse: null,
}
Thoughts?
ancheetah
force-pushed
the
SDKS-4348-token-revoke
branch
from
45c2862 to
3f7416a
Compare
| let errorKey; | ||
| if (sessionResponse && 'error' in sessionResponse) { | ||
| errorKey = 'sessionResponse'; | ||
| } else if (revokeResponse && 'error' in revokeResponse) { | ||
| errorKey = 'revokeResponse'; | ||
| } else if ( | ||
| deleteResponse && | ||
| typeof deleteResponse === 'object' && | ||
| 'error' in deleteResponse | ||
| ) { | ||
| errorKey = 'deleteResponse'; | ||
| } |
There was a problem hiding this comment.
I know I'm the one who suggested this, but I'm now thinking this errorKey could be misleading. If more than one response has an error, we will only share the first in this key. We could convert this string into an array and iterate over the errors, but at that point, I think we lose the value.
So, I'm thinking we just remove the errorKey, but leave error. Thoughts?
There was a problem hiding this comment.
Yeah let's just remove errorKey
| await page.getByRole('button', { name: 'Next' }).click(); | ||
|
|
||
| await page.waitForURL('http://localhost:8443/ping-am/**'); | ||
| await page.waitForURL('http://localhost:8443/ping-am/**', { waitUntil: 'networkidle' }); |
There was a problem hiding this comment.
I think networkidle is an anti-pattern or discouraged.
There was a problem hiding this comment.
'networkidle' - DISCOURAGED consider operation to be finished when there are no network connections for at least 500 ms. Don't use this method for testing, rely on web assertions to assess readiness instead.
There was a problem hiding this comment.
This true but we use it a lot in our tests rights now. We have a ticket to do a bunch of Playwright updates later on.
https://pingidentity.atlassian.net/browse/SDKS-4331
There was a problem hiding this comment.
If we do use networkidle in more places, let's just note it in that Playwright improvement story and leave it for then. Ultimately, I'd like to create a utility function that does this since "clicking a button and waiting for a response" as it is a super common pattern. That was the purpose of the old util file, and I'd like to keep to that goal.
There was a problem hiding this comment.
Updated the ticket
ancheetah
force-pushed
the
SDKS-4348-token-revoke
branch
from
3f7416a to
c48d9c8
Compare
4 participants
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4348
Description
token.revokemethod in OIDC clientuser.infoendpoint and client methodChangeset added