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

Persist cookies #184

Merged
merged 2 commits into from
Jan 3, 2024
Merged

Persist cookies #184

merged 2 commits into from
Jan 3, 2024

Conversation

jredrejo
Copy link
Member

@jredrejo jredrejo commented Dec 19, 2023

Ensure cookies are accepted and saved right after the webview finishes loading the page.
Closes: #183

Testing apk: https://drive.google.com/file/d/1gjRuBh6-ISpx2OOAfsPb21eUQLJL77-O/view?usp=drive_link

@jredrejo jredrejo requested review from rtibbles and pcenov December 19, 2023 18:24
@pcenov
Copy link
Member

pcenov commented Dec 21, 2023

Hi @jredrejo since the issue described in #183 was more like a question of what exactly is the expected behavior, and since this information is not provided in this PR I can only describe what I am observing in terms of behavior now:

  1. If there is only 1 registered user when I close the app and relaunch it I am being automatically signed in as the same user - this is OK.
  2. If there are multiple users and I am signed in as one of the users, when I close the app and relaunch it I am being automatically signed in as the same user - this could be a problem because if the admin user was the one who was signed in and the device is then given to a learner then the learner will be signed in as an admin upon launching the app.
  3. If there are multiple users, and I am signed out, when I close the app and relaunch it I always get automatically signed in as the admin user - for me this is a security issue.

So I have to ask the same question as before: Shouldn't this rule for automatically signing in the user be applied only if that is the only user on the device?

@jredrejo
Copy link
Member Author

jredrejo commented Jan 2, 2024

Hi @jredrejo since the issue described in #183 was more like a question of what exactly is the expected behavior, and since this information is not provided in this PR I can only describe what I am observing in terms of behavior now:

  1. If there is only 1 registered user when I close the app and relaunch it I am being automatically signed in as the same user - this is OK.
  2. If there are multiple users and I am signed in as one of the users, when I close the app and relaunch it I am being automatically signed in as the same user - this could be a problem because if the admin user was the one who was signed in and the device is then given to a learner then the learner will be signed in as an admin upon launching the app.
  3. If there are multiple users, and I am signed out, when I close the app and relaunch it I always get automatically signed in as the admin user - for me this is a security issue.

So I have to ask the same question as before: Shouldn't this rule for automatically signing in the user be applied only if that is the only user on the device?

well, we're not in the same situation, because before you were always signed as the admin user. Now you're signed back as the same user you were before.
Honestly, I'm not sure of which option would be better in this case. This feature was added to do login more transparent and easy for users, specially in the case of children. Applying the same security measures we usually apply for online apps used by adults might not be the best idea.
With more or less work, all of these options can be implemented. Behaviour before applying this PR was clearly incorrect. With this change I would like to hear more opinions about the way to proceed @jtamiace or @rtibbles, any thoughts?

@rtibbles rtibbles self-assigned this Jan 3, 2024
@rtibbles
Copy link
Member

rtibbles commented Jan 3, 2024

I think the security concerns are addressed by other means - we added the PIN option in order to prevent this kind of security hole.

@rtibbles
Copy link
Member

rtibbles commented Jan 3, 2024

I think keeping the same user logged in when you close and then reopen the app is far more intuitive - the only slightly problematic thing here is the auto login as the admin after you sign out from any account.

I am going to merge this, as it definitely ameliorates the situation, and we can see if the resigning in is problematic for users in the field.

@rtibbles rtibbles merged commit 8243c72 into develop Jan 3, 2024
4 checks passed
@rtibbles rtibbles deleted the persist_cookies branch January 3, 2024 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Login does not persist across app restarts when get_os_user is used
3 participants