Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Only return the approved widget capabilities instead of accepting all requested capabilities #7454

Merged
merged 2 commits into from
Jan 6, 2022
Merged

Only return the approved widget capabilities instead of accepting all requested capabilities #7454

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0ff567d
Only return the approved widget capabilities instead of accepting all…
dhenneke Jan 3, 2022
ecf20e7
Restore the original logic that only keeps accepted permissions if th…
dhenneke Jan 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/stores/widgets/StopGapWidgetDriver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import { IContent, IEvent, MatrixEvent } from "matrix-js-sdk/src/models/event";
import { Room } from "matrix-js-sdk/src/models/room";
import { logger } from "matrix-js-sdk/src/logger";

import { iterableDiff, iterableUnion } from "../../utils/iterables";
import { iterableDiff, iterableIntersection } from "../../utils/iterables";
import { MatrixClientPeg } from "../../MatrixClientPeg";
import ActiveRoomObserver from "../../ActiveRoomObserver";
import Modal from "../../Modal";
Expand Down Expand Up @@ -131,7 +131,9 @@ export class StopGapWidgetDriver extends WidgetDriver {
}
}

const allAllowed = new Set(iterableUnion(allowedSoFar, requested));
Copy link
Member

Choose a reason for hiding this comment

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

what was the exact issue with the union? If the widget is approved for [A, B, C, D] and the widget requested [B, C, E], then the widget should only receive [B, C] (the common permissions between the two). It should not be approved for things it didn't request, and obviously not for things it wasn't approved for in the first place.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, we named it union when it was doing a merge? That's backwards...

Copy link
Member

Choose a reason for hiding this comment

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

We should restore this as an intersection, I guess.

Copy link
Contributor Author

@dhenneke dhenneke Jan 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

what was the exact issue with the union? If the widget is approved for [A, B, C, D] and the widget requested [B, C, E], then the widget should only receive [B, C] (the common permissions between the two). It should not be approved for things it didn't request, and obviously not for things it wasn't approved for in the first place.

But when the user is asked and approves E, it should be included in the list. The original issue was that this function not only just returned [B, C], but it also stored [B, C] as approved capabilities in the local storage, practically forgetting the prior approved [A, D] and requesting these permissions on every reload. When I understand matrix-org/matrix-spec-proposals#2974 correctly, the re-exchange should be additive and it should indeed respond with [A, B, C, D, E] (iff the user approved E).

Oh, we named it union when it was doing a merge? That's backwards...

I'm not sure what you mean by "doing a merge". For me that sounds suspiciously like a union of two sets 🤔. Which would make sense to be stored in the local storage (of course not with the requested but only with the approved ones). But given the "to send over the total list of requested and approved capabilities throughout the session" from the MSC, it shouldn't be necessary to calculate an intersection between the requested and approved sets(?).

We also might want to increase the test coverage of this code in the future, given its sensitivity (even though the widget API is still in a quite early state).

Copy link
Member

Choose a reason for hiding this comment

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

If the widget didn't ask for E then it categorically should not be approved to use that capability. Whatever the user last stored should then be persisted as approved, as they've implicitly denied the capabilities not requested.

MSC2974 is additive, though the function here is not. It's a somewhat known limitation that requires breaking the API surface of the widget-api, not fixing here. If the widget never requested A in the widget session though, it shouldn't be approved to use it.

tldr: it's supposed to be an intersection, even if that introduces/maintains bugs at the moment.

Copy link
Member

Choose a reason for hiding this comment

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

I think in practice we'd send not only requestedCapabilities but also approvedCapabilities so the driver knows what to do with the stuff it already approved, and possibly inform the user's consent a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you talk about the "API surface of the widget-api", you mean this interface? So the "Element"/"Host" surfaced part of the widget API and not the widget surfaced part.

Copy link
Member

Choose a reason for hiding this comment

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

Yup! That bit would need changing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the PR to revert to the old logic. Is there a ticket that tracks the progress of the planned API change in the widget API? It would be really nice to be able to use the capabilities renegotiation feature in a widget.

Copy link
Member

Choose a reason for hiding this comment

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

I've opened matrix-org/matrix-widget-api#52 for now to track it.

// discard all previously allowed capabilities if they are not requested
// TODO: this results in an unexpected behavior when this function is called during the capabilities renegotiation of MSC2974 that will be resolved later.
const allAllowed = new Set(iterableIntersection(allowedSoFar, requested));

if (rememberApproved) {
setRememberedCapabilitiesForWidget(this.forWidget, Array.from(allAllowed));
Expand Down