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 https://github.com/os-autoinst/openQA/pull/4151

[okurz]

Fixed a typo and a compile error. Ran t/03-auth.t locally only. Let's await complete CI results.

[codecov[bot]]

Codecov Report

Merging #4156 (4b7e78b) into master (7191508) will increase coverage by 0.04%. The diff coverage is 89.83%.

:exclamation: Current head 4b7e78b differs from pull request most recent head 9ab8eee. Consider uploading reports for the commit 9ab8eee to get more accurate results

@@            Coverage Diff             @@
##           master    #4156      +/-   ##
==========================================
+ Coverage   98.08%   98.12%   +0.04%     
==========================================
  Files         375      375              
  Lines       34808    34678     -130     
==========================================
- Hits        34142    34029     -113     
+ Misses        666      649      -17     

Impacted Files	Coverage Δ
t/03-auth.t	100.00% <ø> (ø)
lib/OpenQA/WebAPI/Auth/OpenID.pm	80.32% <82.35%> (+33.66%)	:arrow_up:
t/03-auth-openid.t	100.00% <100.00%> (ø)
lib/OpenQA/WebAPI/Controller/API/V1/Job.pm	94.95% <0.00%> (-0.73%)	:arrow_down:
lib/OpenQA/WebAPI/Controller/Test.pm	94.57% <0.00%> (-0.06%)	:arrow_down:
t/ui/01-list.t	99.15% <0.00%> (-0.05%)	:arrow_down:
lib/OpenQA/Scheduler/Model/Jobs.pm	97.34% <0.00%> (-0.03%)	:arrow_down:
lib/OpenQA/Worker/WebUIConnection.pm	94.55% <0.00%> (-0.03%)	:arrow_down:
t/lib/OpenQA/SeleniumTest.pm	97.46% <0.00%> (-0.02%)	:arrow_down:
lib/OpenQA/Schema/Result/Jobs.pm	98.87% <0.00%> (-0.01%)	:arrow_down:
... and 20 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[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.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[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.

[mergify[bot]]

This pull request is now in conflicts. Could you fix it? 🙏

[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. https://github.com/openSUSE/cavil/commit/24b08a5e1eeda5be3cc91ea97e974f1d70cd29b0

[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

[mergify[bot]]

This pull request is now in conflicts. Could you fix it? 🙏

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[mergify[bot]]

This pull request is now in conflicts. Could you fix it? 🙏

[mergify[bot]]

This pull request is now in conflicts. Could you fix it? 🙏

[codecov[bot]]

Codecov Report

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

Project coverage is 98.42%. Comparing base (1d22005) to head (78a9e47). Report is 385 commits behind head on master.

Files	Patch %	Lines
lib/OpenQA/WebAPI/Auth/OpenID.pm	84.84%	5 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4156      +/-   ##
==========================================
+ Coverage   98.38%   98.42%   +0.04%     
==========================================
  Files         391      392       +1     
  Lines       37882    37908      +26     
==========================================
+ Hits        37270    37312      +42     
+ Misses        612      596      -16     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.