----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review95934 -----------------------------------------------------------
Thanks for addressing the issues raised in the previous review. We are getting closer. I left a few comments (many of them minor style issues). The important question for me is how can we make the authorization logic that is spread over many methods simpler by perhaps introducing some utility methods. I provided a suggestion for one of the cases, but I didn't spend the time to work it out for the `partition` case that is more important (and probably harder). I would be interested in your thoughts as you are more familiar with the code. I'd be happy to spend a bit more time on this to see if I can come up with something, if you prefer. Just let me know. core/src/main/scala/kafka/network/RequestChannel.scala (line 48) <https://reviews.apache.org/r/34492/#comment151127> Normally one would use `Option[Session]` here. Are we using `null` due to efficiency concerns? Sorry if I am missing some context. core/src/main/scala/kafka/security/auth/Acl.scala (line 116) <https://reviews.apache.org/r/34492/#comment151130> Nitpick: no need for `()` and space before `:` should be removed. core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala (line 25) <https://reviews.apache.org/r/34492/#comment151131> Nitpick: space before `:` should be removed. core/src/main/scala/kafka/security/auth/Operation.scala (line 42) <https://reviews.apache.org/r/34492/#comment151129> Generally a good idea to set the result type for public methods. This makes it possible to change the underlying implementation without affecting binary compatibility. For example, here we may set the result type as `Seq[Operation]`, which would give us the option of changing the underlying implementation to `Vector` if that turned out to be better. In `Scala`, `List` is a concrete type unlike `Java`. Not sure what is the usual policy for Kafka though, would be useful to have some input from Jun. If we decide to change it, there are other places where the same comment would apply. core/src/main/scala/kafka/security/auth/Resource.scala (line 24) <https://reviews.apache.org/r/34492/#comment151132> Nitpick: space before `:` should be removed. core/src/main/scala/kafka/server/KafkaApis.scala (line 101) <https://reviews.apache.org/r/34492/#comment151134> This code exists in 4 places, how about we introduce an method like: ``` def authorizeClusterAction(authorizer, request): Unit = { if (authorizer.map(_.authorizer(request.session, ClusterAction, Resource.ClusterResource)).getOrElse(false)) throw new AuthorizationException(s"Request $request is not authorized.") } ``` And then callers can just do (as an example): `authorizeClusterAction(authorizer, leaderAndIsrRequest)` Am I missing something? core/src/main/scala/kafka/server/KafkaApis.scala (lines 186 - 189) <https://reviews.apache.org/r/34492/#comment151135> Nitpick: space after `case`. There are a number of other cases like this. core/src/main/scala/kafka/server/KafkaApis.scala (line 282) <https://reviews.apache.org/r/34492/#comment151140> We have a lot of these `partition` calls like this. Is there no way to extract a utility method so that the call is simpler and there is less duplication? core/src/main/scala/kafka/server/KafkaApis.scala (line 545) <https://reviews.apache.org/r/34492/#comment151141> Nitpick: val instead of var. core/src/test/scala/unit/kafka/security/auth/AclTest.scala (line 24) <https://reviews.apache.org/r/34492/#comment151142> We don't use `JUnit3Suite` anymore. Either use `JUnitSuite` or don't inherit from anything (we have both examples in the codebase now). This applies to all the tests. I noticed that Jun already mentioned this in another test. core/src/test/scala/unit/kafka/security/auth/AclTest.scala (line 30) <https://reviews.apache.org/r/34492/#comment151143> @Test annotation is needed after you remove `JUnit3Suite`. This applies to all the tests. - Ismael Juma On Aug. 11, 2015, 1:32 a.m., Parth Brahmbhatt wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34492/ > ----------------------------------------------------------- > > (Updated Aug. 11, 2015, 1:32 a.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. > > > 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/network/RequestChannel.scala > 20741281dcaa76374ea6f86a2185dad27b515339 > 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 > 7ea509c2c41acc00430c74e025e069a833aac4e7 > core/src/main/scala/kafka/server/KafkaConfig.scala > dbe170f87331f43e2dc30165080d2cb7dfe5fdbf > core/src/main/scala/kafka/server/KafkaServer.scala > 84d4730ac634f9a5bf12a656e422fea03ad72da8 > 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/KafkaConfigConfigDefTest.scala > PRE-CREATION > > Diff: https://reviews.apache.org/r/34492/diff/ > > > Testing > ------- > > > Thanks, > > Parth Brahmbhatt > >
