Simplify OpenQA::WebAPI::Auth::OpenID

[okurz]

• Use signatures in OpenQA::WebAPI::Auth::OpenID
• Simplify OpenQA::WebAPI::Auth::OpenID

[kraih]

I wouldn't put too much effort into this. With OpenID Connect support coming to Mojolicious::Plugin::OAuth2 and legacy OpenID being deprecated, we should be able to get rid of this code at some point in the not so distant future.

[okurz]

I agree. But the above refactoring was really just a 10 minute effort while actively reviewing #4151

[Martchus]

Unfortunately the patch coverage doesn't look great. So at least also test it manually.

[okurz]

I tested locally with

sed -i 's/^# method = .*OpenID/method = OpenID/;s/^#httpsonly = 1/httpsonly = 0/' etc/openqa/openqa.ini
NOT_HEADLESS=1 perl -Ilib -d t/ui/01-list.t

and hit

Can't locate object method "_handle_verified" via package "OpenQA::Shared::Controller::Session" at lib/OpenQA/WebAPI/Auth/OpenID.pm line 122

likely we should try to increase the test coverage with automatic tests.

[okurz]

I extended unit tests also for the method _handle_verified but I am still running into

Can't locate object method "_handle_verified" via package "OpenQA::Shared::Controller::Session" at lib/OpenQA/WebAPI/Auth/OpenID.pm line 122

when running something like NOT_HEADLESS=1 perl -Ilib -d t/ui/01-list.t. Some help would be appreciated.

[Martchus]

The patch coverage check is still complaining but the change looks generally good (code is mainly moved around).

[kraih]

That reminds me, i've replaced OpenID with OpenID Connect in Cavil now. The change can probably be replicated for openQA to simplify OpenQA::WebAPI::Auth::OpenID further. openSUSE/cavil@24b08a5

[okurz]

That reminds me, i've replaced OpenID with OpenID Connect in Cavil now. The change can probably be replicated for openQA to simplify OpenQA::WebAPI::Auth::OpenID further. openSUSE/cavil@24b08a5

That looks nice. Would be cool if you could propose a PR doing that.

[okurz]

The patch coverage check is still complaining but the change looks generally good (code is mainly moved around).

Yes, we should extend test coverage first. If anyone of you fancies doing so be my guest otherwise a candidate for a mob session.

[kraih]

That looks nice. Would be cool if you could propose a PR doing that.

Sure, once it's in the backlog. 😉 https://progress.opensuse.org/issues/89023

[codecov]

Codecov Report

Attention: Patch coverage is 91.52542% with 5 lines in your changes missing coverage. Please review.

Project coverage is 98.49%. Comparing base (2d77acd) to head (f3c5f75).

Files	Patch %	Lines
lib/OpenQA/WebAPI/Auth/OpenID.pm	84.84%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4156      +/-   ##
==========================================
+ Coverage   98.44%   98.49%   +0.04%     
==========================================
  Files         393      394       +1     
  Lines       38631    38657      +26     
==========================================
+ Hits        38032    38074      +42     
+ Misses        599      583      -16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[okurz]

Fixed the problem #4156 (comment) with help from @Martchus . I shouldn't try to call member methods on the object when those are actually called on the controller. Now properly calling those functions as free functions with $c as first argument. Maybe that was all that was necessary

[okurz]

Everything good except patch coverage. As agreed in before merging based on manual test results.