Adding base RestHandler class for Enrollment APIs #76564
Conversation
This change adding an abstract RestHandler class and extends it by enrollment API classes (node and Kibana enrollment). It will handle the cases when `enrollment.enabled` is not set to `true`. It will return an appropriate exception in this case.
|
Pinging @elastic/es-security (Team:Security) |
| License.OperationMode licenseMode = randomFrom(License.OperationMode.BASIC, License.OperationMode.TRIAL, | ||
| License.OperationMode.PLATINUM, License.OperationMode.ENTERPRISE); | ||
| final TestUtils.UpdatableLicenseState licenseState = new TestUtils.UpdatableLicenseState(settings); | ||
| licenseState.update(licenseMode, true, Long.MAX_VALUE, null); | ||
|
|
There was a problem hiding this comment.
nit: We don't check the licenseState ever in this tests, so you don't necessarily need this, you can use new XPackLicenseState(() -> 0) in your constructor below
| if (failedFeature != null) { | ||
| return failedFeature; | ||
| } else if (XPackSettings.ENROLLMENT_ENABLED.get(settings) == false) { | ||
| return new ElasticsearchSecurityException("Enrollment mode is not enabled", RestStatus.FORBIDDEN); |
There was a problem hiding this comment.
Can we add a more actionable message here? Something like "Enrollment mode [" + XPackSettings.ENROLLMENT_ENABLED.getKey() + "] is not enabled" ? WDYT ?
| } else if (XPackSettings.ENROLLMENT_ENABLED.get(settings) == false) { | ||
| return new ElasticsearchSecurityException("Enrollment mode is not enabled", RestStatus.FORBIDDEN); | ||
| return new ElasticsearchSecurityException("Enrollment mode [" + XPackSettings.ENROLLMENT_ENABLED.getKey() + "] is not " + | ||
| "configured", |
There was a problem hiding this comment.
nit again: I find "not enabled" to be less confusing than "not configured" but I'll leave this up to you. ( It can "be configured" ==> explicitly set to false and we'd need to throw here.
There was a problem hiding this comment.
I think "Enrollment mode [xpack.security.enrollment.enabled] is not enabled" is a bit repetitive.
There was a problem hiding this comment.
May be "Enrollment mode [xpack.security.enrollment.enabled] is not true"? Or not set?
There was a problem hiding this comment.
"not set" has the same problem with "not configured" , it can be set, to false and we'd still throw. The confusion comes from having a single sentence that has
- The "Enrollment mode" , which can be enabled or disabled
- The
xpack.security.enrollment.enabledwhich can be set totrueorfalse.
We can either
- revert to a more generic message like you had it originally "Enrollment mode is not enabled" or
- expand a bit to "Enrollment mode is not enabled. Set [" + XPackSettings.ENROLLMENT_ENABLED.getKey() + "] to true, in order to use this API."
There was a problem hiding this comment.
I like the last one!
|
@elasticmachine update branch |
|
@elasticmachine update branch |
|
@elasticmachine update branch |
|
@elasticmachine update branch |
This change adding an abstract RestHandler class and extends it by
enrollment API classes (node and Kibana enrollment). It will handle the
cases when
enrollment.enabledis not set totrue. It will return anappropriate exception in this case.
Resolves: #76097