commons-app / apps-android-commons

The Wikimedia Commons Android app allows users to upload pictures from their Android phone/tablet to Wikimedia Commons
https://commons-app.github.io/
Apache License 2.0
982 stars 1.18k forks source link

precise error message if password has become invalid after password change #5522

Open OpenGreenStreet opened 5 months ago

OpenGreenStreet commented 5 months ago

What is the user problem or growth opportunity you want to see solved?

I would like there to be a pop-up message (or a clear error message: more than "Failed") if the password is no longer valid after a password change.

How do you know that this problem exists today? Why is this important?

Background: changed my password using the web frontend, but didn't think to change it in the Common app as well. It took me quite a while to recognize the connection...

Who will benefit from it?

all user

Anything else you would like to add?

https://commons.wikimedia.org/wiki/Commons:Mobile_app/Feedback#Feedback_from_Molgreen_for_version_4.2.1~14b6c455b_6

shashankiitbhu commented 5 months ago

I would like to work on this

whym commented 5 months ago

"after a password change" might be difficult to detect from the app. I could be wrong but I imagine MediaWiki doesn't let clients (like this app) know how old a user's password is on the server side, from a security standpoint. What I think we could do instead is:

OpenGreenStreet commented 5 months ago

I can understand that. How would it be if, instead of the meaningless error message "Failed" there would be a short keyword list of possible errors and also a link to a long version? (On the other hand, I wonder if the app doesn't receive a message about why the upload failed: for example, a code about a failed authentication).

OpenGreenStreet commented 5 months ago

There are more people who have this problem: https://commons.wikimedia.org/wiki/Commons_talk:Mobile_app#App_broken

OpenGreenStreet commented 5 months ago

Would there perhaps be a way to check whether the user is successfully logged in before each upload?

ShashwatKedia commented 5 months ago

@OpenGreenStreet We do check if the user is logged in before even allowing the user to add media details of the upload. Here's the code :

Screenshot 2024-02-08 at 11 56 43 AM
OpenGreenStreet commented 4 months ago

@ShashwatKedia Yes, thank you. I assume this source code refers to the initial login? I am interested in checking the current validity of the login before each individual upload process. From my point of view, this is the only way to recognize whether the password has been changed in the meantime. (Sorry: I have no idea how something like this could be implemented programmatically).

shashankiitbhu commented 4 months ago

@OpenGreenStreet No, the above is code is a check in Upload Activity (which is started when an Upload Is triggered) so this is checked every time you try to upload something

OpenGreenStreet commented 4 months ago

@shashankiitbhu Okay, but then I don't understand why the uploader only receives the unspecific message "Upload error" at the very end of the upload process?

shashankiitbhu commented 4 months ago

@OpenGreenStreet What the above piece of code does is it checks if the User is Logged in (If the user's current session is valid or not) and if it's not then it takes the user to the login page, but this happens immediately after you trigger an upload (as soon as Upload Activity is visible to user), so If you can go till the end of Upload Process then this logic isn't getting called at all.

OpenGreenStreet commented 4 months ago

@shashankiitbhu The problem is that the user assumes that the app does not work. I (and others: Link: https://commons.wikimedia.org/wiki/Commons_talk:Mobile_app#App_broken) only realized after weeks(!) that the upload error was related to my own (long forgotten) last password change. If you're not persistent, you probably think that the app just doesn't work . . . and you will stop using the app or remove it from your smartphone altogether

Is there nothing that can be read and checked that only logged-in users can do? (For example, I can only see which user is the administrator when logged in).

shashankiitbhu commented 4 months ago

@OpenGreenStreet Yes, If you are asking about what restrictions non-logged-in users have then you can see that by clicking "skip" at the Login Page and a restricted version will where you can not Upload Anything or Review Anything, as you can see below: WhatsApp Image 2024-02-10 at 11 57 46 AM WhatsApp Image 2024-02-10 at 11 57 47 AM

Comparing it to the logged-in Version we can see that there are many things only logged-in users can do.

I think the issue here is that the password is changed from the web frontend when you are already Logged-In on the app and you are then unable to upload anything after that. Is this correct?

OpenGreenStreet commented 4 months ago

@shashankiitbhu Yes, that's exactly the problem:

"I think the issue here is that the password is changed from the web frontend when you are already Logged-In on the app and you are then unable to upload anything after that. Is this correct?"

It won't happen to me again. I just want to prevent others from giving up in frustration and no longer using the Commons app (which I really like!).

(By the way: For me, the Commons app is a key application that significantly lowers the threshold for uploading images to Wikimedia Commons).

whym commented 4 months ago

It looks like login failure upon upload attempt is reported internally, but it gets lost somewhere before reaching the user. I tried this:

  1. Log in from the app
  2. Log in from the web interface
  3. Change password from the web interface
  4. Try to upload a file from the app
  5. Fail

There was no error message that mentions log in failure or wrong password. (EDIT: in the user's eyes, I mean.)

From Logcat: (a long list of messages) ``` 2024-02-10 16:08:23.875 12733-13098 CsrfTokenClient fr.free.nrw.commons W fr.free.nrw.commons.auth.login.LoginFailedException: Incorrect password or confirmation code entered. Please try again. at fr.free.nrw.commons.auth.login.LoginClient.loginBlocking(LoginClient.kt:138) at fr.free.nrw.commons.auth.csrf.CsrfTokenClient.getTokenBlocking(CsrfTokenClient.kt:36) at fr.free.nrw.commons.upload.UploadClient.uploadChunkToStash(UploadClient.java:192) at fr.free.nrw.commons.upload.UploadClient.lambda$uploadFileToStash$2$fr-free-nrw-commons-upload-UploadClient(UploadClient.java:124) at fr.free.nrw.commons.upload.UploadClient$$ExternalSyntheticLambda3.accept(Unknown Source:23) at io.reactivex.internal.observers.LambdaObserver.onNext(LambdaObserver.java:63) at io.reactivex.internal.operators.observable.ObservableFromIterable$FromIterableDisposable.run(ObservableFromIterable.java:98) at io.reactivex.internal.operators.observable.ObservableFromIterable.subscribeActual(ObservableFromIterable.java:58) at io.reactivex.Observable.subscribe(Observable.java:12267) at io.reactivex.Observable.subscribe(Observable.java:12253) at io.reactivex.Observable.subscribe(Observable.java:12155) at io.reactivex.Observable.forEach(Observable.java:9105) at fr.free.nrw.commons.upload.UploadClient.uploadFileToStash(UploadClient.java:99) at fr.free.nrw.commons.upload.worker.UploadWorker.uploadContribution(UploadWorker.kt:339) at fr.free.nrw.commons.upload.worker.UploadWorker.access$uploadContribution(UploadWorker.kt:52) at fr.free.nrw.commons.upload.worker.UploadWorker$doWork$2$invokeSuspend$$inlined$map$1$2.emit(Emitters.kt:242) at kotlinx.coroutines.flow.FlowKt__BuildersKt$asFlow$$inlined$unsafeFlow$3.collect(SafeCollector.common.kt:115) at fr.free.nrw.commons.upload.worker.UploadWorker$doWork$2$invokeSuspend$$inlined$map$1.collect(SafeCollector.common.kt:113) at kotlinx.coroutines.flow.FlowKt__CollectKt.collect(Collect.kt:30) at kotlinx.coroutines.flow.FlowKt.collect(Unknown Source:1) at fr.free.nrw.commons.upload.worker.UploadWorker$doWork$2.invokeSuspend(UploadWorker.kt:242) at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33) at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106) at kotlinx.coroutines.internal.LimitedDispatcher.run(LimitedDispatcher.kt:42) at kotlinx.coroutines.scheduling.TaskImpl.run(Tasks.kt:95) at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:570) at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:750) at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:677) at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:664) 2024-02-10 16:08:23.881 12733-13098 UploadClient fr.free.nrw.commons E Failed to upload chunk to stash java.io.IOException: Invalid token, or login failure. at fr.free.nrw.commons.auth.csrf.CsrfTokenClient.getTokenBlocking(CsrfTokenClient.kt:59) at fr.free.nrw.commons.upload.UploadClient.uploadChunkToStash(UploadClient.java:192) at fr.free.nrw.commons.upload.UploadClient.lambda$uploadFileToStash$2$fr-free-nrw-commons-upload-UploadClient(UploadClient.java:124) at fr.free.nrw.commons.upload.UploadClient$$ExternalSyntheticLambda3.accept(Unknown Source:23) at io.reactivex.internal.observers.LambdaObserver.onNext(LambdaObserver.java:63) at io.reactivex.internal.operators.observable.ObservableFromIterable$FromIterableDisposable.run(ObservableFromIterable.java:98) at io.reactivex.internal.operators.observable.ObservableFromIterable.subscribeActual(ObservableFromIterable.java:58) at io.reactivex.Observable.subscribe(Observable.java:12267) at io.reactivex.Observable.subscribe(Observable.java:12253) at io.reactivex.Observable.subscribe(Observable.java:12155) at io.reactivex.Observable.forEach(Observable.java:9105) at fr.free.nrw.commons.upload.UploadClient.uploadFileToStash(UploadClient.java:99) at fr.free.nrw.commons.upload.worker.UploadWorker.uploadContribution(UploadWorker.kt:339) at fr.free.nrw.commons.upload.worker.UploadWorker.access$uploadContribution(UploadWorker.kt:52) at fr.free.nrw.commons.upload.worker.UploadWorker$doWork$2$invokeSuspend$$inlined$map$1$2.emit(Emitters.kt:242) at kotlinx.coroutines.flow.FlowKt__BuildersKt$asFlow$$inlined$unsafeFlow$3.collect(SafeCollector.common.kt:115) at fr.free.nrw.commons.upload.worker.UploadWorker$doWork$2$invokeSuspend$$inlined$map$1.collect(SafeCollector.common.kt:113) at kotlinx.coroutines.flow.FlowKt__CollectKt.collect(Collect.kt:30) at kotlinx.coroutines.flow.FlowKt.collect(Unknown Source:1) at fr.free.nrw.commons.upload.worker.UploadWorker$doWork$2.invokeSuspend(UploadWorker.kt:242) at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33) at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106) at kotlinx.coroutines.internal.LimitedDispatcher.run(LimitedDispatcher.kt:42) at kotlinx.coroutines.scheduling.TaskImpl.run(Tasks.kt:95) at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:570) at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:750) at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:677) at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:664) 2024-02-10 16:08:23.889 12733-13098 UploadClient fr.free.nrw.commons E Received error in chunk upload java.io.IOException: Invalid token, or login failure. at fr.free.nrw.commons.auth.csrf.CsrfTokenClient.getTokenBlocking(CsrfTokenClient.kt:59) at fr.free.nrw.commons.upload.UploadClient.uploadChunkToStash(UploadClient.java:192) at fr.free.nrw.commons.upload.UploadClient.lambda$uploadFileToStash$2$fr-free-nrw-commons-upload-UploadClient(UploadClient.java:124) at fr.free.nrw.commons.upload.UploadClient$$ExternalSyntheticLambda3.accept(Unknown Source:23) at io.reactivex.internal.observers.LambdaObserver.onNext(LambdaObserver.java:63) at io.reactivex.internal.operators.observable.ObservableFromIterable$FromIterableDisposable.run(ObservableFromIterable.java:98) at io.reactivex.internal.operators.observable.ObservableFromIterable.subscribeActual(ObservableFromIterable.java:58) at io.reactivex.Observable.subscribe(Observable.java:12267) at io.reactivex.Observable.subscribe(Observable.java:12253) at io.reactivex.Observable.subscribe(Observable.java:12155) at io.reactivex.Observable.forEach(Observable.java:9105) at fr.free.nrw.commons.upload.UploadClient.uploadFileToStash(UploadClient.java:99) at fr.free.nrw.commons.upload.worker.UploadWorker.uploadContribution(UploadWorker.kt:339) at fr.free.nrw.commons.upload.worker.UploadWorker.access$uploadContribution(UploadWorker.kt:52) at fr.free.nrw.commons.upload.worker.UploadWorker$doWork$2$invokeSuspend$$inlined$map$1$2.emit(Emitters.kt:242) at kotlinx.coroutines.flow.FlowKt__BuildersKt$asFlow$$inlined$unsafeFlow$3.collect(SafeCollector.common.kt:115) at fr.free.nrw.commons.upload.worker.UploadWorker$doWork$2$invokeSuspend$$inlined$map$1.collect(SafeCollector.common.kt:113) at kotlinx.coroutines.flow.FlowKt__CollectKt.collect(Collect.kt:30) at kotlinx.coroutines.flow.FlowKt.collect(Unknown Source:1) at fr.free.nrw.commons.upload.worker.UploadWorker$doWork$2.invokeSuspend(UploadWorker.kt:242) at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33) at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106) at kotlinx.coroutines.internal.LimitedDispatcher.run(LimitedDispatcher.kt:42) at kotlinx.coroutines.scheduling.TaskImpl.run(Tasks.kt:95) at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:570) at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:750) at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:677) at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:664) 2024-02-10 16:08:32.703 12733-13787 OkHttpConn...nterceptor fr.free.nrw.commons E java.io.IOException: {"errors":[{"code":"login-required","text":"You must be logged in.","module":"query+notifications"}],"docref":"See https://commons.wikimedia.org/w/api.php for API usage. Subscribe to the mediawiki-api-announce mailing list at <https://lists.wikimedia.org/postorius/lists/mediawiki-api-announce.lists.wikimedia.org/> for notice of API deprecations and breaking changes.","servedby":"mw2405"} at fr.free.nrw.commons.OkHttpConnectionFactory$UnsuccessfulResponseInterceptor.intercept(OkHttpConnectionFactory.java:88) at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:109) at okhttp3.logging.HttpLoggingInterceptor.intercept(HttpLoggingInterceptor.kt:221) at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:109) at okhttp3.internal.connection.RealCall.getResponseWithInterceptorChain$okhttp(RealCall.kt:201) at okhttp3.internal.connection.RealCall.execute(RealCall.kt:154) at retrofit2.OkHttpCall.execute(OkHttpCall.java:190) at retrofit2.adapter.rxjava2.CallExecuteObservable.subscribeActual(CallExecuteObservable.java:45) at io.reactivex.Observable.subscribe(Observable.java:12267) at retrofit2.adapter.rxjava2.BodyObservable.subscribeActual(BodyObservable.java:34) at io.reactivex.Observable.subscribe(Observable.java:12267) at io.reactivex.internal.operators.observable.ObservableMap.subscribeActual(ObservableMap.java:32) at io.reactivex.Observable.subscribe(Observable.java:12267) at io.reactivex.internal.operators.observable.ObservableFlatMap.subscribeActual(ObservableFlatMap.java:55) at io.reactivex.Observable.subscribe(Observable.java:12267) at io.reactivex.internal.operators.observable.ObservableMap.subscribeActual(ObservableMap.java:32) at io.reactivex.Observable.subscribe(Observable.java:12267) at io.reactivex.internal.operators.observable.ObservableToListSingle.subscribeActual(ObservableToListSingle.java:58) at io.reactivex.Single.subscribe(Single.java:3603) at io.reactivex.internal.operators.single.SingleSubscribeOn$SubscribeOnObserver.run(SingleSubscribeOn.java:89) at io.reactivex.Scheduler$DisposeTask.run(Scheduler.java:578) at io.reactivex.internal.schedulers.ScheduledRunnable.run(ScheduledRunnable.java:66) at io.reactivex.internal.schedulers.ScheduledRunnable.call(ScheduledRunnable.java:57) at java.util.concurrent.FutureTask.run(FutureTask.java:264) at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:307) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:644) at java.lang.Thread.run(Thread.java:1012) 2024-02-10 16:08:33.141 12733-13097 okhttp.OkHttpClient fr.free.nrw.commons I --> GET https://commons.wikimedia.org/w/api.php?format=json&formatversion=2&errorformat=plaintext&action=query&meta=tokens&type=login 2024-02-10 16:08:33.480 12733-13097 okhttp.OkHttpClient fr.free.nrw.commons I <-- 200 https://commons.wikimedia.org/w/api.php?format=json&formatversion=2&errorformat=plaintext&action=query&meta=tokens&type=login (339ms, 102-byte body) 2024-02-10 16:08:33.484 12733-13097 okhttp.OkHttpClient fr.free.nrw.commons I --> POST https://commons.wikimedia.org/w/api.php?format=json&formatversion=2&errorformat=plaintext&action=clientlogin&rememberMe= (150-byte body) 2024-02-10 16:08:34.339 12733-13097 okhttp.OkHttpClient fr.free.nrw.commons I <-- 200 https://commons.wikimedia.org/w/api.php?format=json&formatversion=2&errorformat=plaintext&action=clientlogin&rememberMe= (854ms, 167-byte body) ```
OpenGreenStreet commented 4 months ago

@whym thank you, that's exactly how it was for me and it took me weeks to recognize the connection.

shashankiitbhu commented 4 months ago

@whym Yes that is exactly what is happening, we are getting an empty token but that message is getting over generalised to just being an error

Screenshot 2024-02-10 at 2 01 50 PM
shashankiitbhu commented 4 months ago

After a thorough examination of the code, I found the following solution that can be Implemented - After the Upload Process is complete we can show a message to the user that their Login Session has become Invalid and we will then send the user to Login Page (which I guess is the correct flow here). Note that this will only take place for the above case (when user's token is Invalid) not in case of any other upload failure.

@whym @OpenGreenStreet @nicolas-raoul If this sounds good then I'll make these changes and add the PR here, thanks :)

shashankiitbhu commented 4 months ago

Here is how the flow will work after you Make an Upload (with your password changed from the web frontend) :

https://github.com/commons-app/apps-android-commons/assets/126143257/d0cf3879-21f2-4cc2-966b-f4d395b8b031

This way has the following advantages:

  1. Users will have a clear idea of what they need to do
  2. It will Prevent users from making further actions that require the user to be signed In
  3. After User Logs In Again (With the New Password) the unfinished upload will start again and no data/work will be lost
whym commented 4 months ago

@shashankiitbhu Thank you for working on this. I generally support your approach - showing the login screen is better than showing a message saying to logout and login. Some minor points:

shashankiitbhu commented 4 months ago

@whym

  1. Yes, the username can be pre-filled (but shouldn't we keep this open for the user? If they want to login from another account? )

  2. It can be persistent, we can show that exact message ("Your Login has expired...") in login form like in below screenshot (below "Login to Commons Beta Account") Screenshot_2024-02-12-12-59-22-547_fr.free.nrw.commons.beta.jpg

whym commented 4 months ago

(but shouldn't we keep this open for the user? If they want to login from another account? )

I meant pre-filled and editable.

In the "wrong password or invalid token" context while trying to upload something, I assume the app already has the username internally and it is valid. But yes, it is possible that the user might have changed the name on the website, so the username might need to be edited. (I don't know if that would cause the same invalid token error, or something else, though.)

shashankiitbhu commented 4 months ago

@whym ok so let's make the username pre-filled, will make the PR soon for this

shashankiitbhu commented 4 months ago

@whym @nicolas-raoul Made the changes in the following PR, please take a look at it

shashankiitbhu commented 4 months ago

@whym @sivaraam @nicolas-raoul Upon More Examination of the app's behavior when the password is changed by the frontend website I found out that apart from this issue there are a few more that arise due to this change, for example - reviewing something (like nominating an Image for deletion by clicking "No") ends with failure and no precise message about that failure. So all these issues are linked to the user session becoming Invalid and the app not handling it well.

These are the logs when I am trying to review a contribution:

java.lang.RuntimeException: App believes we're logged in, but got anonymous token.
                                                                                                        at fr.free.nrw.commons.auth.csrf.CsrfTokenClient.getTokenBlocking(CsrfTokenClient.kt:51)
                                                                                                        at fr.free.nrw.commons.actions.PageEditClient.prependEdit(PageEditClient.kt:60)
                                                                                                        at fr.free.nrw.commons.delete.DeleteHelper.delete(DeleteHelper.java:106)
                                                                                                        at fr.free.nrw.commons.delete.DeleteHelper.makeDeletion(DeleteHelper.java:67)
                                                                                                        at fr.free.nrw.commons.delete.DeleteHelper.lambda$askReasonAndExecute$5$fr-free-nrw-commons-delete-DeleteHelper(DeleteHelper.java:223)
                                                                                                        at fr.free.nrw.commons.delete.DeleteHelper$$ExternalSyntheticLambda6.call(Unknown Source:8)
                                                                                                        at io.reactivex.internal.operators.single.SingleDefer.subscribeActual(SingleDefer.java:36)
                                                                                                        at io.reactivex.Single.subscribe(Single.java:3603)
                                                                                                        at io.reactivex.internal.operators.single.SingleSubscribeOn$SubscribeOnObserver.run(SingleSubscribeOn.java:89)
                                                                                                        at io.reactivex.Scheduler$DisposeTask.run(Scheduler.java:578)
                                                                                                        at io.reactivex.internal.schedulers.ScheduledRunnable.run(ScheduledRunnable.java:66)
                                                                                                        at io.reactivex.internal.schedulers.ScheduledRunnable.call(ScheduledRunnable.java:57)
                                                                                                        at javaa.util.concurrent.FutureTask.run(FutureTask.java:264)
                                                                                                        at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:307)

Overall what we need to fix here is how we handle this case when the user's Current Session Becomes Invalid, I have the following two solutions :

  1. Handle Each of these cases Individually (which are similar up to an extent) and Inform the user accordingly like I have done in the above PR for Upload (Similarly we can handle Review too)

  2. Check every time the user opens the app to see if their session is valid or not ( It's an expensive operation and this situation is not that common when user change their password from frontend, considering that making a network request every time the user opens the app seems Inefficient)

    Please Review both of these options and suggest if there is a better solution than this, Thanks

whym commented 4 months ago

That's a great catch! Approch 1 sounds better. PageEditClient is used in category editing, too. I believe we want to add the same error recovery there. (Or it might be automatically covered, depending on how you implement it.)

shashankiitbhu commented 4 months ago

@whym Great, so going forward with the 1st approach. I think since I am fixing for each case, it'll be better to have separate PRs for each case @nicolas-raoul what's your opinion on this? If having separate PRs works here then can you merge the above PR so that I can add PRs for other cases?

shashankiitbhu commented 4 months ago

@whym Great, so going forward with the 1st approach. I think since I am fixing for each case, it'll be better to have separate PRs for each case @nicolas-raoul what's your opinion on this? If having separate PRs works here then can you merge the above PR so that I can add PRs for other cases?

@nicolas-raoul @whym Just Pinging you to ask if the approach works then can I go forward with the other 2 PRs? handling the other 2 cases?

shashankiitbhu commented 3 months ago

@sivaraam @whym For the related two Issues, Should I create a Single Issue Named - Unexpected Behaviour after the Password Change ?? Or Should I just make the PRs and related them to this Issue?

@whym @sivaraam @nicolas-raoul Upon More Examination of the app's behavior when the password is changed by the frontend website I found out that apart from this issue there are a few more that arise due to this change, for example - reviewing something (like nominating an Image for deletion by clicking "No") ends with failure and no precise message about that failure. So all these issues are linked to the user session becoming Invalid and the app not handling it well.

These are the logs when I am trying to review a contribution:

java.lang.RuntimeException: App believes we're logged in, but got anonymous token.
                                                                                                      at fr.free.nrw.commons.auth.csrf.CsrfTokenClient.getTokenBlocking(CsrfTokenClient.kt:51)
                                                                                                      at fr.free.nrw.commons.actions.PageEditClient.prependEdit(PageEditClient.kt:60)
                                                                                                      at fr.free.nrw.commons.delete.DeleteHelper.delete(DeleteHelper.java:106)
                                                                                                      at fr.free.nrw.commons.delete.DeleteHelper.makeDeletion(DeleteHelper.java:67)
                                                                                                      at fr.free.nrw.commons.delete.DeleteHelper.lambda$askReasonAndExecute$5$fr-free-nrw-commons-delete-DeleteHelper(DeleteHelper.java:223)
                                                                                                      at fr.free.nrw.commons.delete.DeleteHelper$$ExternalSyntheticLambda6.call(Unknown Source:8)
                                                                                                      at io.reactivex.internal.operators.single.SingleDefer.subscribeActual(SingleDefer.java:36)
                                                                                                      at io.reactivex.Single.subscribe(Single.java:3603)
                                                                                                      at io.reactivex.internal.operators.single.SingleSubscribeOn$SubscribeOnObserver.run(SingleSubscribeOn.java:89)
                                                                                                      at io.reactivex.Scheduler$DisposeTask.run(Scheduler.java:578)
                                                                                                      at io.reactivex.internal.schedulers.ScheduledRunnable.run(ScheduledRunnable.java:66)
                                                                                                      at io.reactivex.internal.schedulers.ScheduledRunnable.call(ScheduledRunnable.java:57)
                                                                                                      at javaa.util.concurrent.FutureTask.run(FutureTask.java:264)
                                                                                                      at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:307)

Overall what we need to fix here is how we handle this case when the user's Current Session Becomes Invalid, I have the following two solutions :

  1. Handle Each of these cases Individually (which are similar up to an extent) and Inform the user accordingly like I have done in the above PR for Upload (Similarly we can handle Review too)
  2. Check every time the user opens the app to see if their session is valid or not ( It's an expensive operation and this situation is not that common when user change their password from frontend, considering that making a network request every time the user opens the app seems Inefficient)

Please Review both of these options and suggest if there is a better solution than this, Thanks

This is one of the Issues

OpenGreenStreet commented 3 months ago

@whym, @shashankiitbhu Captcha: I noticed something else: in the following scenario, the app cannot accept a correct password:

OpenGreenStreet commented 3 months ago

Just as an aside: I noticed afterwards that the current version of the app behaves "quite normally" until the category is queried: (I first noticed that something was "weird" because no categories were offered/selectable).

sivaraam commented 3 months ago

@whym, @shashankiitbhu Captcha: I noticed something else: in the following scenario, the app cannot accept a correct password:

Thanks for the report, @OpenGreenStreet. I've opened issue #5679 about this.

OpenGreenStreet commented 6 days ago

The problem persists even with version 5.0.1~da0b2c28e. No meaningful message is displayed after a previous password change via the Wikipedia web interface:

nicolas-raoul commented 6 days ago

Thanks for the feedback! Would you mind explaining what action led to each screenshot's situation?

OpenGreenStreet commented 6 days ago

@nicolas-raoul

*_2: Apparently every upload attempt was recognized by the system as a logon with an incorrect password

*_3: For information only: after successfully logging out and successfully logging in again

*_4: Successful upload with valid password

OpenGreenStreet commented 6 days ago

@nicolas-raoul : One more thing: after logging off and on again, all settings are lost (for example for EXIF). Perhaps there could be a menu item "Relogin".

sivaraam commented 7 hours ago

@OpenGreenStreet I think the invalid login credential is not communicated as clearly as it should with the current set of changes. But you should certainly see a difference in v5.0.1. Specifically, the notification you receive in your notification drawer should have a message mentioning "Your log-in has expired. Please log in again". Do you not see that notification?

Of course, it is not the most obvious way to communicate this. We should've at least had a toast mentioning this. It's sad that tapping on the notification doesn't take you to the login screen either. We could certainly improve this 👍🏼

On a side note, I suppose the flow you see when you try to Thank a user for a good image in the "Review" screen would match closely with what you expect.