Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[fix][admin] Fix producer/consume permission can’t get schema #15956

Merged
merged 3 commits into from
Jun 9, 2022
Merged

[fix][admin] Fix producer/consume permission can’t get schema #15956

merged 3 commits into from
Jun 9, 2022

Conversation

Technoboy-
Copy link
Contributor

@Technoboy- Technoboy- commented Jun 7, 2022 •
edited
Loading

Motivation

Currently, we need admin permissions to operate the schema API. This is because the admin permission was defined when the schema API was first added. See #1381.
Later, then adding authentication granularity with #6428, we don't change the schema API part. So leave the admin permission today.

But the binary protocol allows the produce/consume permission to get the schema, so change the related method permission to produce/consume.

Modifications

  • get schema need GET_METADATA permission

Documentation

  • doc-not-needed
    (Please explain why)

@Technoboy- Technoboy- self-assigned this Jun 7, 2022
@Technoboy- Technoboy- added this to the 2.11.0 milestone Jun 7, 2022
@Technoboy- Technoboy- added area/admin doc-not-needed Your PR changes do not impact docs labels Jun 7, 2022
@Technoboy- Technoboy- marked this pull request as ready for review June 7, 2022 04:56
@nodece
Copy link
Member

nodece commented Jun 7, 2022

I suggest introduce the GET_SCHEMA in org.apache.pulsar.common.policies.data.TopicOperation.

gaoran10
gaoran10 reviewed Jun 8, 2022
View reviewed changes
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiSchemaWithAuthTest.java
}

@Test
public void testGetCreateDeleteSchema() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a trivial suggestion, maybe we could add a data provider to test different cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, because there are tests to cover no auth case, so this test only enables auth to test the right permission.

@gaoran10
Copy link
Contributor

gaoran10 commented Jun 8, 2022 •
edited
Loading

@nodece It seems that the PRODUCE and CONSUME should contain the GET_SCHEMA permission, or else PRODUCE and CONSUME permission couldn't work well, or users need to grant PRODUCE and GET_SCHEMA permissions at the same time, then they could produce messages.

@Technoboy-
Copy link
Contributor Author

@nodece It seems that the PRODUCE and CONSUME should contain the GET_SCHEMA permission, or else PRODUCE and CONSUME permission couldn't work well, or users need to grant PRODUCE and GET_SCHEMA permissions at the same time, then they could produce messages.

yes, right. So use GET_METADATA instead. Because the user has produce or consume permission can also get the schema...

@nodece
Copy link
Member

nodece commented Jun 8, 2022

OK

gaoran10
gaoran10 approved these changes Jun 8, 2022
View reviewed changes
Copy link
Contributor

@gaoran10 gaoran10 left a comment

Choose a reason for hiding this comment

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

LGTM

nodece
nodece reviewed Jun 8, 2022
View reviewed changes
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiSchemaWithAuthTest.java Outdated Show resolved Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiSchemaWithAuthTest.java Outdated Show resolved Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiSchemaWithAuthTest.java Outdated Show resolved Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiSchemaWithAuthTest.java Outdated Show resolved Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiSchemaWithAuthTest.java Outdated Show resolved Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiSchemaWithAuthTest.java Outdated Show resolved Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiSchemaWithAuthTest.java Outdated Show resolved Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiSchemaWithAuthTest.java Outdated Show resolved Hide resolved
@Technoboy- Technoboy- merged commit 91fe3b2 into apache:master Jun 9, 2022
@eolivelli
Copy link
Contributor

This is an important security related change.
Did I miss some discussion on dev@ ?

@nicoloboschi nicoloboschi added the release/important-notice The changes which are important should be mentioned in the release note label Jun 9, 2022
@codelipenghui
Copy link
Contributor

@eolivelli It should be a bug. From the binary protocol, the produce and consume permission can get schema, but the REST API can't.

And, only the tenant admin can get schema is unreasonable. It looks like the PR fixed a BUG that Have the produce/consume permission but not able to get schema.

@Technoboy- Technoboy- changed the title [modify][admin] Change the permissions of the schema API from Admin to normal produce/consume [fix][admin] Fix producer/consume permission can’t get schema Jun 10, 2022
@yuruguo
Copy link
Contributor

yuruguo commented Jun 10, 2022 •
edited
Loading

@Technoboy- @eolivelli @codelipenghui This pr should be related to issue12419 and the problem to be solved is that role has the lookup topic permission should also have the get permission of the topic schema.

@yuruguo yuruguo mentioned this pull request Jun 11, 2022
Merged
1 task
Technoboy- added a commit to Technoboy-/pulsar that referenced this pull request Jun 13, 2022
@Technoboy-
[fix][admin] Fix producer/consume permission can’t get schema (apache…
b1dc876 
…#15956)
@Technoboy- Technoboy- mentioned this pull request Jun 13, 2022
Merged
1 task
codelipenghui pushed a commit that referenced this pull request Jun 13, 2022
@Technoboy-
[fix][admin] Fix producer/consume permission can’t get schema (#15956) (
f3b4e86 
#16026)

Cherry-pick #15956.
### Motivation
Currently, we need admin permissions to operate the schema API. This is because the admin permission was defined when the schema API was first added. See #1381.
Later, then adding authentication granularity with #6428, we don't change the schema API part.  So leave the admin permission today.

But the binary protocol allows the produce/consume permission to get the schema, so change the related method permission to `produce/consume`.
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jun 13, 2022
@Technoboy- @nicoloboschi
[fix][admin] Fix producer/consume permission can’t get schema (apache…
790ee2f 
…#15956) (apache#16026)

Cherry-pick apache#15956.
### Motivation
Currently, we need admin permissions to operate the schema API. This is because the admin permission was defined when the schema API was first added. See apache#1381.
Later, then adding authentication granularity with apache#6428, we don't change the schema API part.  So leave the admin permission today.

But the binary protocol allows the produce/consume permission to get the schema, so change the related method permission to `produce/consume`.

(cherry picked from commit f3b4e86)
@Technoboy- Technoboy- deleted the change-schema-api-permission branch August 10, 2022 05:52
@nodece
Copy link
Member

nodece commented Sep 15, 2022

This PR also needs to cherry-pick to branch-2.9.

@Technoboy- Technoboy- mentioned this pull request Sep 20, 2022
Merged
1 task
@Technoboy- Technoboy- added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Sep 20, 2022
@Technoboy-
Copy link
Contributor Author

This PR also needs to cherry-pick to branch-2.9.

Done.

@Technoboy- Technoboy- mentioned this pull request Sep 30, 2022
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@congbobo184 congbobo184 congbobo184 approved these changes

@nodece nodece nodece approved these changes

@gaoran10 gaoran10 gaoran10 approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@mattisonchao mattisonchao Awaiting requested review from mattisonchao

Assignees

@Technoboy- Technoboy-

Labels
area/admin cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 doc-not-needed Your PR changes do not impact docs release/important-notice The changes which are important should be mentioned in the release note release/2.9.4 release/2.10.1
Projects
None yet
Milestone
2.11.0
Development

Successfully merging this pull request may close these issues.
9 participants
@Technoboy- @nodece @gaoran10 @eolivelli @codelipenghui @yuruguo @congbobo184 @mattisonchao @nicoloboschi