Improve the logic around schedule application in LockEndpoint.

bzbarsky-apple · bzbarsky-apple · commit 3e358092ee4f · 2024-02-05T18:16:15.000-05:00
The old logic had several bugs:

* For kYearDayScheduleUser users, there was no schedule enforcement at all,
  since the outer "if" never tested true.
* For kWeekDayScheduleUser users, there was no schedule enforcement at all,
  since the inner "if" never tested true.
* For kScheduleRestrictedUser users, access was allowed if there was any
  schedule, weekday or yearday, that granted access.  But the intent is that
  access should be disallowed if there are year day schedules and none of them
  grants access, no matter what's going on with weekday.  And vice versa.
diff --git a/examples/lock-app/lock-common/include/LockEndpoint.h b/examples/lock-app/lock-common/include/LockEndpoint.h
@@ -106,8 +106,16 @@ class LockEndpoint
                       OperationSourceEnum opSource = OperationSourceEnum::kUnspecified);
     const char * lockStateToString(DlLockState lockState) const;
 
-    bool weekDayScheduleInAction(uint16_t userIndex) const;
-    bool yearDayScheduleInAction(uint16_t userIndex) const;
+    // Returns true if week day schedules should apply to the user, there are
+    // schedules defined for the user, and access is not currently allowed by
+    // those schedules.  The outparam indicates whether there were in fact any
+    // year day schedules defined for the user.
+    bool weekDayScheduleForbidsAccess(uint16_t userIndex, bool * haveSchedule) const;
+    // Returns true if year day schedules should apply to the user, there are
+    // schedules defined for the user, and access is not currently allowed by
+    // those schedules.  The outparam indicates whether there were in fact any
+    // year day schedules defined for the user.
+    bool yearDayScheduleForbidsAccess(uint16_t userIndex, bool * haveSchedule) const;
 
     static void OnLockActionCompleteCallback(chip::System::Layer *, void * callbackContext);
 
diff --git a/examples/lock-app/lock-common/src/LockEndpoint.cpp b/examples/lock-app/lock-common/src/LockEndpoint.cpp
@@ -488,19 +488,20 @@ bool LockEndpoint::setLockState(const Nullable<chip::FabricIndex> & fabricIdx, c
     auto userIndex = static_cast<uint8_t>(user - mLockUsers.begin());
 
     // Check if schedules affect the user
-    if ((user->userType == UserTypeEnum::kScheduleRestrictedUser || user->userType == UserTypeEnum::kWeekDayScheduleUser) &&
-        !weekDayScheduleInAction(userIndex))
+    bool haveWeekDaySchedules = false;
+    bool haveYearDaySchedules = false;
+    if (weekDayScheduleForbidsAccess(userIndex, &haveWeekDaySchedules) ||
+        yearDayScheduleForbidsAccess(userIndex, &haveYearDaySchedules) ||
+        // Also disallow access for a user that's supposed to have _some_
+        // schedule but doesn't have any
+        (user->userType == UserTypeEnum::kScheduleRestrictedUser && !haveWeekDaySchedules && !haveYearDaySchedules))
     {
-        if ((user->userType == UserTypeEnum::kScheduleRestrictedUser || user->userType == UserTypeEnum::kYearDayScheduleUser) &&
-            !yearDayScheduleInAction(userIndex))
-        {
-            ChipLogDetail(Zcl,
-                          "Lock App: associated user is not allowed to operate the lock due to schedules"
-                          "[endpointId=%d,userIndex=%u]",
-                          mEndpointId, userIndex);
-            err = OperationErrorEnum::kRestricted;
-            return false;
-        }
+        ChipLogDetail(Zcl,
+                      "Lock App: associated user is not allowed to operate the lock due to schedules"
+                      "[endpointId=%d,userIndex=%u]",
+                      mEndpointId, userIndex);
+        err = OperationErrorEnum::kRestricted;
+        return false;
     }
     ChipLogProgress(
         Zcl,
@@ -561,12 +562,23 @@ void LockEndpoint::OnLockActionCompleteCallback(chip::System::Layer *, void * ca
     }
 }
 
-bool LockEndpoint::weekDayScheduleInAction(uint16_t userIndex) const
+bool LockEndpoint::weekDayScheduleForbidsAccess(uint16_t userIndex, bool * haveSchedule) const
 {
+    *haveSchedule = std::any_of(mWeekDaySchedules[userIndex].begin(), mWeekDaySchedules[userIndex].end(),
+                                [](const WeekDaysScheduleInfo & s) { return s.status == DlScheduleStatus::kOccupied; });
+
     const auto & user = mLockUsers[userIndex];
     if (user.userType != UserTypeEnum::kScheduleRestrictedUser && user.userType != UserTypeEnum::kWeekDayScheduleUser)
     {
-        return true;
+        // Weekday schedules don't apply to this user.
+        return false;
+    }
+
+    if (user.userType == UserTypeEnum::kScheduleRestrictedUser && !*haveSchedule)
+    {
+        // It's valid to not have any schedules of a given type; on its own this
+        // does not prevent access.
+        return false;
     }
 
     chip::System::Clock::Milliseconds64 cTMs;
@@ -575,7 +587,7 @@ bool LockEndpoint::weekDayScheduleInAction(uint16_t userIndex) const
     {
         ChipLogError(Zcl, "Lock App: unable to get current time to check user schedules [endpointId=%d,error=%d (%s)]", mEndpointId,
                      chipError.AsInteger(), chipError.AsString());
-        return false;
+        return true;
     }
     time_t unixEpoch = std::chrono::duration_cast<chip::System::Clock::Seconds32>(cTMs).count();
 
@@ -585,8 +597,9 @@ bool LockEndpoint::weekDayScheduleInAction(uint16_t userIndex) const
     auto currentTime =
         calendarTime.tm_hour * chip::kSecondsPerHour + calendarTime.tm_min * chip::kSecondsPerMinute + calendarTime.tm_sec;
 
-    // Second, check the week day schedules.
-    return std::any_of(
+    // Now check whether any schedule allows the current time.  If it does,
+    // access is not forbidden.
+    return !std::any_of(
         mWeekDaySchedules[userIndex].begin(), mWeekDaySchedules[userIndex].end(),
         [currentTime, calendarTime](const WeekDaysScheduleInfo & s) {
             auto startTime = s.schedule.startHour * chip::kSecondsPerHour + s.schedule.startMinute * chip::kSecondsPerMinute;
@@ -596,12 +609,22 @@ bool LockEndpoint::weekDayScheduleInAction(uint16_t userIndex) const
         });
 }
 
-bool LockEndpoint::yearDayScheduleInAction(uint16_t userIndex) const
+bool LockEndpoint::yearDayScheduleForbidsAccess(uint16_t userIndex, bool * haveSchedule) const
 {
+    *haveSchedule = std::any_of(mYearDaySchedules[userIndex].begin(), mYearDaySchedules[userIndex].end(),
+                                [](const YearDayScheduleInfo & sch) { return sch.status == DlScheduleStatus::kOccupied; });
+
     const auto & user = mLockUsers[userIndex];
     if (user.userType != UserTypeEnum::kScheduleRestrictedUser && user.userType != UserTypeEnum::kYearDayScheduleUser)
     {
-        return true;
+        return false;
+    }
+
+    if (user.userType == UserTypeEnum::kScheduleRestrictedUser && !*haveSchedule)
+    {
+        // It's valid to not have any schedules of a given type; on its own this
+        // does not prevent access.
+        return false;
     }
 
     chip::System::Clock::Milliseconds64 cTMs;
@@ -610,7 +633,7 @@ bool LockEndpoint::yearDayScheduleInAction(uint16_t userIndex) const
     {
         ChipLogError(Zcl, "Lock App: unable to get current time to check user schedules [endpointId=%d,error=%d (%s)]", mEndpointId,
                      chipError.AsInteger(), chipError.AsString());
-        return false;
+        return true;
     }
     auto unixEpoch     = std::chrono::duration_cast<chip::System::Clock::Seconds32>(cTMs).count();
     uint32_t chipEpoch = 0;
@@ -623,11 +646,11 @@ bool LockEndpoint::yearDayScheduleInAction(uint16_t userIndex) const
         return false;
     }
 
-    return std::any_of(mYearDaySchedules[userIndex].begin(), mYearDaySchedules[userIndex].end(),
-                       [chipEpoch](const YearDayScheduleInfo & sch) {
-                           return sch.status == DlScheduleStatus::kOccupied && sch.schedule.localStartTime <= chipEpoch &&
-                               chipEpoch <= sch.schedule.localEndTime;
-                       });
+    return !std::any_of(mYearDaySchedules[userIndex].begin(), mYearDaySchedules[userIndex].end(),
+                        [chipEpoch](const YearDayScheduleInfo & sch) {
+                            return sch.status == DlScheduleStatus::kOccupied && sch.schedule.localStartTime <= chipEpoch &&
+                                chipEpoch <= sch.schedule.localEndTime;
+                        });
 }
 
 const char * LockEndpoint::lockStateToString(DlLockState lockState) const
