search

eduvpn / apple

app for iOS and macOS
Other
61 stars 18 forks source link

check profile stil exists before attempting to connect #488

Closed ghost closed 1 year ago

ghost commented 2 years ago

On server upgrade of nl.eduvpn.org the Profile ID changed from amsterdam to ams. The client should handle this properly, i.e., make sure the profile still exists by calling the /info endpoint. If it doesn't exist and >1 profile is available, allow the user to choose. If there is only one, immediately connect to that one instead.

image

roop commented 2 years ago

@fkooman: If we're in the list-of-servers screen and we click on "Netherlands", I expect the app to always call "/info" and then "/connect". After that, let's say we toggle the VPN switch to off, and then toggle it to on again, at that time, I expect only "/connect" to be called, so if the server profiles changed at that time, this can happen.

  1. When you saw this error, did you initiate the connection from the list screen of the toggle in the connection screen?
  2. Is the above description the correct expected behaviour, or should it change?
ghost commented 2 years ago

@efef

ghost commented 2 years ago

Is the above description the correct expected behaviour, or should it change?

I would assume the /info is always called on connect, not just once, to avoid exactly this scenario... See also https://github.com/eduvpn/documentation/blob/v3/API.md#application-flow

Any reasons why you implemented it like this?

roop commented 2 years ago

@fkooman:

The first three steps of the application flow specified in the API documentation are:

  1. Call /info to retrieve a list of available VPN profiles for the user;
  2. Show the available profiles to the user if there is >1 profile and allow the user to choose. Show "No Profiles Available for your Account" when there are no profiles;
  3. After the user chose (or there was only 1 profile) perform the /connect call as per Connect;

When the user clicks on a server in the list-of-servers view, the app calls /info (step 1), and shows the profiles in a dropdown menu in the connection screen (step 2). The user can choose a profile in the dropdown menu. When the user turns the toggle on, we call /connect (step 3). We don't call /info here even though an indefinite amount of time might have passed waiting for the user to pick a profile (and this is what the flow documentation says as well).

If the user disconnects the VPN, the toggle will be off, and the state of the app is as if we're in step 2, for consistency (because at this point, the app is in the connection screen, showing a list of profiles that the user can choose from). If the user turns on the toggle, it should do the same thing as it did in step 3 above.

I believe this is good behaviour because calling /info in step 3 will make connecting a bit slower. Causing a delay in every connection for avoiding this error is not a good idea, I think. Also, because the server profiles might have changed between step 1 and step 3, the client specially looks for this error and shows an alert that allows the user to refresh profiles and try again, so the user can get back on track with minimal disruption.

ghost commented 2 years ago

I think I agree with you! At least the error message is useful and this shouldn't occur too often in practice.

The only optimization that would make sense (maybe?) is that when/if this (profile gone error) occurs the client automatically calls /info and shows the new list of profiles, or connects to the new one (if there's only one), but I guess for now this behavior is fine...

roop commented 2 years ago

I think I agree with you! At least the error message is useful and this shouldn't occur too often in practice.

👍🏻

The only optimization that would make sense (maybe?) is that when/if this (profile gone error) occurs the client automatically calls /info and shows the new list of profiles

I had said "the client specially looks for this error and shows an alert" -- that's actually not true. If there was any error in the "/connect" call, the client shows this alert (so that the client isn't dependent on the error message text from the server -- that can break too easily).

Therefore, I think it's always better we show the user that there's been an error and let him use the "Refresh Profiles" button.

roop commented 1 year ago

I have verified that when the user clicks on the server in the main screen, we always do an "/info" call and then the "/connect" call.

When the user toggles the switch in the connection screen, we always call "/connect" straightaway, without calling "/info".

So the current behaviour seems to be the sensible thing to do.