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

OIDC: Token refresher class #3769

Merged
merged 16 commits into from
Oct 9, 2023
Merged

Conversation

kerryarchibald
Copy link
Contributor

@kerryarchibald kerryarchibald commented Oct 2, 2023

For element-hq/element-web#25839
See js-sdk #3764 and matrix-org/matrix-react-sdk#11699 for use.

Adds an abstract class responsible for refreshing OIDC native tokens.
Exposes abstract function persistTokens to allow consumer to persist tokens in storage.
Will be used in matrix-react-sdk to create tokenRefreshFunction as passed to http-api

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

This change is marked as an internal change (Task), so will not be included in the changelog.

@kerryarchibald kerryarchibald added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Oct 2, 2023
@kerryarchibald kerryarchibald mentioned this pull request Oct 3, 2023
3 tasks
src/oidc/tokenRefresher.ts Outdated Show resolved Hide resolved
src/http-api/interface.ts Outdated Show resolved Hide resolved
src/oidc/tokenRefresher.ts Show resolved Hide resolved
src/oidc/tokenRefresher.ts Show resolved Hide resolved
src/oidc/tokenRefresher.ts Show resolved Hide resolved
spec/unit/oidc/tokenRefresher.spec.ts Outdated Show resolved Hide resolved
spec/unit/oidc/tokenRefresher.spec.ts Show resolved Hide resolved
spec/unit/oidc/tokenRefresher.spec.ts Outdated Show resolved Hide resolved
spec/unit/oidc/tokenRefresher.spec.ts Show resolved Hide resolved
spec/unit/oidc/tokenRefresher.spec.ts Show resolved Hide resolved
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
src/oidc/tokenRefresher.ts Outdated Show resolved Hide resolved
src/oidc/tokenRefresher.ts Outdated Show resolved Hide resolved
src/http-api/interface.ts Outdated Show resolved Hide resolved
src/http-api/interface.ts Outdated Show resolved Hide resolved
Comment on lines 21 to 31
/**
* @experimental
*/
export type AccessTokens = {
accessToken: string;
refreshToken?: string;
};
/**
* @experimental
*/
export type TokenRefreshFunction = (refreshToken: string) => Promise<AccessTokens>;
Copy link
Member

Choose a reason for hiding this comment

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

please document these things

src/http-api/interface.ts Show resolved Hide resolved
spec/unit/oidc/tokenRefresher.spec.ts Show resolved Hide resolved
Comment on lines 28 to 41
const authConfig = {
issuer: "https://issuer.org/",
};
const clientId = "test-client-id";
const redirectUri = "https://test.org";
const deviceId = "abc123";
const idTokenClaims = {
exp: Date.now() / 1000 + 100000,
aud: clientId,
iss: authConfig.issuer,
sub: "123",
iat: 123,
};
const scope = `openid urn:matrix:org.matrix.msc2967.client:api:* urn:matrix:org.matrix.msc2967.client:device:${deviceId}`;
Copy link
Member

Choose a reason for hiding this comment

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

could do with a few comments on some of the other things here too please!

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
@kerryarchibald
Copy link
Contributor Author

@richvdh if we have agreed guidelines about active voice, tone, etc; they should be documented. I don't think several rounds of these reviews are a good use of either of our time.

@richvdh
Copy link
Member

richvdh commented Oct 5, 2023

if we have agreed guidelines about active voice, tone, etc; they should be documented. I don't think several rounds of these reviews are a good use of either of our time.

I'm afraid I just see it as writing clear, factual documentation - something we've agreed at a team that we need to do better at.

@richvdh
Copy link
Member

richvdh commented Oct 6, 2023

@kerryarchibald I've sent you a longer response privately, but for the public record: please can you document the AccessTokens and TokenRefreshFunction types so that we can get this landed.

@kerryarchibald
Copy link
Contributor Author

@richvdh have added documentation for AccessTokens and TokenRefreshFunction

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

Thank you!

@kerryarchibald kerryarchibald added this pull request to the merge queue Oct 9, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 9, 2023
@kerryarchibald kerryarchibald added this pull request to the merge queue Oct 9, 2023
Merged via the queue into develop with commit 3139f57 Oct 9, 2023
21 checks passed
@kerryarchibald kerryarchibald deleted the kerry/25839/token-refresher-class branch October 9, 2023 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants