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: send correct connection scopes for client #20312

Merged
merged 2 commits into from
Mar 27, 2024

Conversation

znewton
Copy link
Contributor

@znewton znewton commented Mar 25, 2024

Description

When a client joins with AzureClient, it will always try to join in write even if it only has read permissions according to its token claims. On the server (Nexus Lambda), we handle that correctly by creating a read-scoped connection. However, the server would incorrectly handle client details and send connection info like this:

socket.emit("connect_document_success", { /* ... */, mode: "read");
// ...
socket.emitToRoom(
    "tenantId/documentId",
    "signal",
    { 
       /* ... join signal message ...*/,
       client: { /* ... */, mode: "write"
    });

I'm not sure about the specifics of what's happening on the client, but when testing read client Audience and Signals usage in #20042, I realized that the read client audience is not working appropriately without this change. My understanding is it's because Audience is trying to use Quorum (Join Ops) instead of Join Signals because the Join Signal client details said { mode: "write" } even though Connection details said { mode: "read" }

@github-actions github-actions bot added area: server Server related issues (routerlicious) base: main PRs targeted against main branch labels Mar 25, 2024
@znewton znewton merged commit e227db9 into microsoft:main Mar 27, 2024
30 checks passed
@znewton znewton deleted the server/correct-connection-scopes branch March 27, 2024 21:39
znewton added a commit to znewton/FluidFramework that referenced this pull request May 16, 2024
## Description

When a client joins with AzureClient, it will always try to join in
`write` even if it only has `read` permissions according to its token
claims. On the server (Nexus Lambda), we handle that correctly by
creating a read-scoped connection. However, the server would incorrectly
handle client details and send connection info like this:

```js
socket.emit("connect_document_success", { /* ... */, mode: "read");
// ...
socket.emitToRoom(
    "tenantId/documentId",
    "signal",
    { 
       /* ... join signal message ...*/,
       client: { /* ... */, mode: "write"
    });
```

I'm not sure about the specifics of what's happening on the client, but
when testing read client Audience and Signals usage in microsoft#20042, I
realized that the read client audience is not working appropriately
without this change. My understanding is it's because Audience is trying
to use Quorum (Join Ops) instead of Join Signals because the Join Signal
client details said `{ mode: "write" }` even though Connection details
said `{ mode: "read" }`
znewton added a commit that referenced this pull request May 17, 2024
## Description

T9s v4 doesn't have the Quorum scrubbed users fix, and is also missing
the fix for correct Read/Write scopes on socket connection.
Cherry-picking some commits:

---

### server: cover edge cases for scrubbed checkpoint users
(#20259)

6718a9a

---

### server: provided user was not an azure user fixes (#20150)

786abfa

---

### server: fix mutable quorum snapshot (#20329)

5de9472

---

### fix: send correct connection scopes for client (#20312)

7d07fc5
znewton added a commit that referenced this pull request Jul 17, 2024
## Description

#20914 disabled a few Azure E2E tests because they were timing out in
T9s due to a T9s bug that was fixed in #20312. That bug has been patched
into T9s v4 and was also released in T9s v5. We can enable those tests
now.

Validated locally that these pass by running `pnpm test`
RishhiB pushed a commit to RishhiB/FluidFramework-1 that referenced this pull request Jul 18, 2024
## Description

microsoft#20914 disabled a few Azure E2E tests because they were timing out in
T9s due to a T9s bug that was fixed in microsoft#20312. That bug has been patched
into T9s v4 and was also released in T9s v5. We can enable those tests
now.

Validated locally that these pass by running `pnpm test`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: server Server related issues (routerlicious) base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants