Fix sub-feature privilege "minimumLicense" bug #106008
Conversation
We were accidentally comparing license level strings, and we happened to write unit tests that passed. Changed to compare license level enum values instead.
Also removes it from the Security plugin's public contracts. This is not needed anymore, it was only used for the sub-feature privilege iterator.
jportner
added
v8.0.0
release_note:skip
| new KibanaFeature(featureConfig), | ||
| { | ||
| augmentWithSubFeaturePrivileges: true, | ||
| licenseType, | ||
| licenseHasAtLeast: (licenseTypeToCheck: LicenseType) => | ||
| LICENSE_TYPE[licenseTypeToCheck] <= LICENSE_TYPE[licenseType], |
There was a problem hiding this comment.
I don't love importing LICENSE_TYPE directly from the licensing server for this unit test, but it seemed to be the least-bad option here.
| export const licenseMock = { | ||
| create: (features?: Partial<SecurityLicenseFeatures>): jest.Mocked<SecurityLicense> => ({ | ||
| create: ( | ||
| features: Partial<SecurityLicenseFeatures> = {}, | ||
| licenseType: LicenseType = 'basic' // default to basic if this is not specified | ||
| ): jest.Mocked<SecurityLicense> => ({ | ||
| isLicenseAvailable: jest.fn().mockReturnValue(true), | ||
| isEnabled: jest.fn().mockReturnValue(true), | ||
| getType: jest.fn().mockReturnValue('basic'), | ||
| getFeatures: jest.fn(), | ||
| getFeatures: jest.fn().mockReturnValue(features), | ||
| hasAtLeast: jest | ||
| .fn() | ||
| .mockImplementation( | ||
| (licenseTypeToCheck: LicenseType) => | ||
| LICENSE_TYPE[licenseTypeToCheck] <= LICENSE_TYPE[licenseType] | ||
| ), | ||
| features$: features ? of(features as SecurityLicenseFeatures) : of(), | ||
| }), | ||
| }; |
There was a problem hiding this comment.
I don't love importing LICENSE_TYPE directly from the licensing server for this securityLicenseService mock, but it seemed to be the least-bad option here.
| getFeatures(): SecurityLicenseFeatures; | ||
| hasAtLeast(licenseType: LicenseType): boolean | undefined; |
There was a problem hiding this comment.
We could coerce this to a boolean but this seemed like a better idea.
There was a problem hiding this comment.
Can you explain why you think this is a better idea? The one place we are using this implicitly coerces undefined to false, so it seems that we do want coercion, it's just a matter of where it takes place.
IMO it makes more sense to explicitly coerce here, unless/until we have a need for this return type to be an optional boolean.
There was a problem hiding this comment.
getType(), which this supersedes, also returned undefined if the license service wasn't available. Not sure if any other consumers may need to differentiate in the future between "license unavailable" or "minimum license not met". Happy to change it though if you feel explicit coercion is better here, I was a bit on the fence about it anyway.
There was a problem hiding this comment.
I don't feel strongly enough to have you change it, I mostly wanted to understand your reasoning. I'm good either way 👍
jportner
added
the
auto-backport
|
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Public APIs missing comments
Page load bundle
History
To update your PR or re-run it, just comment with: |
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
Co-authored-by: Joe Portner <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
Resolves #105835.
Not labeling this as a bugfix release note, as a bugged version hasn't actually shipped.