-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[saved objects] Create RFC for referencing user profiles from SOs #134595
Comments
Pinging @elastic/kibana-core (Team:Core) |
I'm quite mixed on this subject. On one side I feel like we can't really avoid introducing the user-ref concept to the SO system, but OTOH I'm not sure to see which value that would exactly bring compared to just having the type owners store the user/profile id in their data, and 'resolve' the references themselves. I also foresee some integrity problems related to import/export just by having this user-ref concept used with SOs. Let me start with the first question: What actual upsides do we foresee by having these 'user-refs' being first-class citizens of the SO apis? For the SO references (the existing
Now, for user relations, given the 'users', 'profiles' or whatever are not in Kibana's owned 'data', I don't think any of those points apply:
So, overall, I think adding these user references as first class citizens of the SO system/APIs only make sense if we're adding additional value to our API consumers by doing so. What exact value are we planning to add here? From the issue's description, the only one I see if from one of the listed alternatives:
That would be a valid reasons to add internal support to user references. However, we need to be very careful when enhancing the Now, do we have any other thing(s) in mind regarding what value we could provide by supporting these user-refs natively from the SO APIs? Because if it's just about performing some API calls to ES to resolve the profiles on behalf of the API consumer, I'm personally not sure this is really worth the complexity. To finish on this part, I'd also add that adding more internal logic to the SOR goes into the very opposite direction of what we're trying to achieve with the 'domain service layer' thinking we started talking about (well, that we agreed on) in the BWC-A RFC. Btw, userReferences: [
{
// stable user entity ID
profileId: 'abc123',
// arbitrary string for classifying the relationship between this user & the SO.
type: 'caseAssignment',
}
] If we were to do this, we would at least need to add an User references and import/exportTo get back to the question of being able to check the consistency of user references when importing, this bring a very significant concern to the table: By adding references to data outside of our system (here, users/profiles) inside our saved objects, we basically block the ability to export from a cluster and import to another one, given the user ids / profiles will very likely not be the same between the two clusters, or at least not without introducing invalid data to the system during an import. The options I see here:
None of them sounds really exciting though... Did @elastic/response-ops and/or @elastic/kibana-security though about this problem? Oh, and before anyone get the idea, 'the SO types we're planning to add user references to are not exportable' is not a valid answer if we're talking about introducing the concept for all saved object types. |
We should review the use cases when we have more information, but at this point I agree with @pgayvallet If we anticipate a lot of user profiling persisting logic duplicated between plugins I would be more inclined to create a user profiles library with some utility functions that can manipulate/transform saved objects but doesn't do any persistence:
This could create some standardisation without forcing all plugins to follow exactly the same model. Having said that, adding |
The user IDs are considered stable, so technically I don't know if the reference extraction/injection process would be mandatory. But would probably be a good idea to do it still, just to consolidate any references to user IDs in one place within an object. Otherwise the reference is just saying "hey this ID is mentioned at least one time by this object" which is only marginally useful.
That's a great point, and idk if it was considered when this idea came up. cc @elastic/kibana-security @elastic/response-ops-cases I'm not sure I see a way around this issue though... we will inevitably need some way to reference user IDs from saved objects. Whether or not that support is 1st class in the saved objects service won't change the fact that references like these must exist, and exporting/importing across clusters will no longer "just work". In fact, it almost feels like the export/import concern is another point in favor of standardizing on 1st class support for user refs, because it would mean we could at least have the ability to do validations (as Pierre outlines in the options above).
I think you summed it up well, the main value would be resolving references for folks using the SOC. However I think Rudolf's suggestion of creating helpers to decorate existing SOs might be a better / more conservative place to start, to save us from mucking around too much with the existing SOC. I also think it's worth considering a more generic improvement to how references are handled as we had discussed separately in slack. Today the concept of references assumes you are only ever referring to another saved object, but I could see expanding this model (or introducing a new type of reference if it is too risky to touch the existing ones) to include a We could then have a known list of these types, of which user profiles could be an item, but we could potentially add more over time. That way we don't risk adding |
Thank you all for your answers. My main concern with user references is to standardize the way we are referring to user profiles. If this is finalized, having the user references in the case SO attributes is fine as long as we can easily migrate in the future if you decide to support it on the SOC. Maybe Rudolf's suggestion is enough for the first iteration. @legrego @jportner do I miss something here? Regarding import/export, the Case assignment is considered an MPV at the moment. My assumption is that it is ok to not support importing/exporting to different clusters as far as importing/exporting between spaces (same cluster) is working. When I export a case from one space to another I should be able to see the users assigned to the case. If I import a case to a different cluster the assignees will be broken and maybe this is fine for the first iteration of the initiative (cases could potentially show a warning message). @arisonl @alexfrancoeur Is my assumption correct? What about when in the future we support cloud users? |
From a planning/priority perspective, what @cnasikas proposes makes a lot of sense (unblock progress and help migrate when the time comes). |
Yes, I mentioned that we would want to strip user IDs from saved objects on export, which was one motivating factor for storing these in a single root-level field of the saved object.
I think there's value simply in having this defined as a root-level field so we can ensure that all consumers are using it in a uniform way. We ran into a pretty huge problem when trying to convert to shareable saved object types -- we needed to regenerate SO IDs, and we discovered that lots of consumers were using non-standard ways to link between objects, it was quite a difficult thing to unwind and force them all to use
I'm on the fence -- it might result in fewer total code changes (no need to add new options to SOC APIs for example). OTOH it might result in unhandled exceptions (all consumers currently rely on I guess there are a lot of unknowns, and it feels like the safer thing to do would be a separate
I think a I don't know that Consumers would still have to do something like:
But tbh I don't think we need a dedicated function to add a reference, the existing options are sufficient IMO.
Yes @cnasikas if you defined this in a type-level field, you could also define a migration later to use a root-level field instead. |
I think this makes the most sense as well. For the cases / collaboration use case, if we could simply convert the user reference within a comment to a string I believe that would be sufficient for import of mentions. Assignment I would assume is just empty. I added these high level future facing use cases in an email thread, but wanted to make sure they were captured within this thread as well. Looking ahead, there are four primary use cases I see on the horizon that may require this type of linkage. I'm not sure how helpful this context is for prioritizing architectural decisions, but roadmap wise, this is where we'd like to go.
This may be a tangental discussion, but I wanted to raise it as part of this discuss thread is around standardizing on saved object properties. Metadata such as created by, last modified by, create date, last modified date, etc. should be standard across saved object types. It would be great to start enforcing a schema for all saved objects so we can begin to start building common experiences around these. This is a large part of the content management effort and if we do decided to standardize for user related information, I think we should consider extending that common schema to a few other pieces of metadata as well. Last point (I swear). This discussion came up during the last content management working group. @sebelga is working on an RFC for "metadata events" that may be relevant to this discussion. Linking to the POC in case it's a helpful data point for this discussion: #133022 |
Have we looked at how assigning users to a case (or other SO) is similar to associating a label to an SO ? The fundamental mental model seems very similar if not the same and we should be aligning both if it makes sense. |
That's exactly the point I want to highlight actually: The notion of which team the responsibility should / will go to. If these user references becomes FCC of the SO APIs, the ownership / maintenance of these consistency checks will de facto go to the Core team. Not saying that's bad (nor good), just stating a fact here.
There shouldn't be any issue migrating from an attribute field to a global field later if necessary. SO migrations support it.
@jportner You would only be able to strip the user references array that way, not the usages of those references from within the attributes fields. Or would that be considered acceptable to export objects with missing references from some of their fields? Please tell me if I'm wrong or if I miss something, but it seems to me that such stripping would require logic and therefor should be performed by type-owner provided logic using the |
IMLDO (in my lame duck opinion 😄) it would be acceptable to strip root-level user references on export and force consumers to gracefully handle a situation where there their type-level attribute does not have a corresponding user reference. In other words, I don't think we should assume consumers will do any type-level attribute stripping correctly, and it's better to implement root-level attribute stripping consistently. |
What net effect would stripping root-level user references on export have for import/export within the same cluster? On import, would this nullify all user references for existing saved-objects, or just when a new saved-object was created? |
That's a good point, with this approach, if you exported and re-imported saved objects into the same cluster (or if you simply copied the objects to another space) it would nullify user references. I guess if we wanted to support preserving user references for SO import/copy, we would need to leave them intact for exports and attempt to validate them on import instead. That would probably also be better suited to a root-level attribute, not a type-level attribute. |
There are tactical pros and cons to making user references first class concepts in the SavedObjectsClient but to me this is more of a philosophical question about how much functionality we try to build into our persistence layer. High-level we want to keep the SavedObjectsClient limited to generic functionality that's free from business logic and concepts related to specific domains. Is the Do all user references belong in our persistence layer? Inside the persistence layer a user reference doesn't mean anything... the meaning comes from the domain that uses these user references. A user reference that assigns a case to a user is completely different to a reference from an @ mention in a comment. I can totally see us wanting to show all cases where I was @ mentioned or that was assigned to me, but the SavedObjectsClient won't be able to power these use cases. These will have to be built into the API service layer of the cases plugin. It could work to make @ mentioning and assignment first class concepts in the SavedObjectsClient but since these don't apply to every saved object type, let's put them into another layer (and standardise our approach there). |
This logic makes sense to me and I agree is more philosophically aligned with where we want the SavedObjectsClient to be. I think Rudolf's earlier suggestion of helpers/decorators (#134595 (comment)) is a perfectly viable solution that would still keep things simple for plugins without requiring changes to the SOC. The only use case it doesn't solve is the export/import one; we have an |
We have the onImport warning hooks #87996 which I think fit this use case well. We've speculated a bit in this issue about the user experience when importing saved objects with user references, but I think we need to get confirmation from RAC/solutions/product about the ideal workflow and then work back from that and fill any gaps if necessary. |
Ah, that's right - I forgot about the warning hooks. I think that could be enough as solutions could at least warn folks who are importing objects that contain user references. But agreed, let's clarify the desired workflow. cc @arisonl @kobelb @cnasikas Any thoughts on this from the Cases perspective? |
Thought I'd give you a TLDR on the thread so far, just to make my question clearer 🙂 We just talked about this again in our team meeting. Currently we are thinking it would make the most sense to introduce some helpers for:
// 1
addUserProfileReference(object: SavedObject) => SavedObject
// 2
retrieveUserProfileReference(object: SavedObject) => UserId
// 3
decorateUserProfileReferences(object: SavedObject) => Promise<SavedObject>
// or maybe
decorateUserProfileReferences(objects: SavedObject[]) => Promise<SavedObject[]>; That way the core team wouldn't be blocking progress on case assignment; these are helpers that could be built into your application code, and we could later consider pulling them into a shared location (maybe security plugin?). The one question that would remain would be how to deal with a scenario where a user exports SOs from one cluster and imports into another cluster where the referenced user IDs may no longer be valid. Currently we have an async Core can help identify the right scalable solution for this, but it starts with understanding what's the ideal UX, and what's an acceptable UX for an MVP. That's the part we are looking for your input on 🙂 |
Thank you, @lukeelmers. As long as the way to refer to user profiles is standardized (which seems to be the case with the helper functions) so we can migrate in the future if supported by Core, it is ok to put the references to the Cases' SO. About the imports, I think it is ok (@alexfrancoeur @arisonl correct me if I am wrong) to not show a warning on imports for the MVP. We can show a warning inside a case when we fetch the user profiles to show the assignees. Even better, we can show |
I agree with @cnasikas |
It would be great if the outcome of this issue is a solution that is not tightly coupled to saved objects. As we saw with RFC for the saved object metadata we might want to be able to have user references on other objects and let interface UserReference {
// whatever is needed
profileId: string;
};
interface MyDomainObject {
createdBy: string; // '#createdByUser'
userRefs: {
[ref: string]: UserReference;
};
}
interface SavedObject {
...
createdBy: string; // '#createdByUser'
userRefs: {
[ref: string]: UserReference;
};
};
// API only exposed to internal Kibana plugins to convert references to user profiles
retrieveUser(userRef: UserReference) => User
// Automatically maps all user references to their corresponding root attributes
soClient.get('myType' 'abc'); |
@lukeelmers As discussed offline, agreed with the above. Very lean UX handling for the MVP. I am wondering if a warning could be presented on export, i.e. before attempting to import (e.g. something along the lines of "the saved objects you are exporting contain user references which are specific to this deployment"). We'll sync with Design on options, but it is an assumption for the MVP that user profiles are deployment specific. |
Beyond that I'm a bit fuzzy on the value a user gets from migrating cases between deployments without the context of user references/assignments. I expect a user would want at least a textual description of past assignments or references in the case history. Obfuscating those profiles then becomes a user decision when exporting and mapping profiles to targets is an import option. |
With the introduction of User Profiles, we expect there to be an increasing need for owners of SOs to reference users from within their SO types.
One concrete use case we have now is the need to assign users to a Case. While it doesn't make sense to add the concept of "assignment" to SOs directly, Cases will still need a reliable way to reference a user profile from within their SO type's attributes so that they can create their own assignment fields.
We expect to see more use cases for linking user profiles to SOs emerge from other initiatives like the content management project... A full list of use cases is currently being compiled.
While we could leave it up to SO type owners to reference user profiles however they'd like (since user profile IDs are considered stable), it'd be useful to have these consolidated in one place similar to how we handle SO references. This would allow us to do things like automatically enriching SOs with user profile metadata.
Since user profiles & their corresponding metadata are accessed directly from the ES user profile APIs (via APIs provided by Kibana's
security
plugin), they don't exist as saved objects and therefore we cannot use the existingreferences
for them. (And we likely wouldn't want to ever store user profile data directly in SOs due to security/privacy concerns)So the proposal here is to start an RFC around how to link SOs to user profiles. This could look something like this:
We could use this info in a few different ways:
This issue is to track any research that needs to happen in order to inform an RFC for such a feature. The expected outcome is a lightweight RFC proposing a path forward, or a recommendation against pursuing this idea altogether.
As Cases would be the first user of this functionality, @cnasikas will available to collaborate with someone from Core on an RFC.
cc @kobelb @stacey-gammon @arisonl @alexfrancoeur @jonathan-buttner @jportner @azasypkin @legrego
The text was updated successfully, but these errors were encountered: