Re: Review Request 66610: OOZIE-3196 Authorization: restrict world readability by user

2018-04-20 Thread András Piros via Review Board 

---
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)


Please don't change license header - it'll cause RAT errors.



core/src/main/java/org/apache/oozie/service/AuthorizationService.java
Lines 97 (patched)


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)


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)


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)


Please remove dead code.



core/src/main/java/org/apache/oozie/service/AuthorizationService.java
Line 139 (original), 156 (patched)


Leave old formatting.



core/src/main/java/org/apache/oozie/service/AuthorizationService.java
Lines 163 (patched)


Leave old formatting.



core/src/main/java/org/apache/oozie/service/AuthorizationService.java
Line 196 (original), 226 (patched)


Leave old formatting.



core/src/main/java/org/apache/oozie/service/AuthorizationService.java
Line 200 (original), 229 (patched)


Leave old formatting.



core/src/main/java/org/apache/oozie/service/AuthorizationService.java
Lines 234 (patched)


I'd use `INFO` level instead.



core/src/main/java/org/apache/oozie/service/AuthorizationService.java
Line 209 (original), 243 (patched)


Leave old formatting.



core/src/main/java/org/apache/oozie/service/AuthorizationService.java
Line 241 (original), 275 (patched)


Leave old formatting.



core/src/main/java/org/apache/oozie/service/AuthorizationService.java
Line 272 (original), 305 (patched)


Leave old formatting.



core/src/main/java/org/apache/oozie/service/AuthorizationService.java
Line 289 (original), 321 (patched)


Leave old formatting.



core/src/main/java/org/apache/oozie/service/AuthorizationService.java
Lines 366-377 (patched)


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)


Remove empty line.



core/src/main/java/org/apache/oozie/service/AuthorizationService.java
Line 369 (original), 411 (patched)


Leave old formatting.



core/src/main/java/org/apache/oozie/service/AuthorizationService.java
Line 374 (original), 415 (patched)


Leave old formatting.



core/src/main/java/org/apache/oozie/service/AuthorizationService.java
Line 378 (original), 418 (patched)


Leave old formatting.



core/src/main/java/org/apache/oozie/service/AuthorizationService.java
Line 424 (original), 463 (patched)


Leave old formatting.



core/src/main/java/org/apache/oozie/service/AuthorizationService.java
Line 429 (original), 467 (patched)


Leave old formatting.



core/src/main/java/org/apache/oozie/service/AuthorizationService.java
Line 433 (original), 470 (patched)


Leave old formatting.



core/src/main/java/org/apache/oozie/service/AuthorizationService.java

Re: Review Request 66610: OOZIE-3196 Authorization: restrict world readability by user

2018-04-15 Thread Denes Bodo 

---
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)


Please do not merge imports with *



core/src/main/java/org/apache/oozie/service/AuthorizationService.java
Lines 98-101 (patched)


Please cleanup empty lines



core/src/main/java/org/apache/oozie/service/AuthorizationService.java
Line 119 (original), 134 (patched)


Please use spaces instead of tab



core/src/main/java/org/apache/oozie/service/AuthorizationService.java
Line 208 (original), 242 (patched)


Please use spaces instead of tab



core/src/test/java/org/apache/oozie/test/XDataTestCase.java
Line 782 (original), 788 (patched)


Please use spaces instead of tab



core/src/test/java/org/apache/oozie/test/XDataTestCase.java
Line 783 (original), 789 (patched)


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

2018-04-13 Thread Peter Orova 

---
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