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.

Sign up for GitHub

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

Jump to bottom

Finalize removal of legacy audit logger #116282

Merged
merged 12 commits into from
Oct 30, 2021
Merged

Finalize removal of legacy audit logger #116282

merged 12 commits into from
Oct 30, 2021

Conversation

watson
Copy link
Contributor

@watson watson commented Oct 26, 2021 •
edited by jportner
Loading

@watson watson added release_note:breaking v8.0.0 backport:skip This commit does not require backporting labels Oct 26, 2021
@watson watson self-assigned this Oct 26, 2021
@watson watson mentioned this pull request Oct 26, 2021
Closed
@jportner jportner mentioned this pull request Oct 26, 2021
Merged
3 tasks
@watson watson force-pushed the remove-legacy-audit-logger branch from 9698215 to 14ee649 Compare October 27, 2021 10:04
@watson watson changed the title Remove legacy audit logger Finalize removal of legacy audit logger Oct 27, 2021
@watson watson added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:breaking labels Oct 27, 2021
@watson watson marked this pull request as ready for review October 27, 2021 10:54
@watson watson requested review from a team as code owners October 27, 2021 10:54
@watson watson enabled auto-merge (squash) October 27, 2021 14:46
YulNaumenko
YulNaumenko approved these changes Oct 27, 2021
View reviewed changes
Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alerting changes LGTM!

jportner
jportner reviewed Oct 28, 2021
View reviewed changes
Copy link
Contributor

@jportner jportner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job on this! I have a few nits below, other than those it looks good.

Also, x-pack/plugins/encrypted_saved_objects/README.md currently reads:

The purpose of this plugin is to provide a way to encrypt/decrypt attributes on the custom Saved Objects that works with security and spaces filtering as well as performing audit logging.

We should update that to remove mention of audit logging, since we opted not to add ESO-specific audit events anymore.

docs/user/security/audit-logging.asciidoc Outdated Show resolved Hide resolved
docs/settings/security-settings.asciidoc Outdated Show resolved Hide resolved
docs/settings/security-settings.asciidoc Outdated Show resolved Hide resolved
docs/user/security/audit-logging.asciidoc Outdated Show resolved Hide resolved
docs/user/security/audit-logging.asciidoc Outdated Show resolved Hide resolved
x-pack/plugins/security/server/spaces/secure_spaces_client_wrapper.ts Outdated Show resolved Hide resolved
x-pack/plugins/security/server/audit/audit_events.ts Show resolved Hide resolved
x-pack/plugins/security/common/licensing/license_service.ts
@@ -119,7 +116,6 @@ export class SecurityLicenseService {
showRoleMappingsManagement: isLicenseGoldOrBetter,
allowAccessAgreement: isLicenseGoldOrBetter,
allowAuditLogging: isLicenseGoldOrBetter,
allowLegacyAuditLogging: isLicenseStandardOrBetter,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL there is such thing as a "Standard" license.

x-pack/plugins/security/common/licensing/license_service.test.ts Show resolved Hide resolved
x-pack/plugins/alerting/server/authorization/alerting_authorization.ts
Comment on lines 267 to 269
if (!authorizedRuleTypes.size) {
throw Boom.forbidden(
this.auditLogger.logUnscopedAuthorizationFailure(username!, 'find', authorizationEntity)
);
throw Boom.forbidden(`Unauthorized to find ${authorizationEntity}s for any rule types`);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a better error message 👍

@watson watson mentioned this pull request Oct 28, 2021
Merged
@watson watson removed the backport:skip This commit does not require backporting label Oct 28, 2021
@watson watson added auto-backport Deprecated - use backport:version if exact versions are needed v8.1.0 labels Oct 28, 2021
@watson watson requested a review from jportner October 28, 2021 08:51
@watson
Copy link
Contributor Author

watson commented Oct 28, 2021

@elasticmachine merge upstream

jportner
jportner reviewed Oct 28, 2021
View reviewed changes
Copy link
Contributor

@jportner jportner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, one last thing! I mentioned in #116282 (comment) that the ensureAuthorizedAtSpace method in the SecureSpacesWrapper has a now-unused method parameter, it can be removed. I can't comment on that part of the code directly as you didn't change it 😅

jportner
jportner approved these changes Oct 28, 2021
View reviewed changes
Copy link
Contributor

@jportner jportner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes! LGTM

@watson
Copy link
Contributor Author

watson commented Oct 30, 2021

@elasticmachine merge upstream

@watson watson merged commit 8a39a11 into elastic:main Oct 30, 2021
@kibanamachine
Copy link
Contributor

💚 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 54 47 -7

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
security 49.9KB 49.8KB -107.0B
Unknown metric groups

API count

id before after diff
security 116 108 -8

References to deprecated APIs

id before after diff
actions 15 12 -3
alerting 20 15 -5
encryptedSavedObjects 4 2 -2
security 71 69 -2
total -12

History

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

cc @watson

@kibanamachine kibanamachine mentioned this pull request Oct 30, 2021
Merged
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 30, 2021
@watson @kibanamachine
Finalize removal of legacy audit logger (elastic#116282)
c65acb1
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
✅ 8.0

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Oct 31, 2021
@kibanamachine @watson
Finalize removal of legacy audit logger (#116282) (#116902)
9da239b 
Co-authored-by: Thomas Watson <w@tson.dk>
@jportner jportner mentioned this pull request Nov 1, 2021
Merged
@watson watson deleted the remove-legacy-audit-logger branch November 2, 2021 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YulNaumenko YulNaumenko YulNaumenko approved these changes

@jportner jportner jportner approved these changes

@pgayvallet pgayvallet pgayvallet approved these changes

Assignees

@watson watson

Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes v8.0.0 v8.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@watson @kibanamachine @pgayvallet @jportner @YulNaumenko