[feature] Reusable auth token validation logic

[codesankalp]

Closes #100
Fixes #217
Fixes #222
Fixes #251

[codesankalp]

Working on improving tests.

[nemesifier]

I see progress but many points in my previous review have not been addressed:
#242 (review)

[nemesifier]

Looks on the right track, please see my comments below.

[nemesifier]

During functional testing I found out a few issues.

Captive portal login being called also when page is not reloaded

I was testing that the validate token endpoint was not being called when changing pages, which works, that's great!

However, I noticed something which should not happen: the captive portal login form URL should be called only when the user is either logging in or being recognized and logged in back automatically. We should avoid calling the captive portal login URL should not be called if the user is already authenticated in the app.

If you prefer, we can move this to a dedicated issue and work on it as a follow up to this PR. If you like this approach please copy this text (including the gif and create a new issue).

PS: we'll need a test for this change/fix.

Inactive user glitch

I flagged a user as inactive and the username field is showing an error which shouldn't be there:

Please add an automated test which ensures no errors are shown in the form if the user is inactive.

[nemesifier]

While taking a closer look at the code, I found out the following:

• verifyMobileNumber is still lying around, please do a project wide search and ensure is removed

[codesankalp]

While taking a closer look at the code, I found out the following:

• verifyMobileNumber is still lying around, please do a project wide search and ensure is removed

Ok, will recheck again and remove it.

[nemesifier]

Here's another issue I found out during testing.

Try simulating high network latency by adding these two lines to openwisp-radius:

diff --git a/openwisp_radius/api/views.py b/openwisp_radius/api/views.py
index 0311e0e..5b6a03f 100644
--- a/openwisp_radius/api/views.py
+++ b/openwisp_radius/api/views.py
@@ -313,11 +313,15 @@ class ValidateAuthTokenView(
         """
         Used to check whether the auth token of a user is valid or not.
         """
+        from time import sleep
+        sleep(5)

Then, log in, once you are redirected to the status page, reload.

The expected behavior is that the loading overlay covers the page while the data is loaded.
But this is not happening.

I suggest making sure the logic to do this can be also reused, so we don't need to write it in each component.

[codesankalp]

I have solved all the above-mentioned changes and rebased the branch.

[nemesifier]

There is a conflict after merging #245. Can you please resolve it?

While checking out those conflicts more closely I found a few key lines which are confusing to me and I really recommend clearing out any confusion and simplify the code as much as possible, please see my comments below for more information.

I will also shortly do another round of manual testing.

[nemesifier]

when does this case happen? Can you please add a comment?

[nemesifier]

Result of another round of testing: we're almost there!
I found one key issue which we need to address with utmost care. Please read on.

Captive portal login being called also when page is not reloaded

I was testing that the validate token endpoint was not being called when changing pages, which works, that's great!

However, I noticed something which should not happen: the captive portal login form URL should be called only when the user is either logging in or being recognized and logged in back automatically. We should avoid calling the captive portal login URL should not be called if the user is already authenticated in the app.

If you prefer, we can move this to a dedicated issue and work on it as a follow up to this PR. If you like this approach please copy this text (including the gif and create a new issue).

The problem described above is solved but I notice that in some cases it seems the captive portal login is not called when it should be.

Premise: I just noticed my previous review had typos, sorry for that.
This may have contributed to some ambiguity.
So let me rephrase this correctly:

We should perform captive portal login in these cases:

• the user has performed successful login from login component
• the user is recognized from the cookie and is logged in again automatically
• the user clicks on "log in again" after logging out and clicking on "yes" in the modal which asks "Do you want to automatically log in the next time?"

I am testing whether the captive portal login and logout are called correctly and the result do not seem to be 100% consistent.

Captive portal logout seems pretty much always called, which is good.

The fact is that captive portal login doesn't seem to be called when it would have to be.

Here is how I can replicate it:

• open the login page as unauthenticated
• log in

Expected result: captive portal login URL is called (when configured).
Actual result: captive portal login URL is not called.

If I hit reload in the status page I see the captive portal login being called, which is good.
The entire sequence is shown in the animated GIF below.

However, the fact that the captive portal login URL does not get called is a big deal because it means the app will fail to do what is designed to do and the fact that no test is failing make me terribly worried that we may fail at this inadvertently if we break it again while doing other changes.

This is not your fault, because I know you are not aware of these important details and that's why I am here to guide you and make you aware of it.

For the reasons above, it is imperative to add a failing test for this specific case, we need to protect ourselves from the possibility that this app may fail at its core job.

Please also manually test the other cases described above (where I wrote We should perform captive portal login in these cases:).
I also recommend testing them again after you fix the main issue to see if the fix solves also the other cases, if multiple fixes are required, ensure there's an automated test for each (hopefully this is not needed).

BTW: please take a look at this issue #250, which once solved
will help you and everyone working on this app to take advantage of the captive portal mock views I added recently to OpenWISP RADIUS.

PS: we'll need a test for this change/fix.

Can you point out to me the test you added for this?

Inactive user glitch

I flagged a user as inactive and the username field is showing an error which shouldn't be there:

This is fixed 👍.

Please add an automated test which ensures no errors are shown in the form if the user is inactive.

Can you please point out the automated test to me?

[nemesifier]

There's progress but we're not at "professional efficiency" yet, please see my comments below.

[nemesifier]

Great progress @codesankalp, this is almost ready, please follow the suggestions below and we'll be able to merge this.

• follow the code suggestion below regarding a specific test description before anything else
• squash commits so that we end up with one commit for each related issue
• please ensure each commit follows the commit message guidelines regarding referencing and closing issues:
  https://openwisp.io/docs/developer/contributing.html#commit-message-style-guidelines

[nemesifier]

Ready! 👍