Skip to content

Conversation

@yuruguo
Copy link
Contributor

@yuruguo yuruguo commented Jan 30, 2022

Motivation

In #13297, We should not add GET_SCHEMA_COMPATIBILITY_STRATEGY and SET_SCHEMA_COMPATIBILITY_STRATEGY topic operation and complete the permission check on policy schemaCompatibilityStrategy by method validateTopicOperationAsync(), because schemaCompatibilityStrategy is a topic policy not a topic operation, we should use method validateTopicPolicyOperation() to complete.

Documentation

  • no-need-doc

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 30, 2022
@yuruguo yuruguo requested a review from Jason918 February 2, 2022 02:26
Copy link
Contributor

@Jason918 Jason918 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@nodece nodece left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Lgtm

CN you please remember me the PIP in which we are changing all the permissions?

@nodece
Copy link
Member

nodece commented Feb 5, 2022

Lgtm

CN you please remember me the PIP in which we are changing all the permissions?

@eolivelli Sorry, I should make a PIP for #13297. Do we need to make PIR for these changes?

@eolivelli
Copy link
Contributor

eolivelli commented Feb 6, 2022

I mean that during the past months I have seen many patches around permission validation.
I wonder if there is a general umbrella issue or some PIP.
This kind of changes must be documented carefully in the release notes, because they have a big impact on security

@gaoran10
Copy link
Contributor

gaoran10 commented Feb 7, 2022

Could we add a test for this?

@codelipenghui codelipenghui added this to the 2.10.0 milestone Feb 7, 2022
@codelipenghui
Copy link
Contributor

@yuruguo The change looks good to me, could you please help add a test?

@codelipenghui
Copy link
Contributor

@eolivelli We don't have a release that contains #13297, so it will not have a big impact on security. The issue is #13297 implementation does not follow the current topic policy permission handling way.

I wonder if there is a general umbrella issue or some PIP.

😭, looks this is a feature #6428 that was added almost 2 years ago, and no PIP there.

@yuruguo
Copy link
Contributor Author

yuruguo commented Feb 8, 2022

@codelipenghui @315157973 @gaoran10 The test has added and PTAL again, thx!

@codelipenghui
Copy link
Contributor

After checking the current implementation, looks like the PolicyName and PolicyOperation are only for the exception messages, only the tenant admin can access the topic policies.

@codelipenghui codelipenghui merged commit 4886528 into apache:master Feb 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/security doc-not-needed Your PR changes do not impact docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants