----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65893/#review198594 -----------------------------------------------------------
Fix it, then Ship it! agents-common/src/main/java/org/apache/ranger/plugin/contextenricher/RangerTagForEval.java Lines 100 (patched) <https://reviews.apache.org/r/65893/#comment278808> Consider replacing "CollectionUtils.isEmpty(validityPeriodEvaluators)" with "validityPeriodEvaluators == null", to avoid entering this block unnecessarily for scenarios #108/#234 assigns emptyList. agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerValidityScheduleValidator.java Lines 80 (patched) <https://reviews.apache.org/r/65893/#comment278811> Consider replacing "== null" with "StringUtils.isNotEmpty()" agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerValidityScheduleValidator.java Lines 108 (patched) <https://reviews.apache.org/r/65893/#comment278818> normalizedValiditySchedule is assigned a value, even though isValid might be false in #110 below. I notice that 'normalizedValiditySchedule' is referenced in validateTimeRangeSpec() called in #110 below. Perhaps normalizedValiditySchedule should be set to null below, when isValid==false? agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPolicyEngineImpl.java Lines 783 (patched) <https://reviews.apache.org/r/65893/#comment278820> this seems unnecessary here, as above call to tagPolicyRepository.getLikelyMatchPolicyEvaluators() already filters out evaluators (RangerPolicyRepository.java #307). agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPolicyRepository.java Lines 301 (patched) <https://reviews.apache.org/r/65893/#comment278821> "accessTime == null" seems unnecessary, as this is handled within tag.isApplicable(). agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPolicyRepository.java Lines 307 (patched) <https://reviews.apache.org/r/65893/#comment278822> "accessTime == null" seems unnecessary, as this is handled within evaluator.isApplicable(). agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerDefaultPolicyEvaluator.java Lines 626 (patched) <https://reviews.apache.org/r/65893/#comment278823> consider assigning to emptyList() in 'else' case - to be consistant with with other existing create*Evaluators() methods below. agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerValidityScheduleEvaluator.java Lines 50 (patched) <https://reviews.apache.org/r/65893/#comment278833> Consider adding 'final' here. agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerValidityScheduleEvaluator.java Lines 115 (patched) <https://reviews.apache.org/r/65893/#comment278830> Can this offset for timezone be computed in the constuctor? Will eliminate the need to compute this on every call to isApplicable(). agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerValidityScheduleEvaluator.java Lines 130 (patched) <https://reviews.apache.org/r/65893/#comment278832> "ret = false;" seems unnecessary here. agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerValidityScheduleEvaluator.java Lines 142 (patched) <https://reviews.apache.org/r/65893/#comment278831> why logAlways() here? perhaps used in dev testing? agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerValidityScheduleEvaluator.java Lines 206 (patched) <https://reviews.apache.org/r/65893/#comment278834> startTimeInMSs, endTimeInMSs - these parameters may not be necessary, as the caller already filters out accessTime that don't belong in the range. agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerValidityScheduleEvaluator.java Lines 216 (patched) <https://reviews.apache.org/r/65893/#comment278835> Instead of receiving 'accessTime' paramter and instantiating GregorianCalendar in each call to isApplicable(), consider creating the GregorianCalendar instance at the caller so that the same instance can be passed all calls to RangerRecurrenceEvaluator.isApplicable(). agents-common/src/main/java/org/apache/ranger/plugin/resourcematcher/ScheduledTimeRangeMatcher.java Lines 32 (patched) <https://reviews.apache.org/r/65893/#comment278836> "currentTime >= lowerBound && currentTime <= upperBound" seems simpler than the current expression used "currentTime == lowerBound || currentTime == upperBound || (currentTime > lowerBound && currentTime < upperBound);" Any specific reason to handle == lowerBound/upperBound this way? tagsync/src/main/java/org/apache/ranger/tagsync/source/atlas/AtlasNotificationMapper.java Lines 56 (patched) <https://reviews.apache.org/r/65893/#comment278837> this can move out to be a 'static final' - Madhan Neethiraj On March 4, 2018, 4:03 a.m., Abhay Kulkarni wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65893/ > ----------------------------------------------------------- > > (Updated March 4, 2018, 4:03 a.m.) > > > Review request for ranger, Madhan Neethiraj, Ramesh Mani, and Velmurugan > Periasamy. > > > Bugs: RANGER-2000 > https://issues.apache.org/jira/browse/RANGER-2000 > > > Repository: ranger > > > Description > ------- > > Ranger policy engine needs to be able to recognize the start and end times > for policies and enforce them based on period of validity specified by the > user. > Active policies should be checked not only based on the resource, user and > environment context but also whether the policy is effective. > > > Diffs > ----- > > > agents-common/src/main/java/org/apache/ranger/authorization/utils/JsonUtils.java > PRE-CREATION > > agents-common/src/main/java/org/apache/ranger/plugin/contextenricher/RangerTagEnricher.java > 415d4a499 > > agents-common/src/main/java/org/apache/ranger/plugin/contextenricher/RangerTagForEval.java > e31efa3c5 > > agents-common/src/main/java/org/apache/ranger/plugin/errors/ValidationErrorCode.java > a7f7c39ff > > agents-common/src/main/java/org/apache/ranger/plugin/model/RangerPolicy.java > 534fe497a > > agents-common/src/main/java/org/apache/ranger/plugin/model/RangerPolicyResourceSignature.java > 00e60e299 > agents-common/src/main/java/org/apache/ranger/plugin/model/RangerTag.java > 743b02825 > > agents-common/src/main/java/org/apache/ranger/plugin/model/RangerValidityRecurrence.java > PRE-CREATION > > agents-common/src/main/java/org/apache/ranger/plugin/model/RangerValiditySchedule.java > PRE-CREATION > > agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerPolicyValidator.java > 95eeca750 > > agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerValidator.java > 509c1881f > > agents-common/src/main/java/org/apache/ranger/plugin/model/validation/RangerValidityScheduleValidator.java > PRE-CREATION > > agents-common/src/main/java/org/apache/ranger/plugin/policyengine/PolicyEvaluatorForTag.java > 1aad04954 > > agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerAccessResult.java > 1b01ea449 > > agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPolicyEngineImpl.java > f8241c5dd > > agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPolicyRepository.java > 23d1efa15 > > agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerAbstractPolicyEvaluator.java > d218c730b > > agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerDefaultDataMaskPolicyItemEvaluator.java > 349ab360b > > agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerDefaultPolicyEvaluator.java > 2b66c7076 > > agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerDefaultPolicyItemEvaluator.java > 956456551 > > agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerDefaultRowFilterPolicyItemEvaluator.java > cacae5a5b > > agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerPolicyEvaluator.java > 7a890b8b2 > > agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerPolicyItemEvaluator.java > e4864031b > > agents-common/src/main/java/org/apache/ranger/plugin/policyevaluator/RangerValidityScheduleEvaluator.java > PRE-CREATION > > agents-common/src/main/java/org/apache/ranger/plugin/resourcematcher/ScheduledTimeAlwaysMatcher.java > PRE-CREATION > > agents-common/src/main/java/org/apache/ranger/plugin/resourcematcher/ScheduledTimeExactMatcher.java > PRE-CREATION > > agents-common/src/main/java/org/apache/ranger/plugin/resourcematcher/ScheduledTimeMatcher.java > PRE-CREATION > > agents-common/src/main/java/org/apache/ranger/plugin/resourcematcher/ScheduledTimeRangeMatcher.java > PRE-CREATION > > agents-common/src/main/java/org/apache/ranger/plugin/store/AbstractPredicateUtil.java > 1e3f1458a > agents-common/src/main/java/org/apache/ranger/plugin/util/SearchFilter.java > 4a8f1397c > > agents-common/src/test/java/org/apache/ranger/plugin/policyengine/TestPolicyEngine.java > f8c692b9f > agents-common/src/test/resources/log4j.xml 558e27b10 > > agents-common/src/test/resources/policyengine/test_policyengine_temporary.json > PRE-CREATION > > agents-common/src/test/resources/policyengine/validityscheduler/test-validity-schedules-invalid.json > PRE-CREATION > > agents-common/src/test/resources/policyengine/validityscheduler/test-validity-schedules-valid-and-applicable.json > PRE-CREATION > > agents-common/src/test/resources/policyengine/validityscheduler/test-validity-schedules-valid.json > PRE-CREATION > > security-admin/db/mysql/patches/032-add-options-to-policy-and-tag-for-time-based-processing.sql > PRE-CREATION > > security-admin/db/oracle/patches/032-add-options-to-policy-and-tag-for-time-based-processing.sql > PRE-CREATION > > security-admin/db/postgres/patches/032-add-options-to-policy-and-tag-for-time-based-processing.sql > PRE-CREATION > > security-admin/db/sqlanywhere/patches/032-add-options-to-policy-and-tag-for-time-based-processing.sql > PRE-CREATION > > security-admin/db/sqlserver/patches/032-add-options-to-policy-and-tag-for-time-based-processing.sql > PRE-CREATION > > security-admin/src/main/java/org/apache/ranger/biz/RangerPolicyRetriever.java > 2c4241dcd > > security-admin/src/main/java/org/apache/ranger/biz/RangerTagDBRetriever.java > 52c12882c > security-admin/src/main/java/org/apache/ranger/biz/TagDBStore.java > 15830185f > security-admin/src/main/java/org/apache/ranger/common/RangerSearchUtil.java > a2250ec3c > security-admin/src/main/java/org/apache/ranger/entity/XXPolicyBase.java > 69d28bb54 > security-admin/src/main/java/org/apache/ranger/entity/XXTag.java 9155385ec > > security-admin/src/main/java/org/apache/ranger/service/RangerPolicyServiceBase.java > def5033c7 > > security-admin/src/main/java/org/apache/ranger/service/RangerTagServiceBase.java > e68aa9256 > src/main/assembly/tagsync.xml c92939566 > > tagsync/src/main/java/org/apache/ranger/tagsync/source/atlas/AtlasNotificationMapper.java > 916aad3cf > > tagsync/src/main/java/org/apache/ranger/tagsync/source/atlas/AtlasTagSource.java > a13a78901 > > tagsync/src/main/java/org/apache/ranger/tagsync/source/atlas/EntityNotificationWrapper.java > e680b1416 > > tagsync/src/main/java/org/apache/ranger/tagsync/source/atlasrest/AtlasRESTTagSource.java > b7158693c > > tagsync/src/main/java/org/apache/ranger/tagsync/source/atlasrest/RangerAtlasEntityWithTags.java > ecbc50249 > > > Diff: https://reviews.apache.org/r/65893/diff/1/ > > > Testing > ------- > > Tested with local VM > > > Thanks, > > Abhay Kulkarni > >
