fix: Change auth from WRITE to READ for specGetAll

[aho135]

Description

The current implementation of specGetAll is unnecessarily restrictive as it uses DATASOURCE_WRITE_RA_GENERATOR for authorization. This PR changes the Authorization to only require READ. This issue was surfaced after testing out the endpoint with the Read Only Authorizer enabled. Since the history endpoint already returns the list of all Supervisors with Read access this change makes the authorization consistent across the two endpoints

Fixed the bug ...

specGetAll returns an empty list for Supervisors when read only authorization is enabled

Release note

---

Key changed/added classes in this PR

• SupervisorResource
• SupervisorResourceTest

---

This PR has:

• been self-reviewed.
• added documentation for new or modified features or behaviors.
• a release note entry in the PR description.
• added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
• added or updated version, license, or notice information in licenses.yaml
• added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
• added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
• added integration tests.
• been tested in a test Druid cluster.

[gianm]

I've often believed that WRITE is appropriate for permissions like this, on the rationale that the ingestion APIs should only be accessible to people with ability to manipulate ingestion objects, which of course requires WRITE permission. I guess your mental model is different, one where READ permission means the user can see ingestion objects but not necessary manipulate them. I wish that the permission model was more fine grained so we could separate READ of the data from READ of the ingestion objects.

I wonder, what's the current state of things? What authorization do other read-only APIs in /druid/indexer/v1/... require? What do the system tables sys.tasks and sys.supervisors check?

[aho135]

I've often believed that WRITE is appropriate for permissions like this, on the rationale that the ingestion APIs should only be accessible to people with ability to manipulate ingestion objects, which of course requires WRITE permission. I guess your mental model is different, one where READ permission means the user can see ingestion objects but not necessary manipulate them. I wish that the permission model was more fine grained so we could separate READ of the data from READ of the ingestion objects.

I wonder, what's the current state of things? What authorization do other read-only APIs in /druid/indexer/v1/... require? What do the system tables sys.tasks and sys.supervisors check?

Thanks for the review @gianm!

I wish that the permission model was more fine grained so we could separate READ of the data from READ of the ingestion objects.

I believe @clintropolis was working on something similar to this with the addition of a Policy in the Authorizer to allow more granular control over table reads

I wonder, what's the current state of things? What authorization do other read-only APIs in /druid/indexer/v1/... require?

These API's also follow the model where users can READ all ingestion objects, but manipulation still requires WRITE. For example, task submission requires write, but /task/{taskid}/status only requires READ. Even for system tables those only require READ access, so I think the changes in this PR at least make things consistent with the current state of things

[aho135]

@gianm Please let me know your thoughts on this one when you get the chance. Thanks!

[abhishekrb19]

These API's also follow the model where users can READ all ingestion objects, but manipulation still requires WRITE. For example, task submission requires write, but /task/{taskid}/status only requires READ. Even for system tables those only require READ access, so I think the changes in this PR at least make things consistent with the current state of things

This seems reasonable to me as the updated access model is internally consistent with the other existing GET APIs noted

[abhishekrb19]

Just confirming that applying a transform directly on the optional supervisorSpecOptional won’t cause any issues without checking .isPresent(). It seems like it won’t, but I noticed this differs from the logic in filterAuthorizedSupervisorIds()

[abhishekrb19]

What do you think about adding a function like filterAuthorizedSupervisorIdsForRead() to make read access explicit? So any new read-only APIs know to call this function instead of filterAuthorizedSupervisorIds()

[aho135]

Thanks for the review @abhishekrb19! Yeah that was kind of a roundabout way of doing things. I made an update to filterAuthorizedSupervisorIds to pass in the Authorization function and it looks cleaner now. Lmk what you think!

[abhishekrb19]

Thanks @aho135. Since #19243 is being included in 37.0.0, should we backport this fix to 37.0.0 as well?
cc: @cecemei

[aho135]

Good call @abhishekrb19 Created #19362 for backporting!

[cecemei]

Thanks @aho135. Since #19243 is being included in 37.0.0, should we backport this fix to 37.0.0 as well? cc: @cecemei

The underlying issue seems to exist for a while, but only just surfaced with ‎ReadOnlyAuthorizer? I’m not entirely sure a backport is necessary here.

[aho135]

@cecemei Yeah this issue only surfaces with ReadOnlyAuthorizer, or any custom Authorizer which does not grant WRITE access. It's a minor bug fix, so it need not disrupt the release flow