> On Aug. 20, 2015, 1:07 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/security/auth/Acl.scala, lines 84-87 > > <https://reviews.apache.org/r/34492/diff/10/?file=1037027#file1037027line84> > > > > Do we need the case match here since acls is always a Set?
Fixed. > On Aug. 20, 2015, 1:07 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/security/auth/ResourceType.scala, line 21 > > <https://reviews.apache.org/r/34492/diff/10/?file=1037033#file1037033line21> > > > > Not needed. Fixed. > On Aug. 20, 2015, 1:07 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/server/KafkaApis.scala, line 104 > > <https://reviews.apache.org/r/34492/diff/10/?file=1037034#file1037034line104> > > > > If authorizer is not specified, getOrElse() should return true, right? > > There are a few other places like that. Fixed. > On Aug. 20, 2015, 1:07 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/server/KafkaApis.scala, lines 189-192 > > <https://reviews.apache.org/r/34492/diff/10/?file=1037034#file1037034line189> > > > > For coding style, to be consistent with most existing code, it seems > > it's better to do authorizer.map{ ... }.getOrElse(). Done. > On Aug. 20, 2015, 1:07 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/server/KafkaApis.scala, lines 641-644 > > <https://reviews.apache.org/r/34492/diff/10/?file=1037034#file1037034line641> > > > > The current logic requires that we grant CREATE on CLUSTER to the > > consumer, which is a bit weird. Perhaps we should just always allow the > > consumer to create the offset topic as long as it has the permission to > > read the topic and the consumer group. That way, we don't have to grant > > CREATE permssion to the consumer. Done. > On Aug. 20, 2015, 1:07 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/server/KafkaApis.scala, line 677 > > <https://reviews.apache.org/r/34492/diff/10/?file=1037034#file1037034line677> > > > > It seems the test on errorCode == ErrorMapping.NoError is unnecessary. The code tries to priortize non authoirzation errors above authorization error. We can only send one error code and IMO if we have an error other than Authorization error we should propogate that to the user. > On Aug. 20, 2015, 1:07 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/server/KafkaApis.scala, line 553 > > <https://reviews.apache.org/r/34492/diff/10/?file=1037034#file1037034line553> > > > > Currently, metadataCache.getTopicMetadata() will return all the > > metadata of all topics if the input topic is empty. This causes a couple of > > issues here. (1) If authorizedTopics is empty, we end up returning more > > topics than needed. (2) If the original request has an empty topic list, we > > will return the metadata of all topics whether the client has the DESCRIBE > > permission or not. Fixed. > On Aug. 20, 2015, 1:07 a.m., Jun Rao wrote: > > core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala, line > > 30 > > <https://reviews.apache.org/r/34492/diff/10/?file=1037042#file1037042line30> > > > > We removed this class in KAFKA-2288 since it's no longer necessary. Removed the class. - Parth ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review95801 ----------------------------------------------------------- On Aug. 20, 2015, 6:27 p.m., Parth Brahmbhatt wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34492/ > ----------------------------------------------------------- > > (Updated Aug. 20, 2015, 6:27 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-2210 > https://issues.apache.org/jira/browse/KAFKA-2210 > > > Repository: kafka > > > Description > ------- > > Addressing review comments from Jun. > > > Adding CREATE check for offset topic only if the topic does not exist already. > > > Addressing some more comments. > > > Removing acl.json file > > > Moving PermissionType to trait instead of enum. Following the convention for > defining constants. > > > Adding authorizer.config.path back. > > > Addressing more comments from Jun. > > > Addressing more comments. > > > Now addressing Ismael's comments. Case sensitive checks. > > > Addressing Jun's comments. > > > Merge remote-tracking branch 'origin/trunk' into az > > Conflicts: > core/src/main/scala/kafka/server/KafkaApis.scala > core/src/main/scala/kafka/server/KafkaServer.scala > > Deleting KafkaConfigDefTest > > > Addressing comments from Ismael. > > > Diffs > ----- > > core/src/main/scala/kafka/api/OffsetRequest.scala > f418868046f7c99aefdccd9956541a0cb72b1500 > core/src/main/scala/kafka/common/AuthorizationException.scala PRE-CREATION > core/src/main/scala/kafka/common/ErrorMapping.scala > c75c68589681b2c9d6eba2b440ce5e58cddf6370 > core/src/main/scala/kafka/security/auth/Acl.scala PRE-CREATION > core/src/main/scala/kafka/security/auth/Authorizer.scala PRE-CREATION > core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala PRE-CREATION > core/src/main/scala/kafka/security/auth/Operation.scala PRE-CREATION > core/src/main/scala/kafka/security/auth/PermissionType.scala PRE-CREATION > core/src/main/scala/kafka/security/auth/Resource.scala PRE-CREATION > core/src/main/scala/kafka/security/auth/ResourceType.scala PRE-CREATION > core/src/main/scala/kafka/server/KafkaApis.scala > 67f0cad802f901f255825aa2158545d7f5e7cc3d > core/src/main/scala/kafka/server/KafkaConfig.scala > d547a01cf7098f216a3775e1e1901c5794e1b24c > core/src/main/scala/kafka/server/KafkaServer.scala > 0e7ba3ede78dbc995c404e0387a6be687703836a > core/src/test/scala/unit/kafka/security/auth/AclTest.scala PRE-CREATION > core/src/test/scala/unit/kafka/security/auth/KafkaPrincipalTest.scala > PRE-CREATION > core/src/test/scala/unit/kafka/security/auth/OperationTest.scala > PRE-CREATION > core/src/test/scala/unit/kafka/security/auth/PermissionTypeTest.scala > PRE-CREATION > core/src/test/scala/unit/kafka/security/auth/ResourceTypeTest.scala > PRE-CREATION > core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala > 3da666f73227fc7ef7093e3790546344065f6825 > > Diff: https://reviews.apache.org/r/34492/diff/ > > > Testing > ------- > > > Thanks, > > Parth Brahmbhatt > >
