From 0fbbc25393cb23b2fcabda682bb2400573040f92 Mon Sep 17 00:00:00 2001 From: Daniel Adam Date: Tue, 26 Mar 2024 14:00:21 +0100 Subject: [PATCH] fixup! util: extend oc_endpoint_addresses_t by custom on change callback --- .github/workflows/ctt-test.yml | 2 +- .../workflows/plgd-device-test-with-cfg.yml | 2 +- .github/workflows/plgd-hub-test-with-cfg.yml | 2 +- .github/workflows/sonar-cloud-analysis.yml | 2 +- .github/workflows/unit-test-with-cfg.yml | 2 +- api/cloud/oc_cloud_context.c | 6 +- api/cloud/oc_cloud_context_internal.h | 4 + api/cloud/oc_cloud_manager.c | 48 ++++++------ api/cloud/oc_cloud_manager_internal.h | 14 ++++ api/cloud/oc_cloud_rd.c | 2 +- api/cloud/oc_cloud_resource.c | 2 +- api/cloud/oc_cloud_store.c | 10 +-- api/cloud/rd_client.c | 6 +- api/cloud/unittest/cloud_manager_test.cpp | 73 +++++++++++++++---- 14 files changed, 116 insertions(+), 59 deletions(-) diff --git a/.github/workflows/ctt-test.yml b/.github/workflows/ctt-test.yml index 1b77ed8f8c..ace85547df 100644 --- a/.github/workflows/ctt-test.yml +++ b/.github/workflows/ctt-test.yml @@ -28,7 +28,7 @@ jobs: - name: Archive results if: success() || failure() - uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4 with: name: logs path: results/*.octt diff --git a/.github/workflows/plgd-device-test-with-cfg.yml b/.github/workflows/plgd-device-test-with-cfg.yml index 3549a172d6..550dcd8ad4 100644 --- a/.github/workflows/plgd-device-test-with-cfg.yml +++ b/.github/workflows/plgd-device-test-with-cfg.yml @@ -129,7 +129,7 @@ jobs: - name: Upload coverage data if: ${{ inputs.coverage }} - uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4 with: name: plgd-device-coverage path: data/coverage/${{ steps.coverage.outputs.filename }} diff --git a/.github/workflows/plgd-hub-test-with-cfg.yml b/.github/workflows/plgd-hub-test-with-cfg.yml index 4de85f19a8..638449573f 100644 --- a/.github/workflows/plgd-hub-test-with-cfg.yml +++ b/.github/workflows/plgd-hub-test-with-cfg.yml @@ -122,7 +122,7 @@ jobs: - name: Upload coverage data if: ${{ inputs.coverage }} - uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4 with: name: plgd-hub-coverage path: data/coverage/${{ steps.coverage.outputs.filename }} diff --git a/.github/workflows/sonar-cloud-analysis.yml b/.github/workflows/sonar-cloud-analysis.yml index 91e63c8168..f67448b4bd 100644 --- a/.github/workflows/sonar-cloud-analysis.yml +++ b/.github/workflows/sonar-cloud-analysis.yml @@ -113,7 +113,7 @@ jobs: build-wrapper-linux-x86-64 --out-dir ${{ env.BUILD_WRAPPER_OUT_DIR }} cmake --build build --verbose --target client-server-static --target all - name: Get coverage from all tests job - uses: actions/download-artifact@v3 + uses: actions/download-artifact@v4 with: path: tools/ diff --git a/.github/workflows/unit-test-with-cfg.yml b/.github/workflows/unit-test-with-cfg.yml index 5197f9b770..4cd25ffeb9 100644 --- a/.github/workflows/unit-test-with-cfg.yml +++ b/.github/workflows/unit-test-with-cfg.yml @@ -135,7 +135,7 @@ jobs: - name: Upload coverage data if: ${{ inputs.coverage }} - uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4 with: name: unit-test-coverage path: tools/${{ steps.coverage.outputs.filename }} diff --git a/api/cloud/oc_cloud_context.c b/api/cloud/oc_cloud_context.c index 1bcb3fe4cf..d944c91851 100644 --- a/api/cloud/oc_cloud_context.c +++ b/api/cloud/oc_cloud_context.c @@ -60,8 +60,8 @@ need_to_reinitialize_cloud_storage(const oc_cloud_context_t *ctx) return cloud_is_deregistering(ctx); } -static void -cloud_on_server_change(void *data) +void +cloud_context_on_server_change(void *data) { oc_cloud_context_t *ctx = (oc_cloud_context_t *)data; if (ctx->cloud_manager) { @@ -97,7 +97,7 @@ cloud_context_init(size_t device) ctx->cloud_ep_state = OC_SESSION_DISCONNECTED; ctx->cloud_ep = oc_new_endpoint(); ctx->selected_identity_cred_id = -1; - oc_cloud_store_initialize(&ctx->store, cloud_on_server_change, ctx); + oc_cloud_store_initialize(&ctx->store, cloud_context_on_server_change, ctx); oc_cloud_store_load(&ctx->store); ctx->store.status &= ~(OC_CLOUD_LOGGED_IN | OC_CLOUD_TOKEN_EXPIRY | OC_CLOUD_REFRESHED_TOKEN | diff --git a/api/cloud/oc_cloud_context_internal.h b/api/cloud/oc_cloud_context_internal.h index 66f8e36605..e6d0180394 100644 --- a/api/cloud/oc_cloud_context_internal.h +++ b/api/cloud/oc_cloud_context_internal.h @@ -171,6 +171,10 @@ void cloud_context_clear_access_token(oc_cloud_context_t *ctx) OC_NONNULL(); bool cloud_context_has_refresh_token(const oc_cloud_context_t *ctx) OC_NONNULL(); +/** @brief Callback invoekd by ctx::store::ci_servers when the selected cloud + * server is changed */ +void cloud_context_on_server_change(void *data) OC_NONNULL(); + /** @brief Initialize the registration context */ void oc_cloud_registration_context_init(oc_cloud_registration_context_t *regctx, const oc_endpoint_addresses_t *servers) diff --git a/api/cloud/oc_cloud_manager.c b/api/cloud/oc_cloud_manager.c index 092524ae48..a999e2f363 100644 --- a/api/cloud/oc_cloud_manager.c +++ b/api/cloud/oc_cloud_manager.c @@ -256,28 +256,6 @@ cloud_start_process(oc_cloud_context_t *ctx) _oc_signal_event_loop(); } -static uint64_t -refresh_token_expires_in_ms(int64_t expires_in_ms) -{ - if (expires_in_ms <= 0) { - return 0; - } - - if (expires_in_ms > (int64_t)MILLISECONDS_PER_HOUR) { - // if time is more than 1h then set expires to (expires_in_ms - 10min). - return (uint64_t)(expires_in_ms - (int64_t)(10 * MILLISECONDS_PER_MINUTE)); - } - if (expires_in_ms > (int64_t)(4 * MILLISECONDS_PER_MINUTE)) { - // if time is more than 240sec then set expires to (expires_in_ms - 2min). - return (uint64_t)(expires_in_ms - (int64_t)(2 * MILLISECONDS_PER_MINUTE)); - } - if (expires_in_ms > (int64_t)(20 * MILLISECONDS_PER_SECOND)) { - // if time is more than 20sec then set expires to (expires_in_ms - 10sec). - return (uint64_t)(expires_in_ms - (int64_t)(10 * MILLISECONDS_PER_SECOND)); - } - return (uint64_t)expires_in_ms; -} - static bool cloud_is_connection_error_or_timeout(oc_status_t code) { @@ -388,7 +366,7 @@ cloud_manager_handle_redirect_response(oc_cloud_context_t *ctx, oc_endpoint_addresses_add(&ctx->store.ci_servers, oc_endpoint_address_make_view_with_uuid( oc_string_view2(redirecturi), sid)) == NULL) { - OC_ERR("failed to add server to the list"); + OC_CLOUD_ERR("failed to add server to the list"); return false; } @@ -697,6 +675,26 @@ on_keepalive_response(oc_cloud_context_t *ctx, bool response_received, return true; } +uint64_t +cloud_manager_calculate_refresh_token_expiration(uint64_t expires_in_ms) +{ + assert(expires_in_ms > 0); + + if (expires_in_ms > (int64_t)MILLISECONDS_PER_HOUR) { + // if time is more than 1h then set expires to (expires_in_ms - 10min). + return (uint64_t)(expires_in_ms - (int64_t)(10 * MILLISECONDS_PER_MINUTE)); + } + if (expires_in_ms > (int64_t)(4 * MILLISECONDS_PER_MINUTE)) { + // if time is more than 240sec then set expires to (expires_in_ms - 2min). + return (uint64_t)(expires_in_ms - (int64_t)(2 * MILLISECONDS_PER_MINUTE)); + } + if (expires_in_ms > (int64_t)(20 * MILLISECONDS_PER_SECOND)) { + // if time is more than 20sec then set expires to (expires_in_ms - 10sec). + return (uint64_t)(expires_in_ms - (int64_t)(10 * MILLISECONDS_PER_SECOND)); + } + return (uint64_t)0; +} + static void cloud_manager_login_handler(oc_client_response_t *data) { @@ -720,8 +718,8 @@ cloud_manager_login_handler(oc_client_response_t *data) if (ctx->store.expires_in > 0) { oc_reset_delayed_callback_ms( ctx, cloud_manager_refresh_token_async, - refresh_token_expires_in_ms(ctx->store.expires_in * - MILLISECONDS_PER_SECOND)); + cloud_manager_calculate_refresh_token_expiration( + ctx->store.expires_in * MILLISECONDS_PER_SECOND)); } oc_reset_delayed_callback(ctx, cloud_manager_callback_handler_async, 0); return; diff --git a/api/cloud/oc_cloud_manager_internal.h b/api/cloud/oc_cloud_manager_internal.h index eaed61a571..b97e40ce2a 100644 --- a/api/cloud/oc_cloud_manager_internal.h +++ b/api/cloud/oc_cloud_manager_internal.h @@ -64,6 +64,20 @@ bool cloud_manager_handle_redirect_response(oc_cloud_context_t *ctx, const oc_rep_t *payload) OC_NONNULL(); +/** + * @brief Calculate the expiration time of the refresh token. + * + * The expiration time is used to determine when the refresh token should be + * refreshed. It should be less than the actual expiration time of the token, so + * it can be refreshed before it really expires. + * + * + * @param expires_in_ms expires_in value in milliseconds (must be >0) + * @return uint64_t expiration time in milliseconds + */ +uint64_t cloud_manager_calculate_refresh_token_expiration( + uint64_t expires_in_ms); + /** * @brief Parse refresh token response retrieved from the server and store the * data to cloud context. diff --git a/api/cloud/oc_cloud_rd.c b/api/cloud/oc_cloud_rd.c index 59ec46dc06..c18c73d60b 100644 --- a/api/cloud/oc_cloud_rd.c +++ b/api/cloud/oc_cloud_rd.c @@ -369,7 +369,7 @@ oc_cloud_publish_resources(size_t device) { oc_cloud_context_t *ctx = oc_cloud_get_context(device); if (ctx == NULL) { - OC_ERR("cannot publish resource: invalid device(%zu)", device); + OC_CLOUD_ERR("cannot publish resource: invalid device(%zu)", device); return -1; } publish_published_resources(ctx); diff --git a/api/cloud/oc_cloud_resource.c b/api/cloud/oc_cloud_resource.c index 3b9f0ca84a..4b78aed2f5 100644 --- a/api/cloud/oc_cloud_resource.c +++ b/api/cloud/oc_cloud_resource.c @@ -157,7 +157,7 @@ cloud_update_from_request(oc_cloud_context_t *ctx, const oc_request_t *request) if (sid != NULL) { oc_string_view_t sidv = oc_string_view2(sid); if (oc_str_to_uuid_v1(sidv.data, sidv.length, &data.sid) < 0) { - OC_ERR("failed parsing sid(%s)", sidv.data); + OC_CLOUD_ERR("failed parsing sid(%s)", sidv.data); return false; } } diff --git a/api/cloud/oc_cloud_store.c b/api/cloud/oc_cloud_store.c index db075d9bd6..a0b1954d05 100644 --- a/api/cloud/oc_cloud_store.c +++ b/api/cloud/oc_cloud_store.c @@ -121,7 +121,7 @@ oc_cloud_store_dump(const oc_cloud_store_t *store) long ret = oc_storage_data_save(OC_CLOUD_STORE_NAME, store->device, store_encode_cloud, store); if (ret <= 0) { - OC_ERR("cannot dump cloud to storage: error(%ld)", ret); + OC_CLOUD_ERR("cannot dump cloud to storage: error(%ld)", ret); return false; } return true; @@ -304,7 +304,7 @@ oc_cloud_store_decode(const oc_rep_t *rep, oc_cloud_store_t *store) // copy data to store if ((csd.ci_server != NULL || csd.ci_servers != NULL) && !cloud_store_set_servers(store, csd.ci_server, csd.sid, csd.ci_servers)) { - OC_WRN("failed to set cloud servers from storage"); + OC_CLOUD_WRN("failed to set cloud servers from storage"); } if (csd.auth_provider != NULL) { @@ -332,7 +332,7 @@ store_decode_cloud(const oc_rep_t *rep, size_t device, void *data) (void)device; oc_cloud_store_t *store = (oc_cloud_store_t *)data; if (!oc_cloud_store_decode(rep, store)) { - OC_ERR("cannot load cloud: cannot decode representation"); + OC_CLOUD_ERR("cannot load cloud: cannot decode representation"); return -1; } return 0; @@ -343,11 +343,11 @@ oc_cloud_store_load(oc_cloud_store_t *store) { if (oc_storage_data_load(OC_CLOUD_STORE_NAME, store->device, store_decode_cloud, store) <= 0) { - OC_DBG("failed to load cloud from storage"); + OC_CLOUD_DBG("failed to load cloud from storage"); oc_cloud_store_reinitialize(store); return false; } - OC_DBG("cloud loaded from storage"); + OC_CLOUD_DBG("cloud loaded from storage"); return true; } diff --git a/api/cloud/rd_client.c b/api/cloud/rd_client.c index 2ffc00aa9a..46696b5fe0 100644 --- a/api/cloud/rd_client.c +++ b/api/cloud/rd_client.c @@ -158,7 +158,7 @@ rd_prepare_write_buffer(oc_write_buffer_t *wb, char *buffer, size_t buffer_size, // enough room to write the "di={id}" part if (buffer_size <= /*di=*/3 + id.length) { - OC_ERR("buffer too small"); + OC_CLOUD_ERR("buffer too small"); return false; } wb->buffer = buffer; @@ -212,10 +212,10 @@ rd_delete_send_packet(const oc_endpoint_t *endpoint, oc_string_view_t query, void *data) { rd_delete_packet_t *pkt = (rd_delete_packet_t *)data; - OC_DBG("Unpublishing links (query=%s)", query.data); + OC_CLOUD_DBG("Unpublishing links (query=%s)", query.data); if (!oc_do_delete(OC_RSRVD_RD_URI, endpoint, query.data, pkt->handler, pkt->qos, pkt->user_data)) { - OC_ERR("failed to unpublish links (query=%s)", query.data); + OC_CLOUD_ERR("failed to unpublish links (query=%s)", query.data); return false; } return true; diff --git a/api/cloud/unittest/cloud_manager_test.cpp b/api/cloud/unittest/cloud_manager_test.cpp index dae83ebbc8..bf9ba635d1 100644 --- a/api/cloud/unittest/cloud_manager_test.cpp +++ b/api/cloud/unittest/cloud_manager_test.cpp @@ -45,6 +45,7 @@ using namespace std::chrono_literals; // cannot use lower value than 1s because oc_cloud_schedule_action_t::timeout is // in seconds static constexpr auto kTimeout = 1s; +static constexpr auto kTestServer = OC_STRING_VIEW("coap://224.0.1.187:5683"); class TestCloudManager : public testing::Test { public: @@ -66,15 +67,16 @@ class TestCloudManager : public testing::Test { memset(&m_context, 0, sizeof(m_context)); m_context.cloud_ep = oc_new_endpoint(); memset(m_context.cloud_ep, 0, sizeof(oc_endpoint_t)); - oc_cloud_store_initialize(&m_context.store, nullptr, nullptr); - oc_string_view_t ep = OC_STRING_VIEW("coap://224.0.1.187:5683"); + oc_cloud_store_initialize(&m_context.store, cloud_context_on_server_change, + &m_context); oc_uuid_t sid; oc_gen_uuid(&sid); - ASSERT_NE(nullptr, oc_endpoint_addresses_add( - &m_context.store.ci_servers, - oc_endpoint_address_make_view_with_uuid(ep, sid))); - ASSERT_TRUE( - oc_endpoint_addresses_select_by_uri(&m_context.store.ci_servers, ep)); + ASSERT_NE(nullptr, + oc_endpoint_addresses_add( + &m_context.store.ci_servers, + oc_endpoint_address_make_view_with_uuid(kTestServer, sid))); + ASSERT_TRUE(oc_endpoint_addresses_select_by_uri(&m_context.store.ci_servers, + kTestServer)); std::string uid = "501"; oc_set_string(&m_context.store.uid, uid.c_str(), uid.length()); std::string token = "access_token"; @@ -82,10 +84,15 @@ class TestCloudManager : public testing::Test { std::string rtoken = "refresh_token"; oc_set_string(&m_context.store.refresh_token, rtoken.c_str(), rtoken.length()); + + // TODO: rm + oc_log_set_level(OC_LOG_LEVEL_DEBUG); } void TearDown() override { + oc_log_set_level(OC_LOG_LEVEL_INFO); + oc_cloud_manager_set_retry_timeouts(nullptr, 0); oc_free_endpoint(m_context.cloud_ep); @@ -349,20 +356,24 @@ TEST_F(TestCloudManager, cloud_manager_select_next_server_on_retry) ASSERT_NE(nullptr, ep); ASSERT_FALSE( oc_endpoint_addresses_is_selected(&m_context.store.ci_servers, uri)); + // default cloud server (127.0.0.1), kTestServer and uri + ASSERT_EQ(3, oc_endpoint_addresses_size(&m_context.store.ci_servers)); - // When + ASSERT_TRUE(oc_endpoint_addresses_is_selected(&m_context.store.ci_servers, + kTestServer)); m_context.store.status = OC_CLOUD_INITIALIZED; m_context.store.cps = OC_CPS_READYTOREGISTER; - cloud_manager_start(&m_context); + oc_cloud_manager_start(&m_context, nullptr, nullptr); // by default: first retry should happen timeout + jitter // ([timeout/2..timeout]) (see default_schedule_action) - oc::TestDevice::PoolEventsMsV1( - /*timeout*/ kTimeout + /*max possible jitter*/ kTimeout, true); - cloud_manager_stop(&m_context); - - // Then - EXPECT_TRUE( - oc_endpoint_addresses_is_selected(&m_context.store.ci_servers, uri)); + auto interval = (/* 3 servers to try */ 3) * + (/*timeout*/ kTimeout + /*max possible jitter*/ kTimeout); + oc::TestDevice::PoolEventsMsV1(interval, true); + + // the retries should loop all servers and loop back to the original + EXPECT_TRUE(oc_endpoint_addresses_is_selected(&m_context.store.ci_servers, + kTestServer)); + oc_cloud_manager_stop(&m_context); } #endif /* !OC_SECURITY */ @@ -448,6 +459,36 @@ TestCloudManagerData::GetPayload(std::optional access_token, return rep; } +TEST_F(TestCloudManagerData, cloud_manager_calculate_refresh_token_expiration) +{ + // long internal (>1hour) -> refresh schedule 10mins before expiration + std::chrono::milliseconds expires_in = 2h; + auto expires_in_sec = + cloud_manager_calculate_refresh_token_expiration(expires_in.count()); + EXPECT_LT(0, expires_in_sec); + EXPECT_GT(expires_in.count(), expires_in_sec); + + // middle internal (>4mins) -> refresh schedule 2mins before expiration + expires_in = 5min; + expires_in_sec = + cloud_manager_calculate_refresh_token_expiration(expires_in.count()); + EXPECT_LT(0, expires_in_sec); + EXPECT_GT(expires_in.count(), expires_in_sec); + + // short internal (>20s) -> refresh schedule 10secs before expiration + expires_in = 1min; + expires_in_sec = + cloud_manager_calculate_refresh_token_expiration(expires_in.count()); + EXPECT_LT(0, expires_in_sec); + EXPECT_GT(expires_in.count(), expires_in_sec); + + // immediate expiration (<=20s) -> refresh immediately + expires_in = 10s; + expires_in_sec = + cloud_manager_calculate_refresh_token_expiration(expires_in.count()); + EXPECT_EQ(0, expires_in_sec); +} + TEST_F(TestCloudManagerData, cloud_manager_parse_register_data_invalid) { // {