Skip to content

Replace license check isAuthAllowed with isSecurityEnabled#54547

Merged
rjernst merged 8 commits intoelastic:masterfrom
rjernst:refactor_license7
Apr 10, 2020
Merged

Replace license check isAuthAllowed with isSecurityEnabled#54547
rjernst merged 8 commits intoelastic:masterfrom
rjernst:refactor_license7

Conversation

@rjernst
Copy link
Copy Markdown
Member

@rjernst rjernst commented Apr 1, 2020

The isAuthAllowed() method for license checking is used by code that
wants to ensure security is both enabled and available. The enabled
state is dynamic and provided by isSecurityEnabled(). But since security
is available with all license types, an check on the license level is
not necessary. Thus, this change replaces isAuthAllowed() with calling
isSecurityEnabled().

The isAuthAllowed() method for license checking is used by code that
wants to ensure security is both enabled and available. The enabled
state is dynamic and provided by isSecurityEnabled(). But since security
is available with all license types, an check on the license level is
not necessary. Thus, this change replaces isAuthAllowed() with calling
isSecurityEnabled().
@rjernst rjernst added >refactoring :Security/License License functionality for commercial features v8.0.0 v7.8.0 labels Apr 1, 2020
@elasticmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-security (:Security/License)

@rjernst rjernst requested a review from tvernum April 1, 2020 00:06
assertBusy(() -> {
for (XPackLicenseState licenseState : internalCluster().getInstances(XPackLicenseState.class)) {
if (licenseState.isAuthAllowed() == false) {
if (licenseState.isSecurityEnabled() == false) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We briefly chatted about this elsewhere but in this test class security should always be enabled so I think that this should no longer be in a loop over internal state but just go ahead and call enableLicensing(OperationMode.BASIC)

@jaymode
Copy link
Copy Markdown
Member

jaymode commented Apr 8, 2020

The test failure that occurs in this PR is due to a change in how Security works when the license mode is missing. Previously isAuthAllowed would return false in the case of a missing license, so the call in testRestAuthenticationByLicenseType to / would succeed without any credentials. Now that the check is only looking at whether security is enabled, credentials need to be added to the calls to / and /_security/_authenticate or the assertion should change to indicate the response is a 401 authentication challenge. Additionally on line 175, the comment // the default of the licensing tests is basic is no longer correct after 8b0526e and should probably be updated to reflect that the default license state is missing.

@rjernst
Copy link
Copy Markdown
Member Author

rjernst commented Apr 8, 2020

Thanks for the analysis @jaymode! After checking with @tvernum, I've removed the test in question. Since authentication is now allowed at all license levels with this change (it was only disallowed with MISSING, which should just be removed since the behavior should fallback to BASIC), the test no longer makes sense.

@rjernst rjernst merged commit 6efa5c5 into elastic:master Apr 10, 2020
@rjernst rjernst deleted the refactor_license7 branch April 10, 2020 17:41
rjernst added a commit to rjernst/elasticsearch that referenced this pull request Apr 10, 2020
…4547)

The isAuthAllowed() method for license checking is used by code that
wants to ensure security is both enabled and available. The enabled
state is dynamic and provided by isSecurityEnabled(). But since security
is available with all license types, an check on the license level is
not necessary. Thus, this change replaces isAuthAllowed() with calling
isSecurityEnabled().
rjernst added a commit that referenced this pull request Apr 13, 2020
…55082)

The isAuthAllowed() method for license checking is used by code that
wants to ensure security is both enabled and available. The enabled
state is dynamic and provided by isSecurityEnabled(). But since security
is available with all license types, an check on the license level is
not necessary. Thus, this change replaces isAuthAllowed() with calling
isSecurityEnabled().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>refactoring :Security/License License functionality for commercial features v7.8.0 v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants