Improve security server types by stacey-gammon · Pull Request #101661 · elastic/kibana

stacey-gammon · 2021-06-08T19:10:03Z

Remove the RecursiveReadonly wrapper to setup and start types.
Export some APIs that are part of the public API.

Before:

There were no setup or start types reported due to the RecursiveResonly wrapper.

After:

kibanamachine · 2021-06-08T21:45:50Z

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`security`	45	49	+4

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`security`	11	5	-6

Unknown metric groups

API count

id	before	after	diff
`security`	95	109	+14

History

💚 Build #129927 succeeded f47a0ca

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

azasypkin

Looks great, thanks a lot! Just a few minor nits and questions.

azasypkin · 2021-06-09T11:39:02Z

x-pack/plugins/security/common/index.ts

 export { SecurityLicense } from './licensing';
+export { AuthenticatedUser } from './model';


nit: while you here ....

Suggested change

export { SecurityLicense } from './licensing';

export { AuthenticatedUser } from './model';

export type { SecurityLicense } from './licensing';

export type { AuthenticatedUser } from './model';

azasypkin · 2021-06-09T11:45:46Z

x-pack/plugins/security/server/authentication/authentication_service.ts

 }

-export interface AuthenticationServiceStart {
+export interface AuthenticationServiceStartInternal extends AuthenticationServiceStart {


question: I see Core uses Internal as a prefix to internal contracts (e.g. InternalHttpServiceSetup) and here we use it as a suffix. Do we have any convention for that?

Not that I am aware of, though I can change to prefix to be consistent.

azasypkin · 2021-06-09T11:50:11Z

x-pack/plugins/security/server/authorization/authorization_service.tsx

+  /**
+   * Actions are used to create the "actions" that are associated with Elasticsearch's
+   * application privileges, and are used to perform the authorization checks implemented
+   * by the various `checkPrivilegesWithRequest` derivatives


nit:

Suggested change

* by the various `checkPrivilegesWithRequest` derivatives

* by the various `checkPrivilegesWithRequest` derivatives.

azasypkin · 2021-06-09T11:52:36Z

x-pack/plugins/security/server/index.ts

 export type { SecurityPluginSetup, SecurityPluginStart };
 export type { AuthenticatedUser } from '../common/model';
 export { ROUTE_TAG_CAN_REDIRECT } from './routes/tags';
+export { AuditServiceSetup } from './audit';


nit:

Suggested change

export { AuditServiceSetup } from './audit';

export type { AuditServiceSetup } from './audit';

azasypkin · 2021-06-09T11:53:51Z

x-pack/plugins/security/server/plugin.ts

 import type { AuthorizationServiceSetup } from './authorization';
 import { AuthorizationService } from './authorization';
+import type { AuthorizationServiceSetupInternal } from './authorization/authorization_service';


nit:

Suggested change

import type { AuthorizationServiceSetup } from './authorization';

import { AuthorizationService } from './authorization';

import type { AuthorizationServiceSetupInternal } from './authorization/authorization_service';

import type { AuthorizationServiceSetup, AuthorizationServiceSetupInternal } from './authorization';

import { AuthorizationService } from './authorization';

azasypkin · 2021-06-09T11:57:36Z

x-pack/plugins/security/server/routes/users/change_password.test.ts

 import { mockAuthenticatedUser } from '../../../common/model/authenticated_user.mock';
-import type { AuthenticationServiceStart } from '../../authentication';
 import { AuthenticationResult } from '../../authentication';
+import type { AuthenticationServiceStartInternal } from '../../authentication/authentication_service';


nit:

Suggested change

import type { AuthenticationServiceStartInternal } from '../../authentication/authentication_service';

import type { AuthenticationServiceStartInternal } from '../../authentication';

azasypkin · 2021-06-09T11:58:33Z

x-pack/plugins/security/server/saved_objects/index.ts

 import { KibanaRequest, SavedObjectsClient } from '../../../../../src/core/server';
 import type { AuditServiceSetup, SecurityAuditLogger } from '../audit';
-import type { AuthorizationServiceSetup } from '../authorization';
+import type { AuthorizationServiceSetupInternal } from '../authorization/authorization_service';


nit:

Suggested change

import type { AuthorizationServiceSetupInternal } from '../authorization/authorization_service';

import type { AuthorizationServiceSetupInternal } from '../authorization';

azasypkin · 2021-06-09T12:01:08Z

x-pack/plugins/security/server/plugin.ts

  /**
   * @deprecated Use `authz` methods from the `SecurityServiceStart` contract instead.
   */
-  authz: Pick<


question: I definitely agree that it's much better to use dedicated interface here, but just out of curiosity - does Pick not play well with the API docs?

It doesn't, it also doesn't work well in an IDE. From best practices:

Ah, missed that somehow, good to know, thanks!

azasypkin · 2021-06-09T12:02:58Z

Wow, didn't realize PR will be automatically merged after a single approval 😢

stacey-gammon · 2021-06-09T12:37:44Z

Guess I shouldn't put on auto-merge before a review. 😬 I'll throw up a new PR to address your comments, thanks for the review!

* Remove RecursiveReadonly wrapper on public API items * Remove Pick and export some types that are part of the public API * Udpate api docs * Export API items that are part of the public API * Add extra comments * update api docs

kibanamachine · 2021-06-09T14:14:10Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

* Remove RecursiveReadonly wrapper on public API items * Remove Pick and export some types that are part of the public API * Udpate api docs * Export API items that are part of the public API * Add extra comments * update api docs Co-authored-by: Stacey Gammon <gammon@elastic.co>

* master: clarify which parts of TM are experimental (elastic#101757) Add sh scripts with _bulk_action route usage examples (elastic#101736) [Uptime] Only register route in side nav if uptime show capability is true (elastic#101709) Use KIBANA_DOCS in doc link service (elastic#101667) [Alerting][Event log] Persisting duration information for active alerts in event log (elastic#101387) Address design issues in Discover/Graph (elastic#101584) Optimize performance for document table (elastic#101715) Change file data visualizer links to point to new location in home application (elastic#101393) [Fleet] Tighten policy permissions, take II (elastic#97366) [ML] Add debounce to the severity control update (elastic#101581) [Fleet] Fix routing issues with `getPath` and `history.push` (elastic#101658) [APM] Add link-to/transaction route (elastic#101731) [Index Patterns] Runtime fields CRUD REST API (elastic#101164) [ILM] Refactor types and fix missing aria labels (elastic#101518) [Lens] New summary row feature for datatable (elastic#101075) Blocks save event filter with a white space name (elastic#101599) Improve security server types (elastic#101661) [APM] Replace side nav with tabs on Settings page (elastic#101460) [APM] Only register items in side nav if user has permissions to see app (elastic#101707) [Security solution][Endpoint] Add back button when to the event filters list (elastic#101280)

Stacey Gammon added 5 commits June 8, 2021 14:46

Remove RecursiveReadonly wrapper on public API items

00662cb

Remove Pick and export some types that are part of the public API

c5a067f

Udpate api docs

86f82a2

Export API items that are part of the public API

69caaa8

Add extra comments

f47a0ca

stacey-gammon requested a review from a team as a code owner June 8, 2021 19:10

legrego requested a review from azasypkin June 8, 2021 19:11

update api docs

8916772

stacey-gammon added release_note:skip Skip the PR/issue when compiling release notes v7.14.0 v8.0.0 labels Jun 8, 2021

stacey-gammon requested a review from legrego June 8, 2021 21:35

stacey-gammon enabled auto-merge (squash) June 8, 2021 21:35

azasypkin approved these changes Jun 9, 2021

View reviewed changes

stacey-gammon merged commit 7604fd7 into elastic:master Jun 9, 2021

stacey-gammon mentioned this pull request Jun 9, 2021

Addresses minor comments from a PR that was auto-merged #101764

Merged

stacey-gammon added the auto-backport Deprecated - use backport:version if exact versions are needed label Jun 9, 2021

kibanamachine mentioned this pull request Jun 9, 2021

[7.x] Improve security server types (#101661) #101767

Merged

azasypkin mentioned this pull request Jun 10, 2021

[Workplace Search] Add Account Settings page imported from Security plugin #99791

Merged

2 tasks

		export { SecurityLicense } from './licensing';
		export { AuthenticatedUser } from './model';

	* by the various `checkPrivilegesWithRequest` derivatives
	* by the various `checkPrivilegesWithRequest` derivatives.

	export { AuditServiceSetup } from './audit';
	export type { AuditServiceSetup } from './audit';

	import type { AuthenticationServiceStartInternal } from '../../authentication/authentication_service';
	import type { AuthenticationServiceStartInternal } from '../../authentication';

	import type { AuthorizationServiceSetupInternal } from '../authorization/authorization_service';
	import type { AuthorizationServiceSetupInternal } from '../authorization';

Conversation

stacey-gammon commented Jun 8, 2021

Uh oh!

kibanamachine commented Jun 8, 2021

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Public APIs missing exports

API count

History

Uh oh!

azasypkin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin commented Jun 9, 2021

Uh oh!

stacey-gammon commented Jun 9, 2021

Uh oh!

kibanamachine commented Jun 9, 2021

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants