Paella: Fix error displayed before authenticating user.

[miesgre]

This Fix a bug in the loading process when the player tries lo authenticate the user.
Before redirecting to the login page, an error message is briefly displayed.

To test, try to open a video that does not exists without being authenticated. Ex: https://develop.opencast.org/paella7/ui/watch.html?id=no-valid-video

Your pull request should…

• have a concise title
• close an accompanying issue if one exists
• be against the correct branch
• include migration scripts and documentation, if appropriate
• pass automated tests
• have a clean commit history
• have proper commit messages (title and body) for all commits

[miesgre]

@KatrinIhler This PR should be to r/15.x? Or it is ok in r/14.x?

[KatrinIhler]

@KatrinIhler This PR should be to r/15.x? Or it is ok in r/14.x?

Since this is a small bug fix, 14 is fine imo.

[karendolan]

Hi @miesgre, our site has tried this same approach in the same part of the code. We still see the red error circle flashing prior to redirecting the page. It is as if the browser requires the current running Javascript function to complete prior to addressing the location.href at the point that it's requested. We have also wrapped promises and timeouts around everything to try break the execution chain, but still get the red error flash before the redirect is executed.

Is it possible to throw an exception, like a throw Error, to break the execution chain, but when caught, the red error UI artifact is not shown? Maybe a special keyword in the Error message like "not an error but a redirect request". It's a common approach in Java to throw an Exception to go down a different code path.

Another idea, which is more hacky, is to create a UI artifact that obscures the red error UI artifact, so that when the Error is thrown and breaks the execution chain, allowing the browser to address the location.href request, no red Error UI artifact shows prior to the location.href being executed.

[karendolan]

[EDIT] Our site takes longer to for the location.href to resolve because we do an extra redirect. Opencast adopters that use the regular Opencast login might not have the same lag time response from location.href as we do.

Adding this sleep(100) might be sufficient to prevent the Error artifact from displaying for the default login page redirect. I don't want to imply that it doesn't help at all.

[miesgre]

@karendolan , please test ithis exact code in your installation. I think it should word. The time in the sleep does not matter. The important is to break the execution chain because the location.href is not executed immediately..

The throw after the sleep is never executed!. I put it only in case one day browsers change this behavior.

You can even change the sleep function to a non resolve promise function if you want.

function nonResolvingPromise() {
  return new Promise(() => {});
}

...
await nonResolvingPromise();

[karendolan]

@miesgre What is different about our site is that the location.href does a redirect chain. It ` returns an HTTP 301, then HTTP 302, then HTTP 200 on the redirected page. This is different than the default Opencast login page which returns a HTTP 200 immediately. Our sites issue could be related to the back and forth of the HTTP 301.

I'll test adding a dummy location.href, that immediately returns a HTTP 200 instead of the HTTP 301, to see if I still see the following log after the sleep().

      await sleep(100);
      player.log.debug('After performing the auth redirect and sleep. With persisted logging on in the browser, this log should not show.');
      // This Error should not happen, as the user is redirected to the auth page.
      throw Error('Video not found and user is not authenticated. Try to log in.');
      ```
      

[karendolan]

I'll test adding a dummy location.href, that immediately returns a HTTP 200 instead of the HTTP 301, to see if I still see the following log after the sleep().

In Safari, I verified that it's the HTTP 301 that causes the havoc. The HTTP 200 URL displays the log line before the sleep and not the log line after the sleep. The HTTP 301 URL displays the log line before the sleep, the one after the sleep, the error log, and the flash of red error on the page prior to the redirect.

Test by adding and not adding the redirect param .../watch.html?id=&redirect=301

      if (!me.roles.includes('ROLE_USER')) {
        player.log.info('Video not found and user is not authenticated. Try to log in.');
+            if( utils.getUrlParameter('redirect') === '301' ) {
+               // Redirect to a URL that returns a HTTP 301
+               location.href ='https://www.iana.org/domains/example';
+               player.log.debug('Redirected to HTTP 301 before sleep');
+               await sleep(100);
+               player.log.debug('Redirected to HTTP 301 after sleep');
+            } else {
+               // Redirect to a URL that returns a HTTP 200
+               location.href = 'https://example.com';
+               player.log.debug('Redirected to HTTP 200 before sleep');
+               await sleep(100);
+               player.log.debug('Redirected to HTTP 200 after sleep');
+            }
         // This Error should not happen, as the user is redirected to the auth page.
        throw Error('Video not found and user is not authenticated. Try to log in.');
      }
     

[EDIT] When I throttle the network and disable cache, both messages, the before and after sleep(), show on Chrome, Safari and Firefox. The HTTP 200 is faster than the HTTP 301 when the network is not throttled, so that the second message (and error message) is usually missing on fast loads. This implies that the location.href is asynchronous, but it cannot be awaited. The await nonResolvingPromise(); is working in my local tests.

[miesgre]

Thanks @karendolan for testing it! I updated the PR with a non resolving promise.
Please test it!

[miesgre]

@karendolan I added a spinner while redirecting. This should solve your problem.

[karendolan]

@miesgre this works great for me!

I can approve it, but I can't merge it because there is a failing github actions Playright test that isn't related to the code but to the target test video.

    Error: Timed out 5000ms waiting for expect(locator).toHaveTitle(expected)

    Locator: locator(':root')
    Expected string: "Dual-Stream Demo - No series | Opencast"
    Received string: "Paella Player 7"
    ```

[karendolan]

The pull works, fixes the red error flash prior to auth redirect and the spinner shows when the redirect takes a few moments.

The github action Playright test fail needs to be addressed.

[miesgre]

The pull works, fixes the red error flash prior to auth redirect and the spinner shows when the redirect takes a few moments.

The github action Playright test fail needs to be addressed.

@karendolan Don't worry about paella tests. Fails because in r/14.x tests are done using https://develop.opencast.org/paella7/ui/watch.html?id=ID-dual-stream-demo, but this video is not public now.

In #5684 It is solved, but it is only in develop

[karendolan]

Then all good! Approved.