SOLR-15738: Convert v2 security APIs to annotations by gerlowskija · Pull Request #897 · apache/solr

gerlowskija · 2022-06-07T16:12:50Z

Description

Solr's been in the slow process of moving its v2 APIs away from the
existing apispec/mapping framework towards one that relies on more
explicit annotations to specify API properties. Many security APIs
remain on the "old" framework.

Solution

This commit converts the APIs from the following apispec files to the
"new" framework:

cluster.security.authentication
cluster.security.authorization
cluster.security.BasicAuth.Commands
cluster.security.JwtAuth.Commands
cluster.security.MultiPluginAuth.Commands
cluster.security.RuleBasedAuthorization
cluster.security.authentication.Commands
cluster.security.authorization.Commands

Tests

None yet, but I intend to add some prior to merging.

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.

Solr's been in the slow process of moving its v2 APIs away from the existing apispec/mapping framework towards one that relies on more explicit annotations to specify API properties. This commit converts the APIs from the cluster.security.authentication and cluster.security.authorization apispec files to the "new" framework. Several other security apispec files still remain, including: - cluster.security.BasicAuth.Commands - cluster.security.JwtAuth.Commands - cluster.security.MultiPluginAuth.Commands - cluster.security.RuleBasedAuthorization - cluster.security.authentication.Commands - cluster.security.authorization.Commands The first four entries in the list above are plugin-specific commands for the `POST /cluster/security/[authentication|authorization]` endpoints. The last two entries (c.s.authentication.Commands and c.s.authorization.Commands) are fallbacks for the POST endpoints, to cover the case where no authc/authz plugins are defined. It's unclear whether or not these are needed going forward.

noblepaul · 2022-06-08T00:13:39Z

solr/core/src/java/org/apache/solr/handler/admin/api/GetAuthenticationConfigAPI.java

+/**
+ * V2 API for fetching the authentication section from Solr's security.json configuration.
+ *
+ * <p>This API (GET /v2/cluster/security/authentication) is analogous to the v1 `GET /solr/admin/authentication` API.


let's stick to the /api/* prefix instead of /v2/* prefix ?

Interesting point, I think we have the /v2/ in the other modifications? I checked https://github.com/apache/solr/blob/main/solr/core/src/java/org/apache/solr/handler/admin/api/SchemaBulkModifyAPI.java#L31 for example.

I've always had the slightest preference for /v2, because while v1 and v2 both exist, it's easy to imagine a user getting confused and looking for a v1 endpoint under the /api path, because hey, it is an api after all. But that's very hypothetical and not something I care much about. But if you guys do, I'm happy to change all of these instances to one or the other.

Just lmk which one wins out and I'll do a separate PR to find-replace it everywhere (it's going to be in a ton of places).

Makes sense!

noblepaul · 2022-06-08T00:15:01Z

Do we really need separate classes for specifying the EndPoint ? can we just make them internal to SeciurityConfHandler ?

We should have concrete POJOs that describe the payloads ?

Two apispec files: cluster.security.authentication.Commands and cluster.security.authorization.Commands, are used as fallbacks if no SpecProvider-compatible authc/authz plugin is registered. This commit converts these two stub apispecs into annotations.

gerlowskija · 2022-06-09T17:14:54Z

Do we really need separate classes for specifying the EndPoint ? can we just make them internal to SeciurityConfHandler ?

The API classes can be internal if there's consensus for that, but my personal vote would be that they stay separate.

The current mapping of APIs -> RequestHandlers is based entirely on the v1 API without any consideration of v2. There's no reason in a v2 world for the GET /cluster/aliases API to live in CollectionsHandler.java.
Many of our RequestHandler's are absolute monsters. The more code we can keep out of those files, the better IMO. Particularly since one of my ultimate goals is to see the "meat" of our APIs move from SomeRequestHandler.handleRequestBody to these individual API classes.

We should have concrete POJOs that describe the payloads ?

We do, yep. You didn't see any at the time of your previous comment because I had only pushed up 2 of the GET APIs at that point (which of course don't have payloads). I've updated the PR to cover all of the security APIs, so you should see payload POJOs now if you take another look (e.g. JWTConfigurationPayload, UpdateRuleBasedAuthPermissionPayload, etc.)

noblepaul · 2022-06-10T00:14:02Z

solr/core/src/java/org/apache/solr/api/AnnotatedApi.java

      }
    }

+    public int hashCode() {


don't you think we should cash and reuse the hash ?

Several of the instance variables we use to compute the hashCode here are non-final. I'd bet those vars are unlikely to change in practice, but I wasn't confident enough in that to put in caching here, just in case.

noblepaul · 2022-06-10T00:14:52Z

solr/core/src/java/org/apache/solr/handler/admin/api/DefaultUpdateAuthenticationConfigAPI.java

+      path = {"/cluster/security/authentication"},
+      method = POST,
+      permission = SECURITY_EDIT_PERM)
+  public void updateAuthenticationConfig(SolrQueryRequest req, SolrQueryResponse rsp)


can't we have concrete classes as params ?

Maybe we should, but I'm not sure what they would be. This file is the annotation conversion for cluster.security.authentication.Commands.json, which is the POST api loaded when no authentication plugin is in use.

The old apispec file didn't define any commands, and there are no examples in the ref-guide (afaict) that show what commands are supported in the no-authc-plugin case.

So in short, I only did things this way because (1) I don't know what the concrete classes should be, and (2) this had parity with the apispec, so it seemed at least pass-able.

(This is also the situation on DefaultUpdateAuthorizationConfigAPI below)

Doing a little more digging here, it looks like SecurityConfHandler actually has other logic that will short circuit this case (i.e. a POST to /authentication or /authorization when no plugin is configured) with a 400 (see here).

Afaict, the only purpose these classes (and the apispecs they came from) serve is to make sure users get a helpful error message about "No plugin being configured", rather than the less-helpful 404 users would get if no API was registered at all until a plugin was registered.

This also explains why the original apispecs didn't list any commands: because the API was only a placeholder to produce a comprehensible error message, it never offered actual functionality. I'm going to rename these classes and add Javadocs so this is all clearer. But I don't think I'll be able to change the concrete-payload-class thing, unless there's something I'm still missing?

noblepaul · 2022-06-10T00:15:08Z

solr/core/src/java/org/apache/solr/handler/admin/api/DefaultUpdateAuthorizationConfigAPI.java

+      path = {"/cluster/security/authorization"},
+      method = POST,
+      permission = SECURITY_EDIT_PERM)
+  public void updateAuthorizationConfig(SolrQueryRequest req, SolrQueryResponse rsp)


concrete classes here too?

(See my reply on DefaultUpdateAuthenticationConfigAPI above)

epugh

LGTM. How the auth stuff works is a bit opaque to me, but the conversion aspect makes sense.

gerlowskija · 2022-06-21T15:42:29Z

Alright, thanks for the reviews guys. Gonna merge this and close out the ticket. Only a few more conversions to do!

Solr's been in the slow process of moving its v2 APIs away from the existing apispec/mapping framework towards one that relies on more explicit annotations to specify API properties. This commit converts the APIs from the cluster.security.authentication and cluster.security.authorization apispec files to the "new" framework.

gerlowskija marked this pull request as draft June 7, 2022 16:12

noblepaul reviewed Jun 8, 2022

View reviewed changes

gerlowskija added 11 commits June 8, 2022 07:12

WIP: convert plugin-specific apispecs to annotations

bdd7662

WIP: Ran into a snafu somewhere

434c07b

Tweak to command registration

5fd1a45

Almost there...just RuleBasedAuth to go

aa5b117

Remove RuleBasedAuthorization apispec

fc10186

Package Javadocs and minor bugfix

1cde1ef

Make spotless happy

4cc4a4f

Make check happy, rd 1

eea82c6

Make spotless happy again

8188efe

Remove unwanted logging

75cff46

noblepaul reviewed Jun 10, 2022

View reviewed changes

gerlowskija added 3 commits June 10, 2022 08:08

Tests for core auth plugin APIs

1188318

Test for JWT auth plugin API

b588f0e

A few class renames and some spotless changes

938277f

gerlowskija marked this pull request as ready for review June 10, 2022 14:35

Merge branch 'main' into SOLR-15738-v2-security-annotation-conversion

b9a2fab

epugh approved these changes Jun 11, 2022

View reviewed changes

Small bugfix for v2 api framework

1d907d8

gerlowskija merged commit bf0b2b0 into apache:main Jun 21, 2022

gerlowskija deleted the SOLR-15738-v2-security-annotation-conversion branch June 21, 2022 15:42

Conversation

gerlowskija commented Jun 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Solution

Tests

Checklist

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!

noblepaul commented Jun 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gerlowskija commented Jun 9, 2022

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!

epugh left a comment

Choose a reason for hiding this comment

Uh oh!

gerlowskija commented Jun 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gerlowskija commented Jun 7, 2022 •

edited

Loading

noblepaul commented Jun 8, 2022 •

edited

Loading