Agent agnostic fdc3 spec #1167
Agent agnostic fdc3 spec #1167
Conversation
✅ Deploy Preview for fdc3 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
This is a big step forward! Lots of comments from me here to think about. A couple of final thoughts:
- Test coverage. We need to make sure everything is tested thoroughly. (I have this for the POC proxy implementation already so I guess we're nearly there)
- I don't see us merging this until we've completed the implementation lines up. This is really important: it's all very well us agreeing a spec, but there's no point agreeing on something if it hasn't been thoroughly tested and shown to work in the wild.
| * @typedef {Object} GetAgentParams Type representing parameters passed to the | ||
| * getAgent function. | ||
| * | ||
| * @property {string} appId The fully qualified appId that represents the application. |
There was a problem hiding this comment.
I don't think this should be here. It's the App Directory's responsibility to describe apps and their appIds. I'm also not convinced by the argument that this is a security feature - we should defer to the other WG for that.
There was a problem hiding this comment.
This 100% comes from the discussions in the WG @robmoffat - its a specific solution to the issue of navigation/refresh as well as disambiguating apps on the same domain. There's a detailed explanation in the group's working doc.
There was a problem hiding this comment.
In the spec it says:
/** The instanceUuid that was issued to the app. This should be
* passed when connecting to the Desktop Agent to help
* identify that this app has connected before and which
* instance it is, enabling the Desktop Agent to reissue
* the same instanceId. The instanceUuid should never be shared
* with other applications and is not available through the
* FDC3 API, allowing it to be used as a shared secret with
* the Desktop Agent that issued the associated instanceId.*/
*/
Is this what you are referring to?
So this is the flow for the requirement here:
- DA opens App A window
- App A "talks back" to DA, and asks for a URL for an iframe
- DA checks client's domain name and window, and responds with a URL for App A.
- App A then opens the iframe...
Problem is:
- User then navigates the same window to App B on the same domain.
- App B talks back to the DA, asks for a URL for an iframe.
- DA checks client's domain name and window and responds with a URL for App B.
- App B is now running as App A (as far as the DA is concerned)
Issue only occurs when more than one app is running on the same domain
Is that correct?
There was a problem hiding this comment.
Yes it happens on same domain window navigations (we work with many firms that publish many apps, both in-house and clients and often use a common domain), but also where firms use certain types of proxy that replace domains and SSL certificates for external sites.
The latter seems a weird practice to me, but we've seen it at a few firms. The former is more often going to be the issue as its common for those apps to be able to navigate to each other with a built-in nav menu.
Further, this is a key part of apps self identifying - it's the link back to the app directory that contains their identity (which as discussed may not be the one they were launched with - but its fundamental that the app code needs to contain a link to the identity, rather than a 3rd party directory). This has been discussed at length in the meetings on several occasions.
There was a problem hiding this comment.
Resolving as we have to have self identification of apps to resolve appIds wherever there are multiple apps running on the same domain AND to confirm that they did not change identity on navigation (they could navigate to another related app or a user could simply type in a different address of path in the address bar)
There was a problem hiding this comment.
I've been thinking about this a lot in the intervening time.
Perhaps we need to "bite the bullet" here and add the app's public key to the directory? Then, the DA can issue a challenge ("hey, encrypt this token with your private key") and the app can respond with something the DA can check.
This is how symphony works, pretty much.
There was a problem hiding this comment.
In any case I think we need a better term than instanceUuid. This is too close to instanceId and doesn't really reflect that it's something unique to the app rather than the app instance.
There was a problem hiding this comment.
In any case I think we need a better term than instanceUuid. This is too close to instanceId and doesn't really reflect that it's something unique to the app rather than the app instance.
Re: instanceUuid, its not used for app identity but instance identity. It should be unique for each instanceId. It's a shared secret generated by the DA when it sees a new instance to allow that instance to get the same instanceId again and should only be known to the DA and that particular instance (only that instance can access the SessionStorage we put it into - different windows for the same app get separate SessionStorage).
It's needed as, if you have two instances of the same app, and they navigate or refresh it's hard for the DA to tell which is which. As per the proposal doc there are edge cases to comparing the window proxy objects (if the app navigate the window proxy may still be the same), there also edges in the instanceUuid approach (window.open to same domain can clone the SessionStorage) so the validation procedure for instance identity is to compare both the instanceUuid and the WindowProxy reference in the postMessage to the existing one you have, both having to match for it to be issued the same instanceId. We shouldn't use the instanceId itself for this as that's not a secret/available to other apps through the API in various ways.
Re: Biting the bullet and adding a public key to the appD - absolutely! However, as per your previous comment we didn't want to step on that group's toes and stated we'd adopt whatever identity + SWG adopts for FDC3. Pretty sure that'll be a key part of it so they could always be raising that issue and PR ASAP. I was expecting that something for both topics would make it in to FDC3 2.2 and hence we'd adjust here as soon as we/they had something there ;-)
| * @property {boolean} channelSelector Flag indicating that the application | ||
| * requires getAgent() to create a channel selector UI. Defaults to true. | ||
| * | ||
| * @property {boolean} intentResolver Flag indicating that the application |
There was a problem hiding this comment.
Perhaps see my POC which allows a pluggable intent resolver: https://github.com/finos-labs/fdc3-for-the-web/blob/8d26526a9dfe854c142221741a1be980d78fb985/packages/fdc3-common/src/index.ts#L8
| timeout ?: number, // Defaults to 750 | ||
| appId ?: string, | ||
| appDUrl ?: string, | ||
| failover ?: (args: GetAgentParams) => Promise<WindowProxy | URL | DesktopAgent> |
There was a problem hiding this comment.
Prefer the idea of pluggable strategies here: https://github.com/finos-labs/fdc3-for-the-web/blob/8d26526a9dfe854c142221741a1be980d78fb985/packages/fdc3-common/src/index.ts#L11
... rather than failover, which only allows a single one. Advantage of strategies is that we can introduce new ones over time, or allow the user to select the ones they want.
Also, while I think of it, we should consider in the group the idea of a Cookie strategy, which uses details from a cookie - not thought this through to a huge extent yet but it would be good if you were reloading an app window with a server-side DA. (One for Derek to think about)
There was a problem hiding this comment.
Rob are you forgetting to discuss these things with the working group - this is all based off what we've discussed in the group and documented. Pluggable strategies is not something you've ever brought up.
There was a problem hiding this comment.
I'm not forgetting - it's just very difficult to bring these up in the WG. Normally I have about 5 minutes to talk at the end of a meeting and I've got other things I want to put across.
There was a problem hiding this comment.
Well you know how to get an item on the agenda. If you want to discuss it let me know.
There was a problem hiding this comment.
Having reviewed your loader approach I still prefer what we're doing here and agreed in the discussion group - although the approaches are similar the difference appears to be that you are breaking out the connection strategies so that the app developer has to pass them in, where we designed this function to attempt the built-in strategies and to expose the failover to allow other approaches to be added for advanced cases (like using an adaptor to connect to a container, or to start a DA), or to prepare the app to run without a DA. That allows for a simpler usage for most apps (which won't provide a failover) or to simplify the implementation of one (e.g. just return a URL).
What you've implemented makes a lot of sense internally, but I think the optional failover function simpler for app developers to understand and implement (although theres not a huge difference).
There was a problem hiding this comment.
Also with the loaders approach its not clear what the sequencing is - we worked through racing the two basic strategies (injected FDC3 vs. a parent window/frame) as they are mutually exclusive and then running the failover afterwards allowing a 'parent' DA to take precedence over an alternative strategy (one of the first things we agreed in the discussion group as a MUST) - it appears you just race those provided (with Promise.any, which is the correct way to do that, here https://github.com/finos-labs/fdc3-for-the-web/blob/4242e9cfb6d0de43cda18af642c8977f629a46de/packages/client/src/index.ts#L33-L34) - which is exactly as planned for the built-in strategies but not the failover, which should come after... Otherwise, a rapidly returning alternative strategy would always override the defaults - and if you're going to do that you don't need the getAgent function at all...
There was a problem hiding this comment.
But like your Loader implementation for use inside the library!
There was a problem hiding this comment.
but on reflection I'll drop the ability to return void from a failover (that I was just adding in) as its not necessary - getAgent will reject its promise with a reason if no agent is found - that should be the way to handle preparing an app for running without an agent
|
|
||
| Success responses (from calls to `validateAppIdentity()` or "WCPValidateAppIdentityResponse" messages) MUST include `ImplementationMetadata` (describing the DA), `AppMetadata` (describing the instance), and `DesktopAgentDetails` record (to be persisted in the next step). | ||
|
|
||
| ### Persist DesktopAgentDetails to SessionStorage (step 3) |
There was a problem hiding this comment.
this doesn't exist in the POC yet. We should add it (and write some tests)
| @@ -0,0 +1,41 @@ | |||
| # Preload DA Specification | |||
There was a problem hiding this comment.
Does this not already exist somewhere?
There was a problem hiding this comment.
Sort of, it's whatever is in the overview document - which is trying to speak to both Application developers and Platform developers (DA developers) at the same time. Consequently it doesn't do a great job of either as they need different content.
If we broke out specifications for DA's like this then I think we could tweak the overview doc to be clearer by splitting into functional goals, specs for using the DA API and specs for implementing it.
|
|
||
| A Preload Desktop Agent MUST provide the FDC3 API via a global accessible as `window.fdc3`. Implementors MAY additionally make the API available through modules, imports, or other means. | ||
|
|
||
| The global `window.fdc3` must only be available after the API is ready to use. To enable applications to avoid using the API before it is ready, implementors MUST provide a global `fdc3Ready` event that is fired when the API is ready for use. Implementations should first check for the existence of the FDC3 API and add a listener for this event if it is not found: |
There was a problem hiding this comment.
In order that apps work both in electron and on the web, we need to be specifying here that apps use getAgent() (or in my view, fdc3Ready() too)
| @@ -0,0 +1,130 @@ | |||
| /** | |||
There was a problem hiding this comment.
Eventually, this and the POC need to agree. I think this needs to happen before this PR gets merged.
There was a problem hiding this comment.
True, although based on my initial read this looks like it agrees with the WG's doc
| @@ -0,0 +1,517 @@ | |||
| /** | |||
There was a problem hiding this comment.
Should be in JSON schema since it's a protocol. Need to merge with existing FDC3 bridging protocol.
There was a problem hiding this comment.
I'll look into this when I get back from vacation. There are a few reasons to go to JSONSchema (particularly if you want to generate code for other languages to do the same thing)
| } | ||
| ``` | ||
|
|
||
| ### Step 2 - Authentication |
There was a problem hiding this comment.
We've assessed that it was needed in the WG meetings, defined a procedure for it that we believe to be secure/deals with some specific edge cases and worked out the interface elements (data that needs to be passed to functions as arguments necessary). THere are multiple bits there that need to be in the standard to enable a secure procedure for this - however implementations can then do more or less based on it.
|
|
||
| A DA may implement its own Channel Selector and Intent Resolver or may utilize the one provided by "@finos/fdc3" via `getAgent()` ("built-in UI"). For instance, a DA may not have the ability to present a channel selector in a window that has been opened with `window.open()`. The built-in UI can be leveraged in this circumstance. Send the `BCPResolveIntent` and `BCPInitializeChannelSelector` messages to invoke the built-in UI. | ||
|
|
||
| ## Disconnects |
There was a problem hiding this comment.
Thats what the first part is saying, perhaps it just needs e.g. before the 'by checking'?
There are a limited number of ways to do this - its possible the MessagePort will also provide something to check - I have a memory that it specifically doesn't provide an event or similar, I don't know why the web standards made that choice, but I believe it was deliberate.
| @@ -0,0 +1,130 @@ | |||
| /** | |||
There was a problem hiding this comment.
True, although based on my initial read this looks like it agrees with the WG's doc
| @@ -0,0 +1,517 @@ | |||
| /** | |||
There was a problem hiding this comment.
I'll look into this when I get back from vacation. There are a few reasons to go to JSONSchema (particularly if you want to generate code for other languages to do the same thing)
|
I'm going to go ahead and merge this into a FINOS repo branch to make it easier for a few us to work on it. That'll reset the comments, which will remain here - we can apply changes through comparison to this PR. |
This PR adds specifications and documentation related to enabling FDC3 for the web. This was based off of the committee's working doc
This assumes that "@finos/fdc3" will implement getAgent(), communication mechanisms, and built-in UI for channel selector and intent resolver.
The protocols were codified as "Web Connection Protocol (WCP)" for negotiating the handshake between the library and desktop agents, and "Browser Communication Protocol (BCP)" which encompasses the complete "wire protocol".
This documentation followed three streams:
fdcobjectgetAgent()- to be used by implementers of the "@finos/fdc3" library, and for reference by desktop agent implementers.Some changes and additions were made from the original working document.
The optional initialization fields for channel selector and intent resolver were eliminated. The DA knows whether it can provide these UI elements and so therefore it should be the DA that decides how to accomplish this. However, there are cases where a DA cannot reasonably provide these UI elements in a browser, therefore this spec mandates that "@finos/fdc3" provide built-in UI capabilities that a DA can optionally enable them.
This spec also covers window disconnects by suggesting use of a well known polling mechanism.
A new folder called "specs" was created to house many of the specifications involved. Previously, there was no true distinction between implementation specs and user specs.