Skip to content

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

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

Open
OpenGreenStreet opened this issue Feb 6, 2024 · 42 comments · Fixed by #5544
Open

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

OpenGreenStreet opened this issue Feb 6, 2024 · 42 comments · Fixed by #5544
Assignees

Comments

@OpenGreenStreet
Copy link

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
Copy link
Contributor

I would like to work on this

@whym
Copy link
Collaborator

whym commented Feb 7, 2024

"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:

  • Always include something like "did you change your password recently?" into the message upon login failure. This could be a noise to a majority of the users, though.
    • A variation: do the above only if the app has been unused for X weeks.
    • Another variation: do the above only if there is non-app contributions in the last Y weeks.

@OpenGreenStreet
Copy link
Author

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
Copy link
Author

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

@OpenGreenStreet
Copy link
Author

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

@ShashwatKedia
Copy link
Contributor

@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
Copy link
Author

OpenGreenStreet commented Feb 10, 2024

@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
Copy link
Contributor

@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
Copy link
Author

@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
Copy link
Contributor

@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
Copy link
Author

OpenGreenStreet commented Feb 10, 2024

@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
Copy link
Contributor

@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
Copy link
Author

@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
Copy link
Collaborator

whym commented Feb 10, 2024

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
Copy link
Author

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

@shashankiitbhu
Copy link
Contributor

@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
Copy link
Contributor

shashankiitbhu commented Feb 10, 2024

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
Copy link
Contributor

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

WhatsApp.Video.2024-02-11.at.12.24.30.AM.mp4

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
Copy link
Collaborator

whym commented Feb 12, 2024

@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:

  • Can the username be pre-filled?
  • Can the message ("Your [L]ogin has expired") be persistent? The user won't expect to be asked to log in again, so I think we want to make sure the message is seen and understood. Maybe a dialog in between the upload activity and login activity, or an embedded message in the login form?

@shashankiitbhu
Copy link
Contributor

@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
Copy link
Collaborator

whym commented Feb 12, 2024

(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
Copy link
Contributor

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

@shashankiitbhu
Copy link
Contributor

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

@shashankiitbhu
Copy link
Contributor

@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
Copy link
Collaborator

whym commented Feb 14, 2024

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
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Author

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

  • The app requires a new password to be entered due to a previous password change (up to this point completely normal).
  • Immediately before the new password was entered in the app, there were several unsuccessful password entries (e.g. due to an "attack" on the account by a third party).
  • When logging in via the web interface, a captcha is also displayed. The app does not recognize this captcha/cannot display it. Entering the correct password also fails in the app.

@OpenGreenStreet
Copy link
Author

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
Copy link
Member

@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.

@nicolas-raoul
Copy link
Member

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

@nicolas-raoul nicolas-raoul reopened this Jul 1, 2024
@OpenGreenStreet
Copy link
Author

@nicolas-raoul

  • No upload possible after a password change

*_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
Copy link
Author

@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
Copy link
Member

sivaraam commented Jul 7, 2024

@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.

@OpenGreenStreet
Copy link
Author

@sivaraam Yes, I also think that the information for users could be better. I just noticed that the upload failed. Then I remembered that I had changed my password. Then I logged out and logged in again. Unfortunately, I couldn't find a "log in again" option. As a result, all my settings were lost.
Funnily enough, my login expired again at the weekend without me changing my password. I had to log out and log in again. The app then crashed. (I sent an error log.) But then everything worked and of course I had to reset all the settings again.

@sivaraam
Copy link
Member

sivaraam commented Jul 8, 2024

Ah. The settings lost on each login is indeed unfortunate. I suppose a setting export/import option would be a good value addition that would help address your use case. Feel free to raise a request for it. We could certainly handle it. 👍🏼

@OpenGreenStreet
Copy link
Author

@sivaraam thank You:

see here: #5766

Is that what you meant?

@sivaraam
Copy link
Member

sivaraam commented Jul 9, 2024

Not really but it is a good starting point. The relogin would help your specific case but a settings export/import option would be more helpful as it would generally give an option to restore the settings, say, when the user switches to a new device.

@OpenGreenStreet
Copy link
Author

@sivaraam I don't really want to log out, I just want to update my password: hence the "relogin" idea

@OpenGreenStreet
Copy link
Author

The problem that there is no indication of an invalid password still exists with version 5.0.2~05ffd123e.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants