[18.0][MIG] auth_oauth_autologin: Migration to 18.0

[natuan9]

No description provided.

[sbidoul]

@natuan9 Thanks for this migration. We could probably merge this, but it would be nice if you could try to include #821 here. Since in 18 we have deep links, a pure backend redirect should work, and it is simpler and provide a better user experience.

[natuan9]

Hi @sbidoul , I have refactored the module to use a pure backend redirect
Here is the testing record

v18-autologin.mp4

[sbidoul]

I have some doubts with the redirect implementation, in particular when the user is already logged in .

Could you pick-up the tests from #821 ?

The implementation in #821 is a bit different, as is the original implementation in f25f5d4 which calls super first. I don't know which approach is better in 18.0.

[sbidoul]

Can you also remove the old icon so the bot will generate a new one after merge?

[natuan9]

Hi @sbidoul , I've removed the icon and added the tests
I didn't call super first, unlike the original implementation, because I want to avoid unnecessary handling from the parent method

[sbidoul]

Hello,

The logic looks good, however I find the tests very hard to read.

In the original version they looked like this and at first sight this gives the same test coverage while being more compact and in my view easier to understand:

from odoo.tests import common


class TestAuthMethod(common.HttpCase):
    def _assert_no_autologin(self, query=""):
        r = self.url_open(f"/web/login{query}", allow_redirects=False)
        self.assertNotEqual(r.status_code, 303)
        self.assertTrue(r.ok)

    def test_end_to_end_default_providers(self):
        # by default no provider is configured
        providers = self.env["auth.oauth.provider"].search(
            [("enabled", "=", True), ("autologin", "=", True)]
        )
        self.assertFalse(providers)
        self._assert_no_autologin()

    def test_end_to_end_one_provider(self):
        provider = self.env["auth.oauth.provider"].search(
            [("enabled", "=", True), ("autologin", "=", False)],
            limit=1,
        )
        self.assertTrue(provider)
        provider.autologin = True
        # some query parameters disable autologin
        self._assert_no_autologin(query="?no_autologin=1")
        self._assert_no_autologin(query="?error=...")
        self._assert_no_autologin(query="?oauth_error=...")
        # test autologin redirect
        r = self.url_open("/web/login", allow_redirects=False)
        self.assertEqual(r.status_code, 303)
        self.assertTrue(r.headers["Location"].startswith(provider.auth_endpoint))

[sbidoul]

/ocabot migration auth_oauth_autologin

[kobros-tech]

@sbidoul

let's merge the module?

[sbidoul]

#868 (comment) still needs to be addressed.

[github-actions]

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

[madduck]

No activity doesn't mean this is resolved.