----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34492/#review86267 -----------------------------------------------------------
core/src/main/scala/kafka/security/auth/Acl.scala <https://reviews.apache.org/r/34492/#comment138201> Do you mean example json or the zk Path? I have added the example json here I think the zk path should not be mentioned here as that is specific to default implementation. core/src/main/scala/kafka/security/auth/Acl.scala <https://reviews.apache.org/r/34492/#comment138200> updated. core/src/main/scala/kafka/security/auth/Acl.scala <https://reviews.apache.org/r/34492/#comment138202> updated. core/src/main/scala/kafka/security/auth/Acl.scala <https://reviews.apache.org/r/34492/#comment138203> I tried exactly that but it tunrs out our current Json parser does not work when a json string has other special characters, somehow gets into some double parsing and fails. Has been long since I wrote this code so dont exactly remember why it was failing but I did try it and with current JsonUtil it does not work. core/src/main/scala/kafka/security/auth/Acl.scala <https://reviews.apache.org/r/34492/#comment138205> I wanted to keep the equality check case insensitive for things like hosts which will be specified using CLI by users and its easy to make mistakes on CLI which is why I did not go with case class. If you disagree that this is improtatn I can fall back to case class. core/src/main/scala/kafka/security/auth/Acl.scala <https://reviews.apache.org/r/34492/#comment138206> Its used for both encode and decode. It gets encoded when we write to zookeeper and decoded when we read from it. As I mentioned above there was some double parsing invovled but do not remember the exact details now. core/src/main/scala/kafka/security/auth/Authorizer.scala <https://reviews.apache.org/r/34492/#comment138207> In the KIP dicussion it was proposed to add a config authoizer.config.path which will contain path to a property files on all broker hosts. This is how the plugin specific property file gets passed on. Do we want to instead use configurable? core/src/main/scala/kafka/security/auth/Authorizer.scala <https://reviews.apache.org/r/34492/#comment138209> Updated the java doc. core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala <https://reviews.apache.org/r/34492/#comment138210> I haven't added Group support yet but they will be of the form Group:<group-name>. Why did you get the impression that groups will not have ":" core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala <https://reviews.apache.org/r/34492/#comment138211> I am sorry I should have read the styling guide more carefully. Updated. core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala <https://reviews.apache.org/r/34492/#comment138212> Yes we can and as mentioned in the design doc when no authentication is configured it will be set as User:DrWho?. core/src/main/scala/kafka/security/auth/KafkaPrincipal.scala <https://reviews.apache.org/r/34492/#comment138214> Added a null check as part of object construction. Again I made all the equality check case insensitive as these will be specified on CLI by users and are easy to get incorrect. If you think this is a potential security hole given most systems have user/principal names as case sensitive I can change this to a case class. core/src/main/scala/kafka/security/auth/Operation.java <https://reviews.apache.org/r/34492/#comment138220> I grepped through kafka code base to understand how enums were used in other parts and all places used java enums. I assumed that was the convention . If that is not the case I can change all enum classes in core to use http://www.scala-lang.org/api/current/index.html#scala.Enumeration core/src/main/scala/kafka/security/auth/Resource.scala <https://reviews.apache.org/r/34492/#comment138223> Done. core/src/main/scala/kafka/security/auth/Resource.scala <https://reviews.apache.org/r/34492/#comment138224> Done. core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/34492/#comment138225> Done. core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/34492/#comment138226> I originally had this as a write request for each topic partition , later I realized this API is only called by controller but forgot to update the code accordingly. Fixed. core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/34492/#comment138250> I think we agreed in KIP that in order to commit offset the client needs read permission on Topic and READ permission on GROUP.Are you suggesting we should not check for topic READ access? core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/34492/#comment138256> Done. core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/34492/#comment138257> Fixed. core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/34492/#comment138259> Fixed. core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/34492/#comment138260> Yes, Fixed. core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/34492/#comment138277> This TODO is misplaced , I have added the create check in handleTopicMetadataRequest. The create here only creates OffsetManager.OffsetsTopicName aka __consumer_offsets. I am assuming __consumer_offsets should be created by brokers when they issue updateMetaDataRequest so adding a create check here as well which means any consumer that tries to consumer before __consumer_offsets is created will get authorization exception unless an explicit CREATE is granted to them on __consumer_offsets topic. we should probably disucss if its ok to allow anyone to create this special topic in absence of which consumers can fail. core/src/main/scala/kafka/server/KafkaConfig.scala <https://reviews.apache.org/r/34492/#comment138278> Not suppose to be part of this PR. Removed. core/src/test/scala/unit/kafka/security/auth/AclTest.scala <https://reviews.apache.org/r/34492/#comment138526> can you elloborate why do you think that is a better approach? core/src/test/scala/unit/kafka/security/auth/AclTest.scala <https://reviews.apache.org/r/34492/#comment138528> Yes, lets decide if we want to get rid of case insensitive checks for hosts and I can remove this test by making acl a case class. core/src/test/scala/unit/kafka/security/auth/ResourceTest.scala <https://reviews.apache.org/r/34492/#comment138523> Same rationale as mentioned few times before for case senstivity. - Parth Brahmbhatt On June 3, 2015, 11:36 p.m., Parth Brahmbhatt wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34492/ > ----------------------------------------------------------- > > (Updated June 3, 2015, 11:36 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. > > > Diffs > ----- > > core/src/main/scala/kafka/api/OffsetRequest.scala > 3d483bc7518ad76f9548772522751afb4d046b78 > 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.java PRE-CREATION > core/src/main/scala/kafka/security/auth/PermissionType.java PRE-CREATION > core/src/main/scala/kafka/security/auth/Resource.scala PRE-CREATION > core/src/main/scala/kafka/security/auth/ResourceType.java PRE-CREATION > core/src/main/scala/kafka/server/KafkaApis.scala > 387e387998fc3a6c9cb585dab02b5f77b0381fbf > core/src/main/scala/kafka/server/KafkaConfig.scala > 6f25afd0e5df98258640252661dee271b1795111 > core/src/main/scala/kafka/server/KafkaServer.scala > e66710d2368334ece66f70d55f57b3f888262620 > core/src/test/resources/acl.json PRE-CREATION > 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/ResourceTest.scala > PRE-CREATION > core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala > 71f48c07723e334e6489efab500a43fa93a52d0c > > Diff: https://reviews.apache.org/r/34492/diff/ > > > Testing > ------- > > > Thanks, > > Parth Brahmbhatt > >
