Make 2FA login work with commons #1048

maskaravivek · 2018-01-07T01:15:32Z

Addresses #328 and #352.

misaochan · 2018-01-07T06:49:25Z

Thanks for the PR @maskaravivek ! Is there anyone here with 2FA enabled on their account who can test if this works? I will try and apply for 2FA next week but not sure how long it will take.

misaochan · 2018-01-09T10:40:07Z

Got 2FA enabled for my production account and tested with real device running Android 7.0. This crashes for me on prodDebug build after I enter the 2FA code and tap Submit.

Logs:

01-09 20:40:45.092: E/AndroidRuntime(23466): FATAL EXCEPTION: main
01-09 20:40:45.092: E/AndroidRuntime(23466): Process: fr.free.nrw.commons.debug, PID: 23466
01-09 20:40:45.092: E/AndroidRuntime(23466): io.reactivex.exceptions.OnErrorNotImplementedException: length=1; index=1
01-09 20:40:45.092: E/AndroidRuntime(23466): 	at io.reactivex.internal.functions.Functions$OnErrorMissingConsumer.accept(Functions.java:704)
01-09 20:40:45.092: E/AndroidRuntime(23466): 	at io.reactivex.internal.functions.Functions$OnErrorMissingConsumer.accept(Functions.java:701)
01-09 20:40:45.092: E/AndroidRuntime(23466): 	at io.reactivex.internal.observers.LambdaObserver.onError(LambdaObserver.java:74)
01-09 20:40:45.092: E/AndroidRuntime(23466): 	at io.reactivex.internal.operators.observable.ObservableObserveOn$ObserveOnObserver.checkTerminated(ObservableObserveOn.java:276)
01-09 20:40:45.092: E/AndroidRuntime(23466): 	at io.reactivex.internal.operators.observable.ObservableObserveOn$ObserveOnObserver.drainNormal(ObservableObserveOn.java:172)
01-09 20:40:45.092: E/AndroidRuntime(23466): 	at io.reactivex.internal.operators.observable.ObservableObserveOn$ObserveOnObserver.run(ObservableObserveOn.java:252)
01-09 20:40:45.092: E/AndroidRuntime(23466): 	at io.reactivex.android.schedulers.HandlerScheduler$ScheduledRunnable.run(HandlerScheduler.java:109)
01-09 20:40:45.092: E/AndroidRuntime(23466): 	at android.os.Handler.handleCallback(Handler.java:751)
01-09 20:40:45.092: E/AndroidRuntime(23466): 	at android.os.Handler.dispatchMessage(Handler.java:95)
01-09 20:40:45.092: E/AndroidRuntime(23466): 	at android.os.Looper.loop(Looper.java:154)
01-09 20:40:45.092: E/AndroidRuntime(23466): 	at android.app.ActivityThread.main(ActivityThread.java:6692)
01-09 20:40:45.092: E/AndroidRuntime(23466): 	at java.lang.reflect.Method.invoke(Native Method)
01-09 20:40:45.092: E/AndroidRuntime(23466): 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:1468)
01-09 20:40:45.092: E/AndroidRuntime(23466): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1358)
01-09 20:40:45.092: E/AndroidRuntime(23466): Caused by: java.lang.ArrayIndexOutOfBoundsException: length=1; index=1
01-09 20:40:45.092: E/AndroidRuntime(23466): 	at org.mediawiki.api.MWApi.setAuthCookie(MWApi.java:98)
01-09 20:40:45.092: E/AndroidRuntime(23466): 	at fr.free.nrw.commons.mwapi.ApacheHttpClientMediaWikiApi.setAuthCookie(ApacheHttpClientMediaWikiApi.java:157)
01-09 20:40:45.092: E/AndroidRuntime(23466): 	at fr.free.nrw.commons.auth.SessionManager.lambda$getAndSetAuthCookie$1$SessionManager(SessionManager.java:67)
01-09 20:40:45.092: E/AndroidRuntime(23466): 	at fr.free.nrw.commons.auth.SessionManager$$Lambda$1.call(Unknown Source)
01-09 20:40:45.092: E/AndroidRuntime(23466): 	at io.reactivex.internal.operators.observable.ObservableFromCallable.subscribeActual(ObservableFromCallable.java:42)
01-09 20:40:45.092: E/AndroidRuntime(23466): 	at io.reactivex.Observable.subscribe(Observable.java:10901)
01-09 20:40:45.092: E/AndroidRuntime(23466): 	at io.reactivex.internal.operators.observable.ObservableSubscribeOn$SubscribeTask.run(ObservableSubscribeOn.java:96)
01-09 20:40:45.092: E/AndroidRuntime(23466): 	at io.reactivex.Scheduler$DisposeTask.run(Scheduler.java:452)
01-09 20:40:45.092: E/AndroidRuntime(23466): 	at io.reactivex.internal.schedulers.ScheduledRunnable.run(ScheduledRunnable.java:61)
01-09 20:40:45.092: E/AndroidRuntime(23466): 	at io.reactivex.internal.schedulers.ScheduledRunnable.call(ScheduledRunnable.java:52)
01-09 20:40:45.092: E/AndroidRuntime(23466): 	at java.util.concurrent.FutureTask.run(FutureTask.java:237)
01-09 20:40:45.092: E/AndroidRuntime(23466): 	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:272)
01-09 20:40:45.092: E/AndroidRuntime(23466): 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1133)
01-09 20:40:45.092: E/AndroidRuntime(23466): 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:607)
01-09 20:40:45.092: E/AndroidRuntime(23466): 	at java.lang.Thread.run(Thread.java:762)

misaochan

Crashes - see comment

misaochan · 2018-01-09T10:44:33Z

@maskaravivek If it would help for you to get 2FA enabled on your production account, I think you can ask them at the link I mentioned in the issue and point them to this PR, stating that the beta servers aren't really sufficient for our testing/debugging needs as the results are not the same. They might help. :)

maskaravivek · 2018-01-20T22:00:00Z

@misaochan Updated the PR. 2FA now works across sessions and no longer crashes.

Also theres no need to create a cookie manually. :)

codecov-io · 2018-01-20T23:38:15Z

Codecov Report

Merging #1048 into master will increase coverage by 0.02%.
The diff coverage is 6%.

@@            Coverage Diff            @@
##           master   #1048      +/-   ##
=========================================
+ Coverage    3.96%   3.98%   +0.02%     
=========================================
  Files         123     123              
  Lines        5523    5566      +43     
  Branches      521     534      +13     
=========================================
+ Hits          219     222       +3     
- Misses       5289    5329      +40     
  Partials       15      15

Impacted Files	Coverage Δ
...ain/java/fr/free/nrw/commons/auth/AccountUtil.java	`0% <ø> (ø)`	⬆️
...ree/nrw/commons/auth/WikiAccountAuthenticator.java	`0% <0%> (ø)`	⬆️
...n/java/fr/free/nrw/commons/auth/LoginActivity.java	`0% <0%> (ø)`	⬆️
.../commons/auth/WikiAccountAuthenticatorService.java	`0% <0%> (ø)`	⬆️
...r/free/nrw/commons/auth/AuthenticatedActivity.java	`0% <0%> (ø)`	⬆️
...ommons/modifications/ModificationsSyncAdapter.java	`0% <0%> (ø)`	⬆️
.../free/nrw/commons/di/CommonsApplicationModule.java	`52.63% <100%> (ø)`	⬆️
.../java/fr/free/nrw/commons/auth/SessionManager.java	`17.14% <14.28%> (-1.38%)`	⬇️
...rw/commons/mwapi/ApacheHttpClientMediaWikiApi.java	`5.81% <20.83%> (+0.41%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f99dd34...a17f2c8. Read the comment docs.

misaochan · 2018-01-21T07:25:56Z

@maskaravivek Great job! 👍 I tested this on an API 25 emulator and it works well for me, managed to login fine with my 2FA acc. Tested uploading and logging out as well - will test logging in on a non-2FA account shortly.

There is a minor bug when logging in - the loading dialog persists even when 2fa code is asked for (see screenshot), and I had to use the back button to stop it.

maskaravivek · 2018-01-21T08:14:54Z

Ah! That wasnt happening for me while i was testing. I will check once again.

Thanks for testing it. :)

misaochan · 2018-01-21T08:41:11Z

It would also be really good if we could get a code review for this PR from a security/auth standpoint. @dbrant , do you think you could help us with this? :)

maskaravivek · 2018-01-26T07:52:12Z

Have rebased with master to resolve conflicts.

misaochan · 2018-01-29T09:50:23Z

Happy to merge this once the loading dialog bug has been fixed. :)

maskaravivek · 2018-02-05T20:30:06Z

@misaochan have fixed the loader issue.

dbrant · 2018-02-05T22:57:27Z

👍 Looks perfectly fine from a security/auth standpoint!

misaochan · 2018-02-06T07:52:51Z

Great job @maskaravivek ! I tested logging in and out with both my 2FA and non-2FA account, both work well for me. :) And thanks for the review, @dbrant !

misaochan requested changes Jan 9, 2018

View reviewed changes

maskaravivek force-pushed the 2FA branch from a52872f to 0f9bcfc Compare January 20, 2018 21:57

commons-app deleted a comment Jan 20, 2018

maskaravivek force-pushed the 2FA branch from 116110c to a17f2c8 Compare January 26, 2018 07:51

commons-app deleted a comment Jan 26, 2018

maskara added 4 commits February 6, 2018 01:52

Make 2FA login work with commons

1ea0a1e

Make login work across sessions

9c23a8c

Fix tests

fb09ddc

Fix more tests

238cdef

maskaravivek force-pushed the 2FA branch from a17f2c8 to 2efba06 Compare February 5, 2018 20:29

Fix loader in 2FA screen

2efba06

commons-app deleted a comment Feb 5, 2018

misaochan approved these changes Feb 6, 2018

View reviewed changes

misaochan merged commit 2bbc260 into commons-app:master Feb 6, 2018

maskaravivek mentioned this pull request Feb 6, 2018

Logging in to app with two-factor authentication #328

Closed

maskaravivek mentioned this pull request Feb 6, 2018

App "minimizes" immediately #352

Closed

misaochan mentioned this pull request Feb 21, 2018

Login Crash: Cannot log in anymore "attr value delimiter missing" #1173

Closed

maskaravivek deleted the 2FA branch September 12, 2018 20:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make 2FA login work with commons #1048

Make 2FA login work with commons #1048

Uh oh!

maskaravivek commented Jan 7, 2018

Uh oh!

misaochan commented Jan 7, 2018

Uh oh!

misaochan commented Jan 9, 2018 •

edited

Loading

Uh oh!

misaochan left a comment

Uh oh!

misaochan commented Jan 9, 2018 •

edited

Loading

Uh oh!

maskaravivek commented Jan 20, 2018

Uh oh!

codecov-io commented Jan 20, 2018 •

edited

Loading

Uh oh!

misaochan commented Jan 21, 2018

Uh oh!

maskaravivek commented Jan 21, 2018

Uh oh!

misaochan commented Jan 21, 2018

Uh oh!

maskaravivek commented Jan 26, 2018

Uh oh!

misaochan commented Jan 29, 2018 •

edited

Loading

Uh oh!

maskaravivek commented Feb 5, 2018

Uh oh!

dbrant commented Feb 5, 2018

Uh oh!

misaochan commented Feb 6, 2018 •

edited

Loading

Uh oh!

Uh oh!

Make 2FA login work with commons #1048

Make 2FA login work with commons #1048

Uh oh!

Conversation

maskaravivek commented Jan 7, 2018

Uh oh!

misaochan commented Jan 7, 2018

Uh oh!

misaochan commented Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

misaochan left a comment

Choose a reason for hiding this comment

Uh oh!

misaochan commented Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maskaravivek commented Jan 20, 2018

Uh oh!

codecov-io commented Jan 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

misaochan commented Jan 21, 2018

Uh oh!

maskaravivek commented Jan 21, 2018

Uh oh!

misaochan commented Jan 21, 2018

Uh oh!

maskaravivek commented Jan 26, 2018

Uh oh!

misaochan commented Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maskaravivek commented Feb 5, 2018

Uh oh!

dbrant commented Feb 5, 2018

Uh oh!

misaochan commented Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

misaochan commented Jan 9, 2018 •

edited

Loading

misaochan commented Jan 9, 2018 •

edited

Loading

codecov-io commented Jan 20, 2018 •

edited

Loading

misaochan commented Jan 29, 2018 •

edited

Loading

misaochan commented Feb 6, 2018 •

edited

Loading