-
Notifications
You must be signed in to change notification settings - Fork 738
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
Handle invalid tokens gracefully #768
Conversation
and avoid duplicated code to manage default onError() in Login fragment
// For M_LIMIT_EXCEEDED | ||
@Json(name = "retry_after_ms") val retryAfterMillis: Long? = null, | ||
// For M_UNKNOWN_TOKEN | ||
@Json(name = "soft_logout") val isSoftLogout: Boolean = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be optional?
I am not sure how moshi manages non optional with default value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's working like that... Even when the param is not present...
const val M_USER_IN_USE = "M_USER_IN_USE" | ||
/** Sent when the room alias given to the createRoom API is already in use. */ | ||
const val M_ROOM_IN_USE = "M_ROOM_IN_USE" | ||
/** (Not documented yet) */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌 ;)
EventBus.getDefault().post(GlobalError.ConsentNotGivenError(matrixError.consentUri)) | ||
} else if (httpCode == HttpURLConnection.HTTP_UNAUTHORIZED /* 401 */ | ||
&& matrixError.code == MatrixError.M_UNKNOWN_TOKEN) { | ||
// TODO Check that this is ok during the login flow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cleaned up, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(and checked :))
|
||
internal interface SignInAgainTask : Task<SignInAgainTask.Params, Unit> { | ||
data class Params( | ||
val password: String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
store password as char[] instead of string? and wipe it when finished.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't it too much paranoid?
|
||
mainActivityStarted = true | ||
|
||
MainActivity.restartApp(this, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
restartApp is making me thinking that there still is the exit(0) thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No exit(0) :)
|
||
override fun bind(holder: Holder) { | ||
holder.textView.text = text | ||
holder.buttonView.setOnClickListener { listener?.invoke() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create clickListener in ViewHolder (and bind the listener in bind())?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should do this in another optimization PR.
Also this is a copy paste of ErrorWithRetryItem class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some remarks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-login with sso tested ok
Tested the clear data, and checked that filesystem related info is cleared
Handle invalid token by cleaning local data.
Also handle the soft logout case (#281)
The login/registration flow has been touched, they have to be tested again.
Also we have open issues on Synapse which makes that all does not work properly:
For reviewers: all cases should be handled. Please also check that there are no regression on the sign in and sign up flows. Thanks
Some screenshots:
Invalid token:
Soft logout with password:
SoftLogout when login flow is not supported (for SSO the wording of the button is "Continue with SSO")