Skip to content
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

Migrate capabilities mixin to New Platform service #45393

Closed
joshdover opened this issue Sep 11, 2019 · 3 comments · Fixed by #51438
Closed

Migrate capabilities mixin to New Platform service #45393

joshdover opened this issue Sep 11, 2019 · 3 comments · Fixed by #51438
Assignees
Labels
blocker Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@joshdover
Copy link
Contributor

The legacy capabilities mixin needs to be migrated to the New Platform as a Core service.

This service should handle:

  • Allowing capability providers to be registered
  • Allowing capability "togglers" to be registered for turning capabilities off and on. This is different than the current "capability modifiers" that can also add options. We should lock this down to only toggle fields.
  • Provide an API endpoint for the client-side to retrieve the current user's capabilities based on the registered applications.
@joshdover joshdover added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform labels Sep 11, 2019
@pgayvallet
Copy link
Contributor

My questions / remarks after inspecting src/legacy/server/capabilities/capabilities_mixin.ts and capabilities usages:

atm the Capabilities type is imported from client to server

import { Capabilities } from '../../../core/public';

I guess we should move the type from src/core/public to src/core/types for proper mixed usage?

The capabilities type is quite permissive

export interface Capabilities {
  /** Navigation link capabilities. */
  navLinks: Record<string, boolean>;

  /** Management section capabilities. */
  management: {
    [sectionId: string]: Record<string, boolean>;
  };

  /** Catalogue capabilities. Catalogue entries drive the visibility of the Kibana homepage options. */
  catalogue: Record<string, boolean>;

  /** Custom capabilities, registered by plugins. */
  [key: string]: Record<string, boolean | Record<string, boolean>>;
}

Some keys are explicitly declared (navLinks,management,catalogue), but the last [key: string]: Record<string, boolean | Record<string, boolean>>; allows to basically put anything in am unstructured way.

There are two things that bothers me here:

  • I would like to type this in a more structured way, however, I'm not really sure how to achieve this while keeping legacy compat?
  • I really don't like the navLinks term. Are we moved in NP from them to Application, I would like to really rename/remove that if possible. However once again, we are really dependant on LP needs and compatibility here.

LP capabilities are (mostly?) from plugin descriptors

In LP, the plugins capabilities are described plugin descriptors:

const defaultCapabilities = mergeCapabilities(
...(await Promise.all(
kbnServer.pluginSpecs
.map(spec => spec.getUiCapabilitiesProvider())
.filter(provider => !!provider)
.map(provider => provider(server))
))
);

uiCapabilities: async function () {
return {
discover: {
show: true,
createShortUrl: true,
save: true,
saveQuery: true,
},
visualize: {

The accessor (uiCapabilities: async function () {) is an async function, however I could not find any usage where a static object wouldn't have worked. So I guess we could add a similar mecanism in each plugin's kibana.json

{
  "id": "data",
  "version": "kibana",
  "server": true,
  "ui": true,
  "capabilities": {
    "navlinks": {
      "myapp": true
    },
    "data": {
      "aFeature": true
    }
  }
}
  • Do we want that?
  • If we do, do we want to also add a programmatic way to register capabilities to manage edge cases?

LP server knows about client navlinks. NP does not

navlinks are a special things in capabilities, as they are not declared in the plugin descriptors but instead generated from kbnServer.uiExports.

server.decorate('request', 'getCapabilities', function() {
// Get legacy nav links
const navLinks = server.getUiNavLinks().reduce(
(acc, spec) => ({
...acc,
[spec._id]: true,
}),
{} as Record<string, boolean>
);
return resolveCapabilities(this, modifiers, defaultCapabilities, { navLinks });
});

export function uiNavLinksMixin(kbnServer, server) {
const uiApps = server.getAllUiApps();
const { navLinkSpecs = [] } = kbnServer.uiExports;
const fromSpecs = navLinkSpecs
.map(navLinkSpec => new UiNavLink(navLinkSpec));
const fromApps = uiApps
.map(app => app.getNavLink())
.filter(Boolean);
const uiNavLinks = fromSpecs
.concat(fromApps)
.sort((a, b) => a.getOrder() - b.getOrder());
server.decorate('server', 'getUiNavLinks', () => (
uiNavLinks.slice(0)
));
}

NP have no way to access the NP plugins application list from server side (and we really don't want to find a way). So I guess the NP plugins will register their app/navlinks the same way they do with other capabilities. Do we all agree on that, or do we want to find some way to do 'auto-discovery' on applications?

current CapabilitiesModifier needs Request access

export type CapabilitiesModifier = (
  request: Request,
  uiCapabilities: Capabilities
) => Capabilities | Promise<Capabilities>;

This is mostly used by spaces/security to access session/space from the request:

legacyAPI.capabilities.registerCapabilitiesModifier(async (request, uiCapabilities) => {
try {
const activeSpace = await spacesService.getActiveSpace(KibanaRequest.from(request));
const features = featuresSetup.getFeatures();
return toggleUICapabilities(features, uiCapabilities, activeSpace);
} catch (e) {
return uiCapabilities;
}
});

const getSpaceId = (request: RequestFacade) => {
// Currently utilized by reporting
const isFakeRequest = typeof (request as any).getBasePath === 'function';
const basePath = isFakeRequest
? (request as Record<string, any>).getBasePath()
: http.basePath.get(request);
const spaceId = getSpaceIdFromPath(basePath, http.basePath.serverBasePath);
return spaceId;
};

Accessing the current context makes sense for that kind of feature, as capabilities modifiers/togglers will need to know about context to toggle the capabilities.

  • Should we keep this interface parameters, or do we can/want to provide a more specialised object that the raw request?

Client-side capabilities service

We already have a client-side service for this in src/core/public/application/capabilities/capabilities_service.tsx. It currently access the capabilities from the injectedMetadata.

  • Does this issue moves the service as first-class, therefor moving up from the ApplicationService?
  • One of the ticket requirements is

Provide an API endpoint for the client-side to retrieve the current user's capabilities based on the registered applications.

Is the current approach acceptable or do we want to change it for a proper http endpoint and call from frontend?

@joshdover
Copy link
Contributor Author

I guess we should move the type from src/core/public to src/core/types for proper mixed usage?

👍

Some keys are explicitly declared (navLinks,management,catalogue), but the last [key: string]: Record<string, boolean | Record<string, boolean>>; allows to basically put anything in am unstructured way.

There are two things that bothers me here:

  • I would like to type this in a more structured way, however, I'm not really sure how to achieve this while keeping legacy compat?

It would be nice to at least move the custom capabilities under a custom key or something like that.

  • I really don't like the navLinks term. Are we moved in NP from them to Application, I would like to really rename/remove that if possible. However once again, we are really dependant on LP needs and compatibility here.

See comment below about navLinks.

The accessor (uiCapabilities: async function () {) is an async function, however I could not find any usage where a static object wouldn't have worked. So I guess we could add a similar mecanism in each plugin's kibana.json

  • Do we want that?
  • If we do, do we want to also add a programmatic way to register capabilities to manage edge cases?

Before this change, we needed uiCapabilities to be able to do dynamic things. I agree that we probably don't need this anymore and a static spec would be fine.

LP server knows about client navlinks. NP does not

navlinks are a special things in capabilities, as they are not declared in the plugin descriptors but instead generated from kbnServer.uiExports.

NP have no way to access the NP plugins application list from server side (and we really don't want to find a way). So I guess the NP plugins will register their app/navlinks the same way they do with other capabilities. Do we all agree on that, or do we want to find some way to do 'auto-discovery' on applications?

Agreed that navLinks are strange in here, this was really just necessary for LP usage. It's important to note that this part of capabilities is not read anywhere in the server-side code, only toggled by the capabilities modifiers.

Given that, the server doesn't actually need to know about all the possible navLinks in the system.

One flow that could work (and was the original intent) is that the client-side could send a list of all the registered applications to a backend endpoint and receive a list back of which apps should be available. I believe this pattern would work with both legacy and NP apps.

It actually worked like this for a bit until we found some issues with missing security request interceptors. That has since been resolved. See #36319 for more details.

Accessing the current context makes sense for that kind of feature, as capabilities modifiers/togglers will need to know about context to toggle the capabilities.

  • Should we keep this interface parameters, or do we can/want to provide a more specialised object that the raw request?

A more specific interface probably makes sense, you'll probably need to look at these two modifiers that are registered and see what they actually need access to.

Client-side capabilities service

We already have a client-side service for this in src/core/public/application/capabilities/capabilities_service.tsx. It currently access the capabilities from the injectedMetadata.

  • Does this issue moves the service as first-class, therefor moving up from the ApplicationService?
  • One of the ticket requirements is

Provide an API endpoint for the client-side to retrieve the current user's capabilities based on the registered applications.

Is the current approach acceptable or do we want to change it for a proper http endpoint and call from frontend?

As mentioned above, the current approach probably isn't what we want. We should make an HTTP request with the list of registered applications so that the backend can manage toggling access to those.

In terms of where this lives in Core, I think being part of ApplicationService is debatable. The capabilities data is not exactly coupled to applications, but we need the list of capabilities in order to make the request to the backend and filter the applications. Regardless, it may make sense to move it to a first-class service from an API consumer standpoint.

@pgayvallet
Copy link
Contributor

@joshdover I got a feeling the Application/NavLink capabilities are like a specific case and that they were added in capabilities for legacy reasons which may not be of much sense now.

I see conflicting direction regarding how we want to manage them between this issue and #45291:

I don't think implementing both makes much sense?

IMHO we either

  • Decide that all capabilities should be managed from the server-side, and close Integrate Licensing & Feature Controls with ApplicationService #45291
  • Decide that applications/navlinks are something specific which does not belongs to Capabilities and should be managed from the client side, and therefor we remove them from the 'server-managed' capabilities in the scope of this issue.

I've got two mains reasons that would make me choose the second option:

  • As you stated, they are unused on the server-side

It's important to note that this part of capabilities is not read anywhere in the server-side code, only toggled by the capabilities modifiers.

WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants