Solve review issues and add retries for http call

resources/prosody-plugins/mod_auth_token.lua

@@ -74,27 +74,26 @@ function provider.delete_user(username)
 	return nil;
 end
 
-local function validate_result(session, res, error, reason)
-    if res == false then
-        log("warn",
-            "Error verifying token err:%s, reason:%s", error, reason);
-        session.auth_token = nil;
-        return res, error, reason;
-    end
-end
-
 function provider.get_sasl_handler(session)
 
 	local function get_username_from_token(self, message)
 
         -- retrieve custom public key from server and save it on the session
-        local event_result = prosody.events.fire_event("pre-jitsi-authentication-fetch-key", session);
+        local pre_event_result = prosody.events.fire_event("pre-jitsi-authentication-fetch-key", session);
-        if event_result ~= nil then
+        if pre_event_result ~= nil and pre_event_result.res == false then
-            validate_result(session,event_result.res, event_result.error, event_result.reason)
+            log("warn",
+                "Error verifying token on pre authentication stage:%s, reason:%s", pre_event_result.error, pre_event_result.reason);
+            session.auth_token = nil;
+            return pre_event_result.res, pre_event_result.error, pre_event_result.reason;
         end
 
         local res, error, reason = token_util:process_and_verify_token(session);
-        validate_result(session, res, error, reason);
+        if res == false then
+            log("warn",
+                "Error verifying token err:%s, reason:%s", error, reason);
+            session.auth_token = nil;
+            return res, error, reason;
+        end
 
         local customUsername
             = prosody.events.fire_event("pre-jitsi-authentication", session);
@@ -112,9 +111,12 @@ function provider.get_sasl_handler(session)
             self.username = message;
         end
 
-        local event_result = prosody.events.fire_event("post-jitsi-authentication", session);
+        local post_event_result = prosody.events.fire_event("post-jitsi-authentication", session);
-        if event_result ~= nil then
+        if post_event_result ~= nil and post_event_result.res == false then
-            validate_result(session,event_result.res, event_result.error, event_result.reason)
+            log("warn",
+                "Error verifying token on post authentication stage :%s, reason:%s", post_event_result.error, post_event_result.reason);
+            session.auth_token = nil;
+            return post_event_result.res, post_event_result.error, post_event_result.reason;
         end
 
         return res;

resources/prosody-plugins/token/util.lib.lua

@@ -5,17 +5,13 @@ local basexx = require "basexx";
 local have_async, async = pcall(require, "util.async");
 local hex = require "util.hex";
 local jwt = require "luajwtjitsi";
-local http = require "net.http";
 local jid = require "util.jid";
 local json_safe = require "cjson.safe";
 local path = require "util.paths";
 local sha256 = require "util.hashes".sha256;
-local timer = require "util.timer";
+local http_get_with_retry = module:require "util".http_get_with_retry;
 
-local http_timeout = 30;
+local nr_retries = 3;
-local http_headers = {
-    ["User-Agent"] = "Prosody ("..prosody.version.."; "..prosody.platform..")"
-};
 
 -- TODO: Figure out a less arbitrary default cache size.
 local cacheSize = module:get_option_number("jwt_pubkey_cache_size", 128);
@@ -131,65 +127,18 @@ function Util:get_public_key(keyId)
     if content == nil then
         -- If the key is not found in the cache.
         module:log("debug", "Cache miss for key: "..keyId);
-        local code;
-        local timeout_occurred;
-        local wait, done = async.waiter();
-
         local keyurl = path.join(self.asapKeyServer, hex.to(sha256(keyId))..'.pem');
-
-        local function cb(content_, code_, response_, request_)
-            if timeout_occurred == nil then
-                content, code = content_, code_;
-                if code == 200 or code == 204 then
-                    self.cache:set(keyId, content);
-                else
-                    module:log("warn", "Error on public key request: Code %s, Content %s",
-                    code_, content_);
-                end
-                done();
-            else
-                module:log("warn", "public key reply delivered after timeout from: %s",keyurl);
-            end
-        end
-
-        -- TODO: Is the done() call racey? Can we cancel this if the request
-        --       succeedes?
-        local function cancel()
-            -- TODO: This check is racey. Not likely to be a problem, but we should
-            --       still stick a mutex on content / code at some point.
-            if code == nil then
-                timeout_occurred = true;
-                module:log("warn", "Timeout %s seconds fetching public key from: %s",http_timeout,keyurl);
-                if http.destroy_request ~= nil then
-                    http.destroy_request(request);
-                end
-                done();
-            end
-        end
-
         module:log("debug", "Fetching public key from: "..keyurl);
-
+        content = http_get_with_retry(keyurl, nr_retries);
-        -- We hash the key ID to work around some legacy behavior and make
+        if content ~= nil then
-        -- deployment easier. It also helps prevent directory
+            self.cache:set(keyId, content);
-        -- traversal attacks (although path cleaning could have done this too).
-        local request = http.request(keyurl, {
-            headers = http_headers or {},
-            method = "GET"
-        }, cb);
-
-        timer.add_task(http_timeout, cancel);
-        wait();
-
-        if code == 200 or code == 204 then
-            return content;
         end
+        return content;
     else
         -- If the key is in the cache, use it.
         module:log("debug", "Cache hit for key: "..keyId);
         return content;
     end
-
-    return nil;
 end
 
 --- Verifies issuer part of token

resources/prosody-plugins/util.lib.lua

@@ -1,5 +1,13 @@
 local jid = require "util.jid";
+local timer = require "util.timer";
+local http = require "net.http";
+
+local http_timeout = 30;
 local have_async, async = pcall(require, "util.async");
+local http_headers = {
+    ["User-Agent"] = "Prosody ("..prosody.version.."; "..prosody.platform..")"
+};
+
 local muc_domain_prefix
     = module:get_option_string("muc_mapper_domain_prefix", "conference");
 
@@ -208,6 +216,68 @@ function is_healthcheck_room(room_jid)
     return false;
 end
 
+-- Utility function to make an http get request and
+-- retry @param retry number of times
+-- @param url endpoint to be called
+-- @param retry nr of retries, if retry is
+-- nil there will be no retries
+-- @returns result of the http call or nil if
+-- the external call failed after the last retry
+function http_get_with_retry(url, retry)
+    local content, code;
+    local wait, done = async.waiter();
+    local function cb(content_, code_, response_, request_)
+        code = code_;
+        if code == 200 or code == 204 then
+            module:log("debug", "External call was successful, content %s", content_);
+            content = content_
+        else
+            module:log("warn", "Error on public key request: Code %s, Content %s",
+                code_, content_);
+        end
+        done();
+    end
+
+    local function call_http()
+        return http.request(url, {
+            headers = http_headers or {},
+            method = "GET"
+        }, cb);
+    end
+
+    local request = call_http();
+
+    local function cancel()
+        -- TODO: This check is racey. Not likely to be a problem, but we should
+        --       still stick a mutex on content / code at some point.
+        if code == nil then
+            -- no longer present in prosody 0.11, so check before calling
+            if http.destroy_request ~= nil then
+                http.destroy_request(request);
+            end
+            if retry == nil then
+                module:log("debug", "External call failed and retry policy is not set");
+                done();
+            elseif retry ~= nil and retry < 1 then
+                module:log("debug", "External call failed after retry")
+                done();
+            else
+                module:log("debug", "External call failed, retry nr %s", retry)
+                retry = retry - 1;
+                request = call_http()
+                return http_timeout;
+            end
+        end
+    end
+    timer.add_task(http_timeout, cancel);
+    wait();
+
+    if code == 200 or code == 204 then
+        return content;
+    end
+    return nil;
+end
+
 return {
     is_feature_allowed = is_feature_allowed;
     is_healthcheck_room = is_healthcheck_room;
@@ -217,4 +287,5 @@ return {
     room_jid_split_subdomain = room_jid_split_subdomain;
     internal_room_jid_match_rewrite = internal_room_jid_match_rewrite;
     update_presence_identity = update_presence_identity;
+    http_get_with_retry = http_get_with_retry;
 };