-
Notifications
You must be signed in to change notification settings - Fork 731
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
Support for login by m.login.token during QR code sign in #7358
Conversation
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.
LGTM, some remark, which may be handled later.
Also when there is new API on the SDK, it's good to add a changelog file with .sdk
extension. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog for more details.
thanks!
@@ -404,6 +406,22 @@ internal class DefaultAuthenticationService @Inject constructor( | |||
) | |||
} | |||
|
|||
override suspend fun loginUsingQrLoginToken( | |||
homeServerConnectionConfig: HomeServerConnectionConfig, | |||
loginToken: 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.
Is it possible to pass all the data from the QR code here? So that the SDK can handle the next step automatically?
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.
At this point we don't actually have the full context yet, so it probably doesn't make sense to do this.
We can look at the overall implementation once done and see what refactoring to do.
...k-android/src/main/java/org/matrix/android/sdk/internal/auth/DefaultAuthenticationService.kt
Outdated
Show resolved
Hide resolved
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.
LGTM! I will check the test failure and merge the PR if it's fine.
SonarCloud Quality Gate failed. |
Type of change
Content
Motivation and context
Screenshots / GIFs
Tests
Tested devices
Checklist
Signed-off-by: Hugh Nimmo-Smith [email protected]