Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup old IE/edge properties and implement unsupported browser error page #26358

Merged
merged 3 commits into from Oct 19, 2022

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Mar 29, 2021

The message gets narrowed a bit, we only show recommendations for mobile apps if you're on a mobile and for sektop browsers if you're on a desktop machine.

Desktop Mobile
dev skjnldsv com_unsupported dev skjnldsv com_unsupported (1)

Ref #20855

@skjnldsv skjnldsv self-assigned this Mar 29, 2021
@skjnldsv skjnldsv added 2. developing Work in progress design Design, UI, UX, etc. enhancement labels Mar 29, 2021
@skjnldsv skjnldsv added this to the Nextcloud 22 milestone Mar 29, 2021
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! :)

Do you think we should offer some additional info like which browser wpuld work? But then we also need to specify version numbers and it gets nerdy fast.

So I think it's fine like you did it! :)

@skjnldsv
Copy link
Member Author

Do you think we should offer some additional info like which browser wpuld work? But then we also need to specify version numbers and it gets nerdy fast.

Yes, that was my question and why I asked for your advice :)
Maybe I can automatically build a list of supported browsers based on our configs 🤔

@skjnldsv

This comment has been minimized.

@skjnldsv
Copy link
Member Author

skjnldsv commented May 29, 2021

So, I can build a check based on our browserlist, but it's not feature-aware. It's version-aware.

So some old FF versions might still be comptible with what we use, but using this method will redirect everyone that is not STRICTLY using one of the browser that is within our browserlist config into this "unsupported" page.

Anyone have a better idea? Maybe just show a warning and only redirect IE?
Or all dead browsers?

cc @juliushaertl @PVince81 @ChristophWurst @eneiluj @danxuliu

@blizzz blizzz mentioned this pull request Jun 2, 2021
57 tasks
@skjnldsv skjnldsv modified the milestones: Nextcloud 22, Nextcloud 23 Jun 2, 2021
@skjnldsv skjnldsv force-pushed the fix/ie-cleanup branch 2 times, most recently from 5336565 to 11db327 Compare June 15, 2021 13:32
.eslintrc.js Outdated Show resolved Hide resolved
@skjnldsv skjnldsv force-pushed the fix/ie-cleanup branch 3 times, most recently from 38d6f9f to c599fe3 Compare October 19, 2022 07:39
config/config.sample.php Outdated Show resolved Hide resolved
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
@skjnldsv skjnldsv merged commit afc4b86 into master Oct 19, 2022
@skjnldsv skjnldsv deleted the fix/ie-cleanup branch October 19, 2022 09:12
@AlvaroBrey
Copy link
Member

AlvaroBrey commented Oct 21, 2022

I believe this change may have broken the login flow in Android (maybe iOS too). See the following video:

simplescreenrecorder-2022-10-21_15.59.17.mp4

As seen in the video, the webview just goes to the Dashboard instead of completing the login flow.

The Android app uses the Android system webview, as a reference.

@AlvaroBrey
Copy link
Member

AlvaroBrey commented Oct 21, 2022

I believe this change may have broken the login flow in Android (maybe iOS too). See the following video:

As seen in the video, the webview just goes to the Dashboard instead of completing the login flow.

The Android app uses the Android system webview, as a reference.

Confirming, setting no_unsupporting_browser_warning to true lets the app login again. Will investigate if we can work around it in the app somehow.

@szaimen
Copy link
Contributor

szaimen commented Oct 21, 2022

Confirming, setting no_unsupporting_browser_warning to true lets the app login again. Will investigate if we can work around it in the app somehow.

Another option wouold be adding the webflow user agent to the supported browsers... what is the user agent of such?

@AlvaroBrey
Copy link
Member

AlvaroBrey commented Oct 21, 2022

After some testing, it looks like the unsupported browser page completely discards the initially loaded URL (in this case /login/flow/...) and redirects to /index.php. This will likely also break login for desktop if the default browser is outdated. Not sure about iOS, cc @marinofaggiana

Another option wouold be adding the webflow user agent to the supported browsers... what is the user agent of such?

This seems difficult. We have two different ways of creating user agents:

  • For most cases we use a combination of properties from the Android device. This would look like this for my emulator, for example: Google sdk_gphone64_x86_64 (Android)
  • For one specific case I don't quite understand, we use a pre-configured string, which is the same we use for API requests. This would look like: Mozilla/5.0 (Android) Nextcloud-android/3.23.0 Alpha1, BUT this is changed for branded builds IIRC. @tobiasKaminsky can hopefully shed some light here

@szaimen
Copy link
Contributor

szaimen commented Oct 21, 2022

So what can we do here then? Improve the browser warning to redirect correctly?

@AlvaroBrey
Copy link
Member

AlvaroBrey commented Oct 21, 2022

So what can we do here then? Improve the browser warning to redirect correctly?

That is needed, yes. Because even if we change the Android app to use the default browser instead of a Webview (and thus login flow v2), if the default browser is outdated the same problem will occur and users will not be able to login.

If the browser warning redirects to the original URL, the login should theoretically be possible afterwards. This is still a UX problem, as every Android user will likely see the warning every time they add an account, but at least they will be able to log in.

Another option is to disable the outdated warning for the /login/flow path and subpaths. Not sure if this is sensible or even possible.

@szaimen
Copy link
Contributor

szaimen commented Oct 21, 2022

cc @skjnldsv on this then :)

@skjnldsv
Copy link
Member Author

Confirming, setting no_unsupporting_browser_warning to true lets the app login again. Will investigate if we can work around it in the app somehow.

Another option wouold be adding the webflow user agent to the supported browsers... what is the user agent of such?

Are you setting something specific to the headers that we could use maybe?

the same problem will occur and users will not be able to login

Well, it's a warning, they can bypass it.
But it's defeating the purpose since we're loging in for the auth flow only.
I guess we could check if this is a login for app authentication or just a standard login which the user aims to use the web ui :)

In any case, this si why we merge such things early, to catch them! Nice found @AlvaroBrey 🎉

@skjnldsv
Copy link
Member Author

We could add a bypass if the server as debug mode enabled too

  • For one specific case I don't quite understand, we use a pre-configured string, which is the same we use for API requests. This would look like: Mozilla/5.0 (Android) Nextcloud-android/3.23.0 Alpha1, BUT this is changed for branded builds IIRC. @tobiasKaminsky can hopefully shed some light here

Yep, will need to know how the user agents behave for branded or not.
Otherwise I can just check for the Nextcloud word in the agent :)

@AlvaroBrey
Copy link
Member

Well, it's a warning, they can bypass it.

Right now the user cannot bypass it, as the login flow URL (with its included tokens) is lost on the redirection.

I guess we could check if this is a login for app authentication or just a standard login which the user aims to use the web ui :)

This is why I suggested to bypass the check on /login/flow (and probably /login/v2/flow too, though the Android app does not use that one), since those are only meant for client authentication, not for web use AFAIK.

Are you setting something specific to the headers that we could use maybe?

Right now we're adding OCS-APIREQUEST=true to the first request only (/index.php/login/flow) but not to subsequent requests. If needed we can probably add more headers/try to add them on every request through the login process.

@skjnldsv
Copy link
Member Author

Right now the user cannot bypass it, as the login flow URL (with its included tokens) is lost on the redirection.

Good call, then we should keep the redirect too :)

This is why I suggested to bypass the check on /login/flow (and probably /login/v2/flow too, though the Android app does not use that one), since those are only meant for client authentication, not for web use AFAIK.

I would like something a bit more standard than an url.
What if we have more URLs to cover ?

Right now we're adding OCS-APIREQUEST=true to the first request only (/index.php/login/flow) but not to subsequent requests. If needed we can probably add more headers/try to add them on every request through the login process.

Let's see what I can do :)

@skjnldsv
Copy link
Member Author

This is why I suggested to bypass the check on /login/flow (and probably /login/v2/flow too, though the Android app does not use that one), since those are only meant for client authentication, not for web use AFAIK.

Maybe we could just load this on default template, and not login/maintenance/error/twofactor authentifications pages. Let's see what I can do here :)

@skjnldsv
Copy link
Member Author

Fix in #34741

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish design Design, UI, UX, etc. enhancement technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Browser update notification for outdated browsers
10 participants