You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@oozie.apache.org by Peter Orova <pe...@gmail.com> on 2018/04/13 15:01:59 UTC
Review Request 66610: OOZIE-3196 Authorization: restrict world
readability by user
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66610/
-----------------------------------------------------------
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
Re: Review Request 66610: OOZIE-3196 Authorization: restrict world
readability by user
Posted by András Piros via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 66610: OOZIE-3196 Authorization: restrict world
readability by user
Posted by Denes Bodo <bo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66610/#review201175
-----------------------------------------------------------
I made minor comments in the code.
Overall I think the code formatter was a bit too aggressive and I would use cleaner code, like more classes should be introducesd which have "isAllowed(...)" method instead of multiple switch statement.
Otherwise I like it. :)
core/src/main/java/org/apache/oozie/service/AuthorizationService.java
Line 28 (original), 28 (patched)
<https://reviews.apache.org/r/66610/#comment282206>
Please do not merge imports with *
core/src/main/java/org/apache/oozie/service/AuthorizationService.java
Lines 98-101 (patched)
<https://reviews.apache.org/r/66610/#comment282207>
Please cleanup empty lines
core/src/main/java/org/apache/oozie/service/AuthorizationService.java
Line 119 (original), 134 (patched)
<https://reviews.apache.org/r/66610/#comment282208>
Please use spaces instead of tab
core/src/main/java/org/apache/oozie/service/AuthorizationService.java
Line 208 (original), 242 (patched)
<https://reviews.apache.org/r/66610/#comment282211>
Please use spaces instead of tab
core/src/test/java/org/apache/oozie/test/XDataTestCase.java
Line 782 (original), 788 (patched)
<https://reviews.apache.org/r/66610/#comment282209>
Please use spaces instead of tab
core/src/test/java/org/apache/oozie/test/XDataTestCase.java
Line 783 (original), 789 (patched)
<https://reviews.apache.org/r/66610/#comment282210>
Please use spaces instead of tab
- Denes Bodo
On April 13, 2018, 5: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, 5: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
>
>
Re: Review Request 66610: OOZIE-3196 Authorization: restrict world
readability by user
Posted by Peter Orova <pe...@gmail.com>.
-----------------------------------------------------------
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