teodesian / TestRail-Perl

Perl interface to TestRail's REST API
5 stars 8 forks source link

Bad Error message when LDAP authentication via TestRail fails #142

Closed troglodyne closed 6 years ago

troglodyne commented 6 years ago

When TestRail fails to bind to the LDAP server, it issues a 401 instead of a 403, which TestRail::API currently allows the catchall of "Bad Arguments" to trigger due to the code not being a 200.

Example: Arguments Bad: {"error":"External auth: Bind: Can't contact LDAP server (failed to retrieve user object)"}

Additionally, it might be better to just die instead of clucking on 403/401 results, as there's no reasonable expectation that subsequent calls will start succeeding when authentication is failing (whatever the reason).

Currently, you instead won't die, but it does break assumptions RE the return type from the call in question (as it will instead be a negative version of the HTTP status code).

teodesian commented 6 years ago

Example Stack trace: ERROR: Arguments Bad: {"error":"External auth: Bind: Can't contact LDAP server (failed to retrieve user object)"} at lib/TestRail/API.pm line 170. TestRail::API::_doRequest(TestRail::API=HASH(0x62787b0), "index.php?/api/v2/get_plans/1\x{26}offset=2500\x{26}limit=250") called at /usr/local/cpanel/3rdparty/perl/526/lib64/perl5/cpanel_lib/TestRail/API.pm line 915 TestRail::API::getPlansPaginated(TestRail::API=HASH(0x62787b0), 1, 250, 2500, undef) called at /usr/local/cpanel/3rdparty/perl/526/lib64/perl5/cpanel_lib/TestRail/API.pm line 888 TestRail::API::getPlans(TestRail::API=HASH(0x62787b0), 1) called at /usr/local/cpanel/3rdparty/perl/526/lib64/perl5/cpanel_lib/TestRail/API.pm line 922 TestRail::API::getPlanByName(TestRail::API=HASH(0x62787b0), 1, "CL 6.5 side-by side SBCL") called at /usr/local/cpanel/t/qa/lib/App/Prove/Cpanel/TestRail.pm line 422

teodesian commented 6 years ago

Unfortunately, this is a 500 ISE. That said, I'm gonna die on 401/403 because of this case.

So, I'm going to add a ~/.testrailrc option to RETRY_ON_ISE (retry up to X times in the event of ISE).

teodesian commented 6 years ago

Actually this should just be folded into a generic RETRY_ON_FAILURE from #141