Integrate Licensing & Feature Controls with ApplicationService #45291
Comments
joshdover
added
Team:Core
|
Pinging @elastic/kibana-platform |
|
The NP api currently have a strong 1-1 binding between In legacy, I lack the context of this decision, so I have to ask: what this something we already decided on ? If I take the exemple of the legacy uiExports: {
[...]
app: {
id: 'kibana',
title: 'Kibana',
listed: false,
main: 'plugins/kibana/kibana',
},
links: [
{
id: 'kibana:discover',
title: i18n.translate('kbn.discoverTitle', {
defaultMessage: 'Discover',
}),
order: -1003,
url: `${kbnBaseUrl}#/discover`,
euiIconType: 'discoverApp',
},
{
id: 'kibana:visualize',
title: i18n.translate('kbn.visualizeTitle', {
defaultMessage: 'Visualize',
}),
order: -1002,
url: `${kbnBaseUrl}#/visualize`,
euiIconType: 'visualizeApp',
},
{
id: 'kibana:dev_tools',
title: i18n.translate('kbn.devToolsTitle', {
defaultMessage: 'Dev Tools',
}),
order: 9001,
url: '/app/kibana#/dev_tools',
euiIconType: 'devToolsApp',
},
[...]
],How would this be migrated to NP ? Will every |
Yes. We will also add "standalone applications" #41981 these are registered apps that don't add a NavLink. |
|
So basically today, we have two kind of apps (apps, not plugins):
Regarding the normal applications, the ones that needs to be disabled for various reasons are doing it in two distinct ways:
The latest is currently only used in two plugins:
I was thinking about removing the capability to keep a disabled navlink, However, after speaking with @timroes and @flash1293 , the use case (at least for
So, basically this seems to be a way to disable the application but to keep the link to not disturb the user in case of expired license. So we seems to have 3 possible states for the application:
So I'm think about two options here for the filter: 1/ type AppFilter = (app: App) => 'enabled' | 'disabled' | 'expired';or 2/ type AppState = 'enabled' | 'disabled' | 'expired';
type NavlinkState = 'visible' | 'hidden' | 'inactive'; // inactive to not misinterpret with 'disabled'
type AppFilter = (app: App) => AppState | {
app: AppState;
navlink: NavlinkState
}In that scenario, we allow the filter function to be able to either return
The second one seems more generic, however I'm not sure we want to manage some combination ( The other question I have is regarding the navlink tooltip in case of Currently in graph, this is what is done: const navLinkUpdates = {};
navLinkUpdates.hidden = true;
const showAppLink = xpackInfo.get('features.graph.showAppLink', false);
navLinkUpdates.hidden = !showAppLink;
if (showAppLink) {
navLinkUpdates.disabled = !xpackInfo.get('features.graph.enableAppLink', false);
navLinkUpdates.tooltip = xpackInfo.get('features.graph.message');
}
npStart.core.chrome.navLinks.update('graph', navLinkUpdates);When the app should be in the new Issue is, if we plan at some point to centralise the app disabling logic (to
WDYT ? |
|
Option 1/ makes sense to me, though maybe we should make the states more generic to fit better in the OSS use case. I don't think type AppFilter = (app: App) =>
'accessible' |
'inaccessibleWithoutNavlink' |
'inaccessibleWithDisabledNavlink';With standalone/hidden apps just ignore AppFilters completely, correct? In other words, being a
I think this depends on the names of the states. If we go with If we need to allow custom tooltips, maybe the name |
|
The OSS argument is a good point, however I'm not a big fan of over-long-and-complicated-name-when-not-absolute-necessity. Maybe Is acceptable as it should be quite clear that the behaviour for
For
We are going closer to the existing So something like |
|
So now the current proposal is something like this: type AppStatus = 'accessible' | 'inaccessible' | 'inaccessibleWithDisabledNavlink';
type AppStatusFields = Pick<AppBase, 'status' | 'tooltip'>
type AppStatusUpdater = (app: AppBase) => Partial<AppStatusFields> | undefined;
interface ApplicationSetup {
[...]
registerAppStatusUpdater(statusUpdater$: Observable<AppStatusUpdater>): void;
} |
Absolutely, because these options should not just affect the Chrome UI but also the ApplicationService router. |
|
You may want to define these values as an integer enum, where the greater value (least permissive option) takes precedence over lower values. This is important since multiple If one updater returns |
|
You're faster than me, was writing something about the precedence! I agree. |
|
Another question I was asking myself is about when The technical constraint I see here if only added in I.E an existing call: import { npStart } from 'ui/new_platform';
[...]
npStart.core.chrome.navLinks.update('graph', navLinkUpdates);I see that import { npSetup } from 'ui/new_platform';
[...]
npSetup.core.application. registerAppStatusUpdater([...])Or is there some subtlety ? Not sure about the core lifecycle in legacy world ? |
|
All legacy code gets executed after both I think we should be able to keep this only on We will just need to make sure that |
interface ApplicationSetup {
registerAppFilter(filter$: Observable<AppFilter>): void;
}
// Example usage inside the licensing plugin
core.application.registerAppFilter(
license$.pipe(
map(license => (app: App) => ({
visible: license.get(`features.${app.id}.showAppLink`),
enabled: license.get(`features.${app.id}.enableAppLink`),
})
)
)There could be more sophisticated cases besides mapping: When a plugin license result depends on another plugin/feature license status, for example. I don't think that the
I don't see a lot of benefits of scattering logic over a few places. Given a proposed syntax: core.application.register({
id: 'ml',
metadata: {
requiredLicenseLevel: ['trial', 'platinum']
},
mount( ... ) { },
})Now application service allows attaching an arbitrary piece of data, which will be used by the license plugin later. It will be hard to track all the dependencies passed through this mechanism.
Just curious why not allow plugins to register a custom renderer for a navLink? We can get rid of
We can expose updater in start phase only for LP. Otherwise plugin authors will be confused about differences between setup and start phases.
We already use the |
I agree, if there many varied calculations, than we should not do a global licensing integration and should instead just provide a mechanism for plugins to register their own filter/updater.
I like this idea for the presentation side of things, but there's more to this than just what the icon looks like. The mechanism needed by licensing and feature controls is to actually show a 404-like page when a user tries to access an App via routing directly to it.
Agreed, we need a standard name. In discussions with @eliperelman and @pgayvallet last week, we decided "chromeless" for these was the least ambiguous name. This replaces the "standalone" and "hidden" nomenclature. |
In my experience, allowing full rendering access to that kind of sensible UI parts always result in breaking some global stuff at some point. I'm not very fond of allowing plugin developers to freely render custom things into global-spaced UI (even if getting rid of vendor types such as
The proposed implementation actually allows to do that. Instead of having security or licensing registering a global app filter, we can just recommend that each plugins register their own filter for their applications. If we go (exclusively) in that direction however, we may want to add the $statusUpdater to the actual app definition instead of allowing to add them afterwise ? const statusUpdaters$ = new BehaviorSubject<AppStatusUpdater>(() => ({}));
core.application.register({
id: 'ml',
statusUpdater$,
mount( ... ) { },
})
// later
statusUpdaters$.next((app) => {
return {
status: 'whatever'
}
})or to add a initial core.application.registerAppFilter('myApp',
license$.pipe(
map(license => (app: App) => ({ // app parameter may even be unnecessary in that case
visible: license.get(`features.${app.id}.showAppLink`),
enabled: license.get(`features.${app.id}.enableAppLink`),
})
)
)( I think I prefer the first approach if we want to forget about delegating this responsibility to a specific plugin). One limitation is that we can't properly manage accessibility status for legacy apps that way (we can with the 'global' filters), but not sure if it's an issue FYI, I have a draft PR with the initial proposed implementation: #50223 that can be a starting point for any wanted changes on the API |
I think makes sense in the licensing case, but maybe not in the feature controls case.
I don't think we need to necessarily worry about legacy plugins, but only if we're ok leaving the legacy code for those apps there until they'e been migrated. I think in most cases this should be fine. |
|
@joshdover @restrry A/ interface ApplicationSetup {
[...]
registerAppStatusUpdater(statusUpdater$: Observable<AppStatusUpdater>): void;
}and B/ core.application.register({
id: 'ml',
statusUpdater$,
mount( ... ) { },
})to answer both cases in the best way ? I have no idea what we plan to do regarding NP feature controls. But the current usages of app filtering / disabling are closer in use to the B proposal. |
|
@kobelb I'm fuzzy now on what we do to with feature controls to prevent access to applications entirely. I know we have logic to filter out navlinks based on feature controls, but where do we currently prevent a user from accessing |
|
So atm, actual app authorisation check is only done server side. The only part that is done client-side is the 'hide from navbar using custom, non-generic per-navlink checks'. Server side authorisation check however is generic, checking permission for every apps in a single place. So, we're back to the ticket description:
If we want to allow this, the A/ proposal of my previous comment seems like the correct approach. If we only want to migrate what's done in LP to hide/disable an app, B/ seems like the way to go. If we want both, we can also implements both, and it wouldn't be much more work. Main question now would be: what do we actually want 😄? I have the feeling that in this specific case, more is probably better than less? Are we all alright to implement both approach? That way we can migrate existing code that hides navlinks using B/, and are still ready for the day we may want to implement (or let security implement) client-side app checks using A/? |
|
tl;dr: I think we will need both, or something close to it. The current server-side authz checks won't really work in the New Platform because we don't do a server-side render for each app like we do in legacy. Given that, there's not any global hooks for security to plug into to restrict access. An alternative option could be to do A for license checks now, use the current capabilities filtering to hide app icons for feature controls, and then add a |
We'll likely have to rework a few things to make this possible, but it won't be a colossal amount of effort. If you can let us know when the hook is available, we can begin taking advantage of it. |
|
So I guess I will implement both in #50223
Precedence of status is shared between them, meaning that the most restricting status will always be applied. Is that alright with everyone? |
|
@pgayvallet works for me. |
There are 3 different cases when an application should be disabled or hidden from the UI:
timelion.ui.enabled).In the Legacy Platform we have a few different mechanisms for controlling which apps are shown & allowed.
chrome.navLinks.update()to disable a navlink based on some of the conditions listed above. Each app has to do this separately and the implementations are not consistent.navLinkitem inuiCapabilities.The ApplicationService should expose a function that allows other systems to register an Observable of filtering functions that can be used to disable and hide applications. This would allow the Licensing and Feature Control plugins to extend the ApplicationService with this knowledge in a single place, rather than each plugin defining this logic.
Example API
The text was updated successfully, but these errors were encountered: