fix: make isAuthenticated validate delegation expiry #985
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where isAuthenticated did not validate delegation expiry and updates its signature to synchronous, simplifying its usage.
- Updated isAuthenticated to include a delegation chain expiry check and made it synchronous.
- Adjusted unit tests to remove unnecessary async handling and add an assertion for delegation expiry.
- Reformatted export statements in index.ts for consistency.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/auth-client/src/index.ts | Made isAuthenticated synchronous with an added delegation expiry check and reformatted exports. |
| packages/auth-client/src/index.test.ts | Updated tests to reflect the synchronous isAuthenticated method and added an assertion for failed delegation validation. |
asabates-dfinity
force-pushed
the
asabates/extended-is-authorized
branch
from
72feeb2 to
37704af
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue with the isAuthenticated method not validating expired delegation chains and updates its signature from asynchronous to synchronous. It also updates the CI workflows to use the latest Ubuntu image and adjusts test usage accordingly.
- Fix isAuthenticated to validate delegation expiry
- Update tests to remove unnecessary async/await usage on isAuthenticated
- Update workflow configurations to use ubuntu-latest
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/mitm.yml | Updated OS version from ubuntu-20.04 to ubuntu-latest |
| packages/auth-client/src/index.test.ts | Removed await for isAuthenticated and added additional checks in tests |
| .github/workflows/unit-tests.yml | Updated OS version in CI workflow |
| .github/workflows/e2e-tests.yml | Updated OS version in CI workflow |
| packages/use-auth-client/src/use-auth-client.ts | Simplified then block to match the synchronous isAuthenticated signature |
| packages/auth-client/src/index.ts | Changed isAuthenticated from asynchronous to synchronous with added delegation expiry check |
Comments suppressed due to low confidence (3)
.github/workflows/mitm.yml:18
- Switching from a fixed Ubuntu version to ubuntu-latest can introduce environmental changes; please verify that all tests continue to pass reliably under the updated CI environment.
os: [ubuntu-latest]
packages/auth-client/src/index.test.ts:55
- Updating the test to remove the await is consistent with the synchronous nature of isAuthenticated; ensure that similar changes are made in all affected tests.
expect(test.isAuthenticated()).toBe(false);
packages/auth-client/src/index.ts:437
- Changing the signature of isAuthenticated to synchronous impacts its usage—please update the documentation and ensure that consumers remove any awaiting of its result.
public isAuthenticated(): boolean {
47a837a to
8e08e59
Compare
packages/auth-client/src/index.ts
Outdated
| @@ -428,8 +434,12 @@ export class AuthClient { | |||
| return this._identity; | |||
| } | |||
|
|
|||
| public async isAuthenticated(): Promise<boolean> { | |||
| return !this.getIdentity().getPrincipal().isAnonymous() && this._chain !== null; | |||
| public isAuthenticated(): boolean { | |||
There was a problem hiding this comment.
I prefer to leave this asynchronous. If we decide to add support for validating the signatures for all delegations in the chain (currently haven't adopted this yet because we wanted to avoid the secp256k1 dependency), this operation will most likely have async behavior.
Breaking the API is costly, and this is why we tagged it async preemptively, even years in advance
There was a problem hiding this comment.
Makes sense, reverted here fb339a1
| setAuthClient(client); | ||
| setIdentity(client.getIdentity()); | ||
| setIsAuthenticated(await client.isAuthenticated()); |
There was a problem hiding this comment.
see here also, I'd like this reverted
docs/CHANGELOG.md
Outdated
There was a problem hiding this comment.
This is a large formatting diff, presumably from autoformatting. Which tool did you use for this?
There was a problem hiding this comment.
Fixed here fb339a1, I was just using VSCode with prettier and eslint :)
Description
Currently
isAuthenticateddoes not work if the delegation has expired, as it does not check at any moment further than the chain has being unset, which only happens when actively logout call is triggered. This produces the scenario in which a consumer can call toisAuthenticated, get a true result and yet their agents are rejected because of the expired authorization.This PR aims to fix this problem by adding a check validating the delegation chain, it also adjusts the signature of the method to non asynchronous as none of the operations done inside are, indeed, asynchronous. This simplifies the usage for the consumer.
How Has This Been Tested?
Updated unit tests to check that it returns false when its expired
Checklist: