----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32559/#review78608 -----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizeHeaderToken.java <https://reviews.apache.org/r/32559/#comment127514> What does this mean? Perhaps the missing context is "<other component x> will do this part". src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizeHeaderToken.java <https://reviews.apache.org/r/32559/#comment127515> Marking this method as deprecated is slightly confusing. I assume it's only because you don't want other code in the project to call it? If that's the case, i suggest you don't use deprecated as the signal and instead docs + code review. src/main/java/org/apache/aurora/scheduler/http/api/security/IniShiroRealmModule.java <https://reviews.apache.org/r/32559/#comment127516> `<p>` src/main/java/org/apache/aurora/scheduler/http/api/security/IniShiroRealmModule.java <https://reviews.apache.org/r/32559/#comment127517> "it" is ambiguous. Consider rewording: "Another filter may still be used for authentication, in which case the ini file can still be used to provide authorization configuration." src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5Realm.java <https://reviews.apache.org/r/32559/#comment127518> comment formatting is off src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5Realm.java <https://reviews.apache.org/r/32559/#comment127521> nonNull? src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5ShiroRealmModule.java <https://reviews.apache.org/r/32559/#comment127525> These are pure magic to me. Please doc. src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5ShiroRealmModule.java <https://reviews.apache.org/r/32559/#comment127524> Why is this `Optional` if it's required? Ditto below. src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5ShiroRealmModule.java <https://reviews.apache.org/r/32559/#comment127589> This is a question of convention - what's the upside to addError/return as opposed to throwing? src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5ShiroRealmModule.java <https://reviews.apache.org/r/32559/#comment127597> Given the behavior i see, i'd prefer a constant and String.format over the resource file and use of StringTemplate. If there are reasons for retaining this complexity, please leave a comment justifying it. src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5ShiroRealmModule.java <https://reviews.apache.org/r/32559/#comment127606> s/jass/jaas/ src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5ShiroRealmModule.java <https://reviews.apache.org/r/32559/#comment127610> Why is this in a `finally`? Seems like any exception is fatal, and there's no more work to do. Use of a finally block makes the expectations of this code confusing. src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5ShiroRealmModule.java <https://reviews.apache.org/r/32559/#comment127526> remove newline src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroKerberosAuthenticationFilter.java <https://reviews.apache.org/r/32559/#comment127617> Please use a consistent exception logging convention - the exception logged on :67 is different. src/test/sh/org/apache/aurora/e2e/test_kerberos_end_to_end.sh <https://reviews.apache.org/r/32559/#comment127627> Please invoke this from the existing end-to-end test script to avoid another qualification step for developers. - Bill Farner On March 27, 2015, 8:50 p.m., Kevin Sweeney wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32559/ > ----------------------------------------------------------- > > (Updated March 27, 2015, 8:50 p.m.) > > > Review request for Aurora, Joshua Cohen and Bill Farner. > > > Bugs: AURORA-812 > https://issues.apache.org/jira/browse/AURORA-812 > > > Repository: aurora > > > Description > ------- > > Support authenticating to the scheduler API with Kerberos. > > > Diffs > ----- > > config/findbugs/excludeFilter.xml 5ff5f87d6bb7d8bf3d9ecb1bc6c6b1a524d1bf15 > examples/vagrant/provision-dev-cluster.sh > ae500436e703140065e5c16fc0e38dbe3214e69f > examples/vagrant/upstart/aurora-scheduler-kerberos.conf PRE-CREATION > > src/main/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityModule.java > ec6a02c4086ee0d5a7529083030d978ea889f677 > > src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizeHeaderToken.java > PRE-CREATION > > src/main/java/org/apache/aurora/scheduler/http/api/security/IniShiroRealmModule.java > 58b559e96402ef282dc0e5ac2de0930906256487 > > src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5Realm.java > PRE-CREATION > > src/main/java/org/apache/aurora/scheduler/http/api/security/Kerberos5ShiroRealmModule.java > PRE-CREATION > > src/main/java/org/apache/aurora/scheduler/http/api/security/KerberosPrincipalParser.java > PRE-CREATION > > src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroKerberosAuthenticationFilter.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroUtils.java > PRE-CREATION > src/main/python/apache/aurora/client/hooks/BUILD > 49d73dfddbcee97ddde1a483f6697865d2566508 > > src/main/resources/org/apache/aurora/scheduler/http/api/security/jaas.conf.st > PRE-CREATION > > src/test/java/org/apache/aurora/scheduler/http/api/security/AuthorizeHeaderTokenTest.java > PRE-CREATION > > src/test/java/org/apache/aurora/scheduler/http/api/security/Kerberos5ShiroRealmModuleTest.java > PRE-CREATION > > src/test/java/org/apache/aurora/scheduler/http/api/security/KerberosPrincipalParserTest.java > PRE-CREATION > > src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroKerberosAuthenticationFilterTest.java > PRE-CREATION > src/test/python/apache/aurora/client/hooks/BUILD > 43f59bc5a2397ad851211e064d83ef6e6de81972 > src/test/sh/org/apache/aurora/e2e/test_kerberos_end_to_end.sh PRE-CREATION > > Diff: https://reviews.apache.org/r/32559/diff/ > > > Testing > ------- > > ./gradlew -Pq build > ./src/test/sh/org/apache/aurora/test_kerberos_end_to_end.sh > > > Thanks, > > Kevin Sweeney > >
