GEANT / CAT

CAT - the Configuration Assistant Tool for Enterprise Wi-Fi networks such as eduroam
Other
93 stars 52 forks source link

Missing Admin API Implementation for STATISTICS-INST #286

Closed IdeasCKCU-dev closed 11 months ago

IdeasCKCU-dev commented 11 months ago

Issue type

Defect/Feature description

Calling the admin API with the "STATISTICS-INST" action returns a not implemented yet error, and code inspection appears to confirm that this functionality is MIA.

How to reproduce issue

IDPID=172
APIKEY=REDACTED
curl -s -X POST --data "{ \"ACTION\": \"STATISTICS-INST\", \"PARAMETERS\": [ { \"NAME\": \"ATTRIB-CAT-INSTID\", \"VALUE\": \"${IDPID}\" } ], \"APIKEY\": \"${APIKEY}\" }" https://cat.eduroam.org/admin/API.php | jq -r '.'

Detail of issue

{
  "result": "ERROR",
  "details": {
    "errorcode": 7,
    "description": "Not implemented yet."
  }
}

I'm guessing something like this needs adding to web/admin/API.php ??

case web\lib\admin\API::ACTION_STATISTICS_INST:
        $retArray = [];
        $idpIdentifier = $adminApi->firstParameterInstance($scrubbedParameters, web\lib\admin\API::AUXATTRIB_CAT_INST_ID);
        if ($idpIdentifier === FALSE) {
            throw new Exception("A required parameter is missing, and this wasn't caught earlier?!");
        } else {
            try {
                $thisIdP = $validator->existingIdP($idpIdentifier, NULL, $fed);
            } catch (Exception $e) {
                $adminApi->returnError(web\lib\admin\API::ERROR_INVALID_PARAMETER, "IdP identifier does not exist!");
                exit(1);
            }
            $retArray[$idpIdentifier] = $thisIdP->getAttributes();
            foreach ($thisIdP->listProfiles() as $oneProfile) {
                $retArray[$idpIdentifier][$oneProfile->identifier] = $oneProfile->getUserDownloadStats();
            }
        }
        $adminApi->returnSuccess($retArray);
        break;
    }
spaetow commented 11 months ago

I second this addition... It will help with statistics-gathering for us as roaming operator. 👍

I think (based on Ed's code fragment), a list of the profiles in the specified organisation (with the download stats of each profile) will probably work for everyone (and that'd actually be perfect for our stats gathering).

restena-sw commented 11 months ago

Thanks for the report, and the excellent code suggestion. I used it almost as-is (the suggestion included the IdP attributes in the response; but this is about stats only. @twoln can you push this change to the test server?

twoln commented 11 months ago

Done on cat-test.eduroam.org

EdKingscote commented 11 months ago

Thanks @twoln.

I've spot checked and the data is aligned with the UI display for a chosen IdP in our world so I believe this is doing the right things based on the available data in CAT today.

The only thing noted is that when the value of ATTRIB-CAT-INSTID is not available within the scope of the federation operator API key the error thrown is required parameter missing - I've not validated how this aligns with other similar API calls, but this is certainly more acceptable than leaking data or failing in a worse way.

restena-sw commented 11 months ago

Sounds good. I guess we can then push this as a hotfix to production CAT, and close the issue afterwards.

twoln commented 11 months ago

Will do today TomaszWiadomość napisana przez Stefan Winter @.***> w dniu 28.11.2023, o godz. 17:24: Sounds good. I guess we can then push this as a hotfix to production CAT, and close the issue afterwards.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

twoln commented 11 months ago

I have added the hot fix to production.