Closed pnc closed 1 year ago
Howdy!
Sorry on the late response. Let's start breaking this down.
My big concern with this back-compat/versioning strategy is that it requires that every call have a version parameter, and that every caller gets every version parameter right. Contrast that w Android's minSDK and targetSDK handling of API level. In the API level logic, the version a caller wants to interact with is specified just in minSDK and targetSDK, at the app level, with no risk of individual calls being made at incorrect or invalid versions.
Basically, I think this can all be simplified by pushing versioning up from the call level to the interface or app level. That simplicity can remove a lot of opportunities for error.
Second, I'm a bit fuzzy on the above diagram, because it conflates Eleos and Zonar Shell in the left-hand column. I think I see where you're going. It'd be clearer if the relationship between Eleos, Shell, and ZLogs - workflow manager, identity provider, and HOS provider - were fully laid out. Unless your intent is for Eleos to be the identity provider?
My other versioning concern is with distribution methods. OpenCab implementers are expected to copy the sample code into their products, then rewrite it. There's typo risk in that. It's not picked up as an AAR or some such. There's chunks of each interface that don't need to be altered by the implementer. Can those be distributed in a form that doesn't have a risk of copy/paste error?
OK, enough problems. On to possible solutions. Let's take Android API level versioning as a possible path. Suppose there's another intent in the picture, like so:
Objective | Identity Consumer | Call | Identity Provider |
---|---|---|---|
Declare desired version | Eleos | ->OpenCab.IdentityContract.SetInterface(minVersion, targetVersion) | Shell |
Return supported version if any | Eleos | <-Return Version(supportedVersion) (0.0 if no acceptable version available) | Shell |
After this sequence, Shell has committed to respond to IdentityContract intents at the version it returned in supportedVersion until IdentityContract.SetInterface is called again. I'm not entirely crazy about this, because of the risks created if there's two consumers with significantly different version expectations on the same device. Still, it's the beginning of an answer, factoring the consumer-provider version handshake out from the data operations.
I don't want to delay response further seeking a perfect answer. FYI, that's what I see at this point. Thoughts?
Some of the feedback from earlier discussions:
Think that was all the points we discussed, but if you remember anything else, let us know @pnc .
@ZonarTimH @colonelchlorine Thanks for the detailed feedback! I'll incorporate these into the draft and/or respond.
@ZonarTimH @rianhouston @VCEleosTech @colonelchlorine
Okay, I believe we've now collected and incorporated the feedback above.
I get the desire to have versioning be a bit more implicit, but I feel pretty strongly that (since either app could get killed, even in the middle of a "connection", since no real persistent connection really exists) each individual call needs to be stateless, and thus it needs to convey all the necessary versioning information. (With automatic app updates, one app could very much be updated while a driver is logged in and I don't want to go try to debug issues that crop up because the collaborating app thinks the upgraded one is still speaking an older version.)
I've updated the interaction table based on Mike's great feedback about returning all drivers' sessions and/or clocks in a single call. Is this logical to you? Spot any mistakes or warts we might regret?
Identity Provider App | ELD App (Identity Consumer, HOS Provider) | |
---|---|---|
Driver A logs in. | ||
Broadcast: IdentityContract.ACTION_DRIVER_LOGIN | ||
App enumerates available Identity providers and selects Eleos App. | ||
App queries provider for list of active drivers. | ||
<- Call IdentityContract.METHOD_GET_ACTIVE_DRIVERS("0.2") | ||
-> Return [Driver{username="A"}] | ||
App currently has no logged in user, and notices that it now needs to have user A logged in to match the identity provider state. | ||
The app makes a call to the Identity provider and signals that it understands version 0.3 of the contract and that it wants session credentials for all active drivers. This is a separate call from METHOD_GET_ACTIVE_DRIVERS since it may be expensive for a provider to fetch or generate credentials, and it should only be called when needed, not simply to monitor the list of active drivers. | ||
<- Call IdentityContract.METHOD_GET_LOGIN_CREDENTIALS("0.3") | ||
The ELD app sees the "0.3" version and knows the calling application supports team drivers via OpenCab. However, the response value is backwards-compatible, so the provider application MAY ignore the version if it does not need to change its own behavior when team driving is available. If the version is "0.2" or lower, the app SHOULD assume the calling application does not support team drivers via OpenCab. | ||
-> Return KEY_VERSION="0.3" KEY_ALL_LOGIN_CREDENTIALS=[ IdentityContract.DriverSession{ username="A", loginCredentials=LoginCredentials{ authority="example" provider="com.eleostech.example" token="kf40m1fpl…d28zckhuf6" } } ] # Backwards-compatible single driver credentials KEY_LOGIN_CREDENTIALS=LoginCredentials{ authority="example" provider="com.eleostech.example" token="kf40m1fpl…d28zckhuf6" | Because the response contains a KEY_VERSION equal to the version supported by the caller, the app knows that the provider app supports team driving (added in version 0.3) and that it can look for KEY_ALL_LOGIN_CREDENTIALS in the response. If the KEY_VERSION were strictly less than the version supported by the caller, with absence indicating version 0.2 support only, then the app would know that the identity provider app does not support team driving and MAY provide an alternate UI for managing team drivers. If KEY_VERSION is strictly greater than the supported version, the provider has chosen not to implement backwards compatibility and the response MUST be treated as an error, since it cannot be safely interpreted by this older app. | |
App adjusts session state to be have driver A, and no other team drivers, logged in. If another driver was previously logged in, they are logged out. | ||
App enumerates available HOS providers and selects ELD App. | ||
App queries HOS provider for current HOS status. | ||
-> Call HOSContract.METHOD_GET_HOS("0.2") | ||
App returns current HOS status for the primary driver using the existing KEY_HOS mechanism. Because the provider app supports OpenCab-based team driving, it includes an empty KEY_TEAM_HOS value, indicating there are no additional team drivers. | ||
<- Return Bundle{ KEY_HOS=HOSStatus{clocks=[…]} KEY_TEAM_HOS=[] } | ||
App displays Driver A's clocks. The calling app MAY display an option to log in a second driver. (Allowing the ELD provider to permit or deny whether it's legal to add an additional team driver is out of scope for OpenCab currently.) | ||
Driver B taps the option to log in, enters their credentials for the Eleos App (not credentials for ELD App). | ||
Broadcast: IdentityContract.ACTION_DRIVER_LOGIN | ||
App queries provider for list of active drivers. | ||
<- Call IdentityContract.METHOD_GET_ACTIVE_DRIVERS("0.2") | ||
-> Return [Driver{username="A"}, Driver{username="B"}] | ||
App currently has only driver A logged in, so it knows that it now needs to have user B logged in as well to match the identity provider state. | ||
The app makes a call to the Identity provider and signals that it understands version 0.3 of the contract and that it wants session credentials for all active drivers. This is a separate call from METHOD_GET_ACTIVE_DRIVERS since it may be expensive for a provider to fetch or generate credentials, and it should only be called when needed, not simply to monitor the list of active drivers. | ||
-> Return KEY_VERSION="0.3" KEY_ALL_LOGIN_CREDENTIALS=[ IdentityContract.DriverSession{ username="A", loginCredentials=LoginCredentials{ authority="example" provider="com.eleostech.example" token="kf40m1fpl…d28zckhuf6" } }, IdentityContract.DriverSession{ username="B", loginCredentials=LoginCredentials{ authority="example" provider="com.eleostech.example" token="p0LLdm3Ma…KEAd8vMN12d" } } ] # Backwards-compatible single driver credentials KEY_LOGIN_CREDENTIALS=LoginCredentials{ authority="example" provider="com.eleostech.example" token="kf40m1fpl…d28zckhuf6" | ||
App adjusts session state to be have driver A and B both logged in. | ||
App queries HOS provider for current HOS status. | ||
-> Call HOSContract.METHOD_GET_HOS("0.2") | ||
App returns current HOS status for the primary driver using the existing KEY_HOS mechanism, and clocks for the additional driver (driver B) in the KEY_TEAM_HOS array. | ||
<- Return Bundle{ KEY_HOS=HOSStatus{clocks=[…driver A's clocks…]} KEY_TEAM_HOS=[HOSStatus{clocks=[…driver B's clocks…]}] } | ||
The app displays duty status and clocks for both driver A and driver B based on the returned information. Normally, the consuming app SHOULD display both sets of clocks with the clocks from KEY_HOS displayed first. |
This looks good to me.
Having all credentials returned in METHOD_GET_ACTIVE_DRIVERS makes the flow a lot simpler.
Is there going to be changes to the clocks object to identify the driver? Because METHOD_GET_HOS KEY_TEAM_HOS returns and array of clocks, do we need to identify the driver?
@markjohnstongeotab I'd love to add a username
to them. We're fine to do that from a PII standpoint, right?
@pnc Actually username is considered PII, so it would be much better for us to provide userId
@pnc We are planning for the team driving change to METHOD_GET_HOS
. Has it been decided exactly how userId
will be included? Based on the interaction diagram it looks like each driver gets their own HOSStatus
object which contains their Clock
s. Will each HOSStatus
have a userId
field? It would be more concise this way, but I can see how including userId
in each Clock
object may be useful for the HOSConsumer in some scenarios. It would be great to have this cleared up.
I also want to confirm that the KEY_TEAM_HOS
value should be an array of HOSStatus
(one per codriver) and not a Map keyed by userid
.
@BoKlassen Oh, thanks for raising. I tend to think the only meaningful value to put on each clock would be the username/ID that corresponds to the username/ID that the identity provider conveyed. (E.g., exact spelling and case, so the HOS consumer would be able to make the association.)
Currently, Geotab's own SSO token contains the username and not the userId:
{"path":"my.geotab.com","credentials":{"userName":"GEOTABOC6TD-PC","sessionId":"quux","database":"eleos_test"}}
So we'll return this as the token
for
KEY_ALL_LOGIN_CREDENTIALS=[
IdentityContract.DriverSession{
username="GEOTABOC6TD-PC",
loginCredentials=LoginCredentials{
authority="com.geotab"
provider="com.eleostech.example"
token="{\"path\":\"my.geotab.com\",\"credentials\":{\"userName\":\"GEOTABOC6TD-PC\",\"sessionId\":\"quux\",\"database\":\"eleos_test\"}}"
}
}
] # Backwards-compatible single driver credentials
KEY_LOGIN_CREDENTIALS=LoginCredentials{
authority="com.geotab"
provider="com.eleostech.example"
token="{\"path\":\"my.geotab.com\",\"credentials\":{\"userName\":\"GEOTABOC6TD-PC\",\"sessionId\":\"quux\",\"database\":\"eleos_test\"}}"
If we add a user identifier to each HOSStatus object, it needs to match that same username
property:
Bundle{
KEY_HOS=HOSStatus{
username="GEOTABOC6TD-PC"
clocks=[…driver A's clocks…]
}
KEY_TEAM_HOS=[
HOSStatus{clocks=[…driver B's clocks…]}
]
}
Complicating this a bit is that we don't require customers to use the same username for actual driver login as they do in Geotab. (There are a variety of good reasons they might do this.) In that case, the interaction might look like this:
KEY_ALL_LOGIN_CREDENTIALS=[
IdentityContract.DriverSession{
username="geotaboc6@example.com",
loginCredentials=LoginCredentials{
authority="com.geotab"
provider="com.eleostech.example"
token="{\"path\":\"my.geotab.com\",\"credentials\":{\"userName\":\"GEOTABOC6TD-PC\",\"sessionId\":\"quux\",\"database\":\"eleos_test\"}}"
}
}
] # Backwards-compatible single driver credentials
KEY_LOGIN_CREDENTIALS=LoginCredentials{
authority="com.geotab"
provider="com.eleostech.example"
token="{\"path\":\"my.geotab.com\",\"credentials\":{\"userName\":\"GEOTABOC6TD-PC\",\"sessionId\":\"quux\",\"database\":\"eleos_test\"}}"
and Geotab, as the Identity consumer, would need to manage the mapping on its side, so that it returns:
Bundle{
KEY_HOS=HOSStatus{
username="geotaboc6@example.com"
clocks=[…driver A's clocks…]
}
KEY_TEAM_HOS=[
HOSStatus{clocks=[…driver B's clocks…]}
]
}
I don't see any value in the Geotab implementation returning:
Bundle{
KEY_HOS=HOSStatus{
username="b812"
clocks=[…driver A's clocks…]
}
KEY_TEAM_HOS=[
HOSStatus{clocks=[…driver B's clocks…]}
]
}
because b812
has no context to another OpenCab app.
We don't actually need the username in a structured/code logic-accessible way (although you are free to include it as a string-type clock for the driver's benefit when they have a teammate to reduce confusion), so I think we can all sidestep this complexity for now if you agree.
@pnc I'm curious how the spec "[doesn't] actually need the username in a structured/code logic-accessible way". I was under the assumption that:
KEY_TEAM_HOS
(hence it is an array), ANDClock
s belong to which driver.Are these assumptions correct? If so, how would the HosConsumer know which Clock
s belong to which drivers, if the username is not in HosStatus
or the Clock
s?
@BoKlassen For our particular app, we need to show all the clocks, and we need to do some logic based on the primary drivers' clocks (as defined by the ones in KEY_HOS), but we don't ever actually need to know which clocks go with which driver. I can imagine that other consumers might want to do so, though.
That said, we do expect Geotab to return string
type clocks that contain the username, as a means of signaling whose clocks are whose to the driver. However, we won't use those programmatically. (That gives you the flexibility to show or not show those usernames depending on whether there's more than one driver logged in or not.)
@pnc In an earlier comment(https://github.com/opencabstandard/opencab/issues/17#issuecomment-1564745525), mentioned about how customers use different usernames for driver login. Could you please confirm if IdentityContract.METHOD_GET_ACTIVE_DRIVERS("0.2") would return Geotab username(driver login)?
@hsudhagar the Eleos platform app responds to IdentityContract.METHOD_GET_ACTIVE_DRIVERS with the Eleos username which the fleet defines and passes to us. Geotab can extract the Geotab username from the token value which Eleos returns when Geotab Drive calls METHOD_GET_LOGIN_CREDENTIALS
We've identified a need to support one or more team drivers in addition to the primary active driver.
The
METHOD_GET_ACTIVE_DRIVERS
call onIdentityContract
already supports this notion, but the ability to to retrieve session credentials or clocks for additional drivers is undefined.This RFC attempts to address three key things not yet defined in the spec:
METHOD_GET_LOGIN_CREDENTIALS
is only for a single user, and there is no (defined) way for it to convey session credentials for a second (or third) user.METHOD_GET_HOS
, but I'd like to start a discussion about whether it's valuable (and how we might) model an HOS status for each driver.Below is an interaction diagram that proposes a way to address all three needs. In particular, it tries to accomplish them in a way that preserves backwards compatibility for existing deployments by giving both providers and consumers a way to signal the maximum version they support, and an indication of whether the collaborating app also supports that version.
I'm eager to get feedback on:
Thanks for any feedback you can provide!
username
argument and return credentials for that username. If the version is "0.2" or lower, the app SHOULD assume the calling application does not support team drivers via OpenCab.username
argument and return clocks only for that username. If the version is "0.2" or lower, the app MAY implement a compatibility mode and return a list of clocks relevant to all drivers or teammates.