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.

Sign up for GitHub

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

Jump to bottom

Fix for failed logins / app crashes #638

Merged
merged 3 commits into from
Nov 9, 2021
Merged

Fix for failed logins / app crashes #638

merged 3 commits into from
Nov 9, 2021

Conversation

mightymop
Copy link
Contributor

@mightymop mightymop commented Jun 4, 2021 •
edited
Loading

@codecov
Copy link

codecov bot commented Jun 4, 2021 •
edited
Loading

Codecov Report

Merging #638 (803d026) into master (5106daf) will decrease coverage by 5.41%.
The diff coverage is 0.00%.

❗ Current head 803d026 differs from pull request most recent head 1d20fbd. Consider uploading reports for the commit 1d20fbd to get more accurate results

@@            Coverage Diff             @@
##           master     #638      +/-   ##
==========================================
- Coverage   46.13%   40.71%   -5.42%     
==========================================
  Files         164      151      -13     
  Lines        6347     6234     -113     
  Branches      833      807      -26     
==========================================
- Hits         2928     2538     -390     
- Misses       2981     3317     +336     
+ Partials      438      379      -59
Impacted Files Coverage Δ
...loud/android/lib/common/OwnCloudClientFactory.java 11.47% <0.00%> (-1.26%) ⬇️
.../main/java/com/nextcloud/common/ResponseOrError.kt 0.00% <0.00%> (-100.00%) ⬇️
...om/owncloud/android/lib/resources/files/Chunk.java 0.00% <0.00%> (-85.72%) ⬇️
...resources/comments/CommentFileRemoteOperation.java 0.00% <0.00%> (-80.77%) ⬇️
.../lib/resources/files/CheckEtagRemoteOperation.java 0.00% <0.00%> (-75.00%) ⬇️
...lib/resources/files/RenameFileRemoteOperation.java 0.00% <0.00%> (-70.59%) ⬇️
...es/comments/MarkCommentsAsReadRemoteOperation.java 0.00% <0.00%> (-68.19%) ⬇️
...sources/files/ReadFileVersionsRemoteOperation.java 0.00% <0.00%> (-58.98%) ⬇️
...b/resources/files/DownloadFileRemoteOperation.java 0.00% <0.00%> (-44.95%) ⬇️
...d/lib/resources/files/MoveFileRemoteOperation.java 0.00% <0.00%> (-44.19%) ⬇️
... and 107 more

This was referenced Jun 5, 2021
Closed
Closed
Closed
@AndyScherzinger AndyScherzinger mentioned this pull request Jun 11, 2021
Closed
@tobiasKaminsky
Copy link
Member

Can you give us some more info why this will fix the problem?
I had created something similar #635 to debug nextcloud/android#8370 …

@mightymop
Copy link
Contributor Author

mightymop commented Jun 16, 2021 •
edited
Loading

Can you give us some more info why this will fix the problem?
I had created something similar #635 to debug nextcloud/android#8370 …

First i took a look into the corresponding source of the owncloud android client and realized that they are using the blocking method instead of the peek... Then i merged the peace of code into my nextcloud repo and did some tests... and could not reproduce the issue ... furthermore i have added some errorhandling to be sure that the code will not try to auth with a null password which would produce the ldap lock...

What the docs say:

peekAuthToken:
Gets an auth token from the AccountManager's cache. If no auth token is cached for this account, null will be returned -- a new auth token will not be generated, and the server will not be contacted. Intended for use by the authenticator, not directly by applications.

It is safe to call this method from the main thread.

blockingGetAuthToken

This convenience helper synchronously gets an auth token with getAuthToken(android.accounts.Account, java.lang.String, boolean, android.accounts.AccountManagerCallback, android.os.Handler).
This method may block while a network request completes, and must never be made from the main thread.

So the questions are:

Why is this code called, when there is no password in cache.
Why is there no password in cache?
etc...

Maybe we have to distinguish between creating a new account (then using blockingGetAuthToken to generate token/password)
and reusing an account (then using peekAuthToken to get token from cache)

@AndyScherzinger AndyScherzinger mentioned this pull request Jul 12, 2021
Closed
@mightymop
Copy link
Contributor Author

Are there any news on the related issue(s)? @tobiasKaminsky You mentioned that you had a similar issue? Could you fix it?

AlvaroBrey
AlvaroBrey previously approved these changes Nov 3, 2021
View reviewed changes
Copy link
Member

@AlvaroBrey AlvaroBrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the right approach. As createNextcloudClient is not called from the main thread, blockingGetAuthToken should be a safe call.

@AlvaroBrey
Merge branch 'master' into fix#8302
1d20fbd 
* master: (129 commits)
  Reset spotbugs
  Bump byte-buddy from 1.11.22 to 1.12.0
  analysis-wrapper: Remove extra spaces from spotbugs table
  Refactor spotbugs setup
  Revert "lint: Enable text report for CI build"
  workflows: Lint: upload lint results as artifacts
  workflows: Fix typo in lint actions
  lint: Enable text report for CI build
  lint: Remove lint.xml, disable InvalidPackage via build.gradle
  As lint 0 🎉 we do not need lint-up anymore, but can rely on plain lint GH action
  Drone: update Lint results to reflect reduced error/warning count [skip ci]
  Empty commit to trigger CI
  Suppress CustomX509TrustManager lint
  Drone: update Lint results to reflect reduced error/warning count [skip ci]
  Bump targetSdk and compileSdk to version 30
  Bump annotation from 1.2.0 to 1.3.0
  Do not check dependencies as we have DependAbot for this
  Fix code formatting via ktlintFormat
  Bump ktlint from 0.42.1 to 0.43.0
  Bump gson from 2.8.8 to 2.8.9
  ...
@AlvaroBrey
Fix spotbugs and formatting
1674556 
Signed-off-by: Álvaro Brey Vilas <alvaro.brey@nextcloud.com>
@nextcloud-android-bot
Copy link
Collaborator

SpotBugs (new)

Warning Type Number
Bad practice Warnings 14
Correctness Warnings 38
Internationalization Warnings 6
Malicious code vulnerability Warnings 52
Multithreaded correctness Warnings 3
Performance Warnings 14
Security Warnings 1
Dodgy code Warnings 42
Total 170

SpotBugs (master)

Warning Type Number
Bad practice Warnings 14
Correctness Warnings 38
Internationalization Warnings 6
Malicious code vulnerability Warnings 52
Multithreaded correctness Warnings 3
Performance Warnings 14
Security Warnings 1
Dodgy code Warnings 42
Total 170

@AlvaroBrey
Copy link
Member

/backport to stable-2.8

@AlvaroBrey AlvaroBrey merged commit 622bdb1 into nextcloud:master Nov 9, 2021
@backportbot-nextcloud
Copy link

The backport to stable-2.8 failed. Please do this backport manually.

@AlvaroBrey AlvaroBrey added this to the NC Android lib 2.9.0 milestone Nov 9, 2021
This was referenced Nov 9, 2021
Merged
Closed
@AlvaroBrey AlvaroBrey mentioned this pull request Nov 23, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlvaroBrey AlvaroBrey AlvaroBrey left review comments

@tobiasKaminsky tobiasKaminsky Awaiting requested review from tobiasKaminsky

Assignees
No one assigned
Labels
3. to review
Projects
None yet
Milestone
No milestone
5 participants
@mightymop @tobiasKaminsky @nextcloud-android-bot @AlvaroBrey @AndyScherzinger