----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66610/#review201620 -----------------------------------------------------------
core/src/main/java/org/apache/oozie/service/AuthorizationService.java Lines 9-11 (original), 9-11 (patched) <https://reviews.apache.org/r/66610/#comment282947> Please don't change license header - it'll cause RAT errors. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Lines 97 (patched) <https://reviews.apache.org/r/66610/#comment282949> Would be nice to have an explanation for each level. `READ_ALL_WRITE_OWN`, and `READ_WRITE_OWN` would be better names. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Lines 102 (patched) <https://reviews.apache.org/r/66610/#comment282952> Let's have a method that calls `ConfigurationService` every time instead. I had to think whether this would cause threading related errors - it wouldn't, but still cleaner to have a method than a caching, non-final field. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Lines 121-123 (patched) <https://reviews.apache.org/r/66610/#comment283016> Let's have a method that calls `ConfigurationService` every time instead. I had to think whether this would cause threading related errors - it wouldn't, but still cleaner to have a method than a caching, non-final field. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Lines 136 (patched) <https://reviews.apache.org/r/66610/#comment282953> Please remove dead code. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Line 139 (original), 156 (patched) <https://reviews.apache.org/r/66610/#comment282954> Leave old formatting. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Lines 163 (patched) <https://reviews.apache.org/r/66610/#comment282955> Leave old formatting. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Line 196 (original), 226 (patched) <https://reviews.apache.org/r/66610/#comment282956> Leave old formatting. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Line 200 (original), 229 (patched) <https://reviews.apache.org/r/66610/#comment282957> Leave old formatting. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Lines 234 (patched) <https://reviews.apache.org/r/66610/#comment283017> I'd use `INFO` level instead. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Line 209 (original), 243 (patched) <https://reviews.apache.org/r/66610/#comment282958> Leave old formatting. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Line 241 (original), 275 (patched) <https://reviews.apache.org/r/66610/#comment282959> Leave old formatting. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Line 272 (original), 305 (patched) <https://reviews.apache.org/r/66610/#comment282960> Leave old formatting. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Line 289 (original), 321 (patched) <https://reviews.apache.org/r/66610/#comment282961> Leave old formatting. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Lines 366-377 (patched) <https://reviews.apache.org/r/66610/#comment282969> Maybe a [`Predicate`](http://google.github.io/guava/releases/19.0/api/docs/com/google/common/base/Predicate.html) would be nicer to read. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Lines 375 (patched) <https://reviews.apache.org/r/66610/#comment282962> Remove empty line. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Line 369 (original), 411 (patched) <https://reviews.apache.org/r/66610/#comment282963> Leave old formatting. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Line 374 (original), 415 (patched) <https://reviews.apache.org/r/66610/#comment282964> Leave old formatting. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Line 378 (original), 418 (patched) <https://reviews.apache.org/r/66610/#comment282965> Leave old formatting. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Line 424 (original), 463 (patched) <https://reviews.apache.org/r/66610/#comment282966> Leave old formatting. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Line 429 (original), 467 (patched) <https://reviews.apache.org/r/66610/#comment282967> Leave old formatting. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Line 433 (original), 470 (patched) <https://reviews.apache.org/r/66610/#comment282968> Leave old formatting. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Lines 507-509 (patched) <https://reviews.apache.org/r/66610/#comment283023> Unclear to me when `IOException` can occur. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Lines 513-524 (patched) <https://reviews.apache.org/r/66610/#comment282970> Maybe a [`Predicate`](http://google.github.io/guava/releases/19.0/api/docs/com/google/common/base/Predicate.html) would be nicer to read. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Lines 521 (patched) <https://reviews.apache.org/r/66610/#comment282971> Remove newline. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Lines 523 (patched) <https://reviews.apache.org/r/66610/#comment282972> Remove newline. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Lines 467-485 (original), 526-544 (patched) <https://reviews.apache.org/r/66610/#comment283020> Should be extracted to a `private static final class Authorizator<B>`. That would accept as type parameter `CoordinatorJobBean`, `BundleJobBean`, or `WorkflowJobBean`. Constructor parameters like `? extends QueryExecutor<B>`, and `jobId`, would also be used. In that case all we would need is to have an `authorize(final String userName)` method. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Line 471 (original), 532 (patched) <https://reviews.apache.org/r/66610/#comment282973> Leave old formatting. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Line 475 (original), 535 (patched) <https://reviews.apache.org/r/66610/#comment282974> Leave old formatting. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Line 482 (original), 541 (patched) <https://reviews.apache.org/r/66610/#comment282975> Isn't another error code an incompatible change here? How is this `AuthorizationException` wrapped when called from `OozieCLI`, from UI, or from a REST servlet? core/src/main/java/org/apache/oozie/service/AuthorizationService.java Lines 487-507 (original), 546-564 (patched) <https://reviews.apache.org/r/66610/#comment283019> Should be extracted to a `private static final class Authorizator<B>`. That would accept as type parameter `CoordinatorJobBean`, `BundleJobBean`, or `WorkflowJobBean`. Constructor parameters like `? extends QueryExecutor<B>`, and `jobId`, would also be used. In that case all we would need is to have an `authorize(final String userName)` method. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Line 493 (original), 552 (patched) <https://reviews.apache.org/r/66610/#comment282976> Leave old formatting. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Line 497 (original), 555 (patched) <https://reviews.apache.org/r/66610/#comment282977> Leave old formatting. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Lines 509-534 (original), 566-583 (patched) <https://reviews.apache.org/r/66610/#comment283018> Should be extracted to a `private static final class Authorizator<B>`. That would accept as type parameter `CoordinatorJobBean`, `BundleJobBean`, or `WorkflowJobBean`. Constructor parameters like `? extends QueryExecutor<B>`, and `jobId`, would also be used. In that case all we would need is to have an `authorize(final String userName)` method. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Line 519 (original), 575 (patched) <https://reviews.apache.org/r/66610/#comment282978> Leave old formatting. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Line 526 (original), 581 (patched) <https://reviews.apache.org/r/66610/#comment282979> Isn't another error code an incompatible change here? How is this `AuthorizationException` wrapped when called from `OozieCLI`, from UI, or from a REST servlet? core/src/main/java/org/apache/oozie/service/AuthorizationService.java Line 560 (original), 609 (patched) <https://reviews.apache.org/r/66610/#comment282980> Leave old formatting. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Line 564 (original), 612 (patched) <https://reviews.apache.org/r/66610/#comment282981> Leave old formatting. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Line 585 (original), 632 (patched) <https://reviews.apache.org/r/66610/#comment282982> Leave old formatting. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Line 589 (original), 635 (patched) <https://reviews.apache.org/r/66610/#comment282983> Leave old formatting. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Line 610 (original), 655 (patched) <https://reviews.apache.org/r/66610/#comment282984> Leave old formatting. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Line 614 (original), 658 (patched) <https://reviews.apache.org/r/66610/#comment282985> Leave old formatting. core/src/main/java/org/apache/oozie/service/AuthorizationService.java Line 627 (original), 670 (patched) <https://reviews.apache.org/r/66610/#comment282986> Leave old formatting. Unclear to me when `IOException` can occur. core/src/main/java/org/apache/oozie/servlet/BaseAdminServlet.java Lines 123 (patched) <https://reviews.apache.org/r/66610/#comment282987> Extract to local variable. core/src/main/java/org/apache/oozie/servlet/BaseAdminServlet.java Line 178 (original), 180 (patched) <https://reviews.apache.org/r/66610/#comment282988> Extract to local variable. core/src/main/java/org/apache/oozie/servlet/BaseAdminServlet.java Lines 342-350 (original), 345-353 (patched) <https://reviews.apache.org/r/66610/#comment283024> Extract method `wrapAuthorize()` that takes a `Callable`. core/src/main/java/org/apache/oozie/servlet/BaseJobServlet.java Lines 73 (patched) <https://reviews.apache.org/r/66610/#comment282989> Extract to local variable. core/src/main/java/org/apache/oozie/servlet/BaseJobServlet.java Line 289 (original), 284 (patched) <https://reviews.apache.org/r/66610/#comment282990> Extract to local variable. core/src/main/java/org/apache/oozie/servlet/BaseJobServlet.java Lines 374-381 (patched) <https://reviews.apache.org/r/66610/#comment282991> Extract method `wrapAuthorize()` that takes a `Callable`. core/src/main/java/org/apache/oozie/servlet/SLAServlet.java Lines 172-179 (patched) <https://reviews.apache.org/r/66610/#comment282992> Reuse extracted method `wrapAuthorize()` that takes a `Callable`. core/src/main/java/org/apache/oozie/servlet/SLAServlet.java Lines 172-179 (patched) <https://reviews.apache.org/r/66610/#comment283025> Extract method `wrapAuthorize()` that takes a `Callable`. core/src/main/java/org/apache/oozie/servlet/V1JobsServlet.java Line 141 (original), 141 (patched) <https://reviews.apache.org/r/66610/#comment282993> Leave old formatting. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 9-11 (original), 9-11 (patched) <https://reviews.apache.org/r/66610/#comment282994> Please leave old license header. Otherwise, a RAT error occurs. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Line 70 (original), 70 (patched) <https://reviews.apache.org/r/66610/#comment282995> Leave old formatting. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Line 96 (original), 96 (patched) <https://reviews.apache.org/r/66610/#comment282996> Leave old formatting. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Line 129 (original), 129 (patched) <https://reviews.apache.org/r/66610/#comment283028> `import static ...AuthorizationService.AuthorizationLevel;` core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Line 140 (original), 140 (patched) <https://reviews.apache.org/r/66610/#comment282997> Leave old formatting. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 183 (patched) <https://reviews.apache.org/r/66610/#comment282998> Leave old formatting. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 194 (patched) <https://reviews.apache.org/r/66610/#comment282999> Leave old formatting. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 207-208 (original), 204-205 (patched) <https://reviews.apache.org/r/66610/#comment283030> Please extract `false` to a well-named parameter. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 217-218 (original), 214-215 (patched) <https://reviews.apache.org/r/66610/#comment283031> Please extract `false` to a well-named parameter. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 218 (patched) <https://reviews.apache.org/r/66610/#comment283046> Should be named like: `testWhenConditionsThenExpected()`. Remove `JobAuhiration` name fragment. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 219 (patched) <https://reviews.apache.org/r/66610/#comment283032> Please extract to well-named parameters. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 222 (patched) <https://reviews.apache.org/r/66610/#comment283047> Should be named like: `testWhenConditionsThenExpected()`. Remove `JobAuhiration` name fragment. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 223 (patched) <https://reviews.apache.org/r/66610/#comment283033> Please extract to well-named parameters. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 226 (patched) <https://reviews.apache.org/r/66610/#comment283048> Should be named like: `testWhenConditionsThenExpected()`. Remove `JobAuhiration` name fragment. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 227 (patched) <https://reviews.apache.org/r/66610/#comment283035> Please extract to well-named parameters. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 230 (patched) <https://reviews.apache.org/r/66610/#comment283049> Should be named like: `testWhenConditionsThenExpected()`. Remove `JobAuhiration` name fragment. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 231 (patched) <https://reviews.apache.org/r/66610/#comment283036> Please extract to well-named parameters. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 235 (patched) <https://reviews.apache.org/r/66610/#comment283050> Should be named like: `testWhenConditionsThenExpected()`. Remove `JobAuhiration` name fragment. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 236 (patched) <https://reviews.apache.org/r/66610/#comment283037> Please extract to well-named parameters. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 239 (patched) <https://reviews.apache.org/r/66610/#comment283051> Should be named like: `testWhenConditionsThenExpected()`. Remove `JobAuhiration` name fragment. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 240 (patched) <https://reviews.apache.org/r/66610/#comment283038> Please extract to well-named parameters. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 243 (patched) <https://reviews.apache.org/r/66610/#comment283052> Should be named like: `testWhenConditionsThenExpected()`. Remove `JobAuhiration` name fragment. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 244 (patched) <https://reviews.apache.org/r/66610/#comment283039> Please extract to well-named parameters. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 247 (patched) <https://reviews.apache.org/r/66610/#comment283053> Should be named like: `testWhenConditionsThenExpected()`. Remove `JobAuhiration` name fragment. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 248 (patched) <https://reviews.apache.org/r/66610/#comment283040> Please extract to well-named parameters.Please extract to well-named parameters. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 251 (patched) <https://reviews.apache.org/r/66610/#comment283054> Should be named like: `testWhenConditionsThenExpected()`. Remove `JobAuhiration` name fragment. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 252 (patched) <https://reviews.apache.org/r/66610/#comment283041> Please extract to well-named parameters. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 255 (patched) <https://reviews.apache.org/r/66610/#comment283055> Should be named like: `testWhenConditionsThenExpected()`. Remove `JobAuhiration` name fragment. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 256 (patched) <https://reviews.apache.org/r/66610/#comment283042> Please extract to well-named parameters. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 259 (patched) <https://reviews.apache.org/r/66610/#comment283056> Should be named like: `testWhenConditionsThenExpected()`. Remove `JobAuhiration` name fragment. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 260 (patched) <https://reviews.apache.org/r/66610/#comment283043> Please extract to well-named parameters. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 263 (patched) <https://reviews.apache.org/r/66610/#comment283057> Should be named like: `testWhenConditionsThenExpected()`. Remove `JobAuhiration` name fragment. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 264 (patched) <https://reviews.apache.org/r/66610/#comment283044> Please extract to well-named parameters. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 268-272 (patched) <https://reviews.apache.org/r/66610/#comment283058> `createWorkflowJobAndAssertAuthorization()` would be a better name. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 274 (patched) <https://reviews.apache.org/r/66610/#comment283059> `assertAuthorization()` would be a better name. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Line 243 (original), 310 (patched) <https://reviews.apache.org/r/66610/#comment283000> Leave old formatting. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Line 250 (original), 316 (patched) <https://reviews.apache.org/r/66610/#comment283001> Leave old formatting. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Line 258 (original), 323 (patched) <https://reviews.apache.org/r/66610/#comment283002> Leave old formatting. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Line 267 (original), 331 (patched) <https://reviews.apache.org/r/66610/#comment283003> Leave old formatting. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Line 277 (original), 340 (patched) <https://reviews.apache.org/r/66610/#comment283004> Leave old formatting. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Line 291 (original), 353 (patched) <https://reviews.apache.org/r/66610/#comment283005> Leave old formatting. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Line 299 (original), 360 (patched) <https://reviews.apache.org/r/66610/#comment283006> Leave old formatting. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Line 308 (original), 368 (patched) <https://reviews.apache.org/r/66610/#comment283007> Leave old formatting. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 382 (patched) <https://reviews.apache.org/r/66610/#comment283008> Leave old formatting. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 326-329 (original), 384-388 (patched) <https://reviews.apache.org/r/66610/#comment283010> Here the meaning has changed. Can you please explain why / create a new test case w/ a well named method name to describe the change? core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 386 (patched) <https://reviews.apache.org/r/66610/#comment283009> Leave old formatting. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 425 (patched) <https://reviews.apache.org/r/66610/#comment283011> Leave old formatting. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 431 (patched) <https://reviews.apache.org/r/66610/#comment283045> Please give a more describing name. core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java Lines 436 (patched) <https://reviews.apache.org/r/66610/#comment283012> Leave old formatting. - András Piros On April 13, 2018, 3:03 p.m., Peter Orova wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66610/ > ----------------------------------------------------------- > > (Updated April 13, 2018, 3:03 p.m.) > > > Review request for oozie, András Piros and Peter Cseh. > > > Repository: oozie-git > > > Description > ------- > > OOZIE-3196 Authorization: restrict world readability by user > > > Diffs > ----- > > core/src/main/java/org/apache/oozie/service/AuthorizationService.java > 33fd9c3c4 > core/src/main/java/org/apache/oozie/servlet/BaseAdminServlet.java 85610eb51 > core/src/main/java/org/apache/oozie/servlet/BaseJobServlet.java e1bd3cf61 > core/src/main/java/org/apache/oozie/servlet/SLAServlet.java 8fad98be9 > core/src/main/java/org/apache/oozie/servlet/V1JobsServlet.java c1ca65fd2 > core/src/main/java/org/apache/oozie/util/db/FailingConnectionWrapper.java > 0e3102530 > core/src/main/java/org/apache/oozie/workflow/lite/LiteWorkflowApp.java > 5e6dc7a78 > core/src/test/java/org/apache/oozie/event/TestEventGeneration.java > 59d04200b > core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java > 8df0904d2 > core/src/test/java/org/apache/oozie/test/XDataTestCase.java a8fe70372 > > > Diff: https://reviews.apache.org/r/66610/diff/1/ > > > Testing > ------- > > -TestAuthorizationService updated with Unit tests > -Manual testing done on kerberized cluster > > > Thanks, > > Peter Orova > >