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

Fixes for the SMS 2FA #152

Merged
merged 5 commits into from
Aug 16, 2024
Merged

Fixes for the SMS 2FA #152

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
394970a
Bump fluxc
hichamboushaba Aug 12, 2024
5e21a05
Show the SMS button when no auth types are provided
hichamboushaba Aug 13, 2024
c511664
Remove unused variable
hichamboushaba Aug 15, 2024
94e7747
Attempt to fix build issue
hichamboushaba Aug 15, 2024
495c669
Bump fluxc to trunk
hichamboushaba Aug 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions WordPressLoginFlow/src/main/java/org/wordpress/android/login/Login2FaFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package org.wordpress.android.login;

import static android.content.Context.CLIPBOARD_SERVICE;

import android.content.ClipboardManager;
import android.content.Context;
import android.os.Bundle;
Expand Down Expand Up @@ -53,8 +55,6 @@
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import static android.content.Context.CLIPBOARD_SERVICE;

import dagger.android.support.AndroidSupportInjection;

public class Login2FaFragment extends LoginBaseFormFragment<LoginListener> implements TextWatcher,
Expand Down Expand Up @@ -132,7 +132,6 @@ public static Login2FaFragment newInstance(String emailAddress, String password,
String userId, String webauthnNonce,
String authenticatorNonce, String backupNonce,
String smsNonce, List<String> authTypes) {
boolean supportsWebauthn = webauthnNonce != null && !webauthnNonce.isEmpty();
Login2FaFragment fragment = new Login2FaFragment();
Bundle args = new Bundle();
args.putString(ARG_EMAIL_ADDRESS, emailAddress);
Expand Down Expand Up @@ -208,7 +207,9 @@ protected void setupContent(ViewGroup rootView) {
// restrict the allowed input chars to just numbers
m2FaInput.getEditText().setKeyListener(DigitsKeyListener.getInstance("0123456789"));

boolean isSmsEnabled = mSupportedAuthTypes.contains(SupportedAuthTypes.PUSH);
// If we didn't get a list of supported auth types, then the flow is not using webauthn,
// We should treat it as if SMS is enabled for the user
boolean isSmsEnabled = mSupportedAuthTypes.isEmpty() || mSupportedAuthTypes.contains(SupportedAuthTypes.PUSH);
mOtpButton = rootView.findViewById(R.id.login_otp_button);
mOtpButton.setVisibility(isSmsEnabled ? View.VISIBLE : View.GONE);
mOtpButton.setText(mSentSmsCode ? R.string.login_text_otp_another : R.string.login_text_otp);
Expand Down
8 changes: 7 additions & 1 deletion build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ ext {
// libs
wordpressLintVersion = '2.1.0'
wordpressUtilsVersion = '3.5.0'
wordpressFluxCVersion = 'trunk-ed60798b4d96ec19863c74b0f525e2e20f4525db'
wordpressFluxCVersion = 'trunk-863f213e0e3a7a4322032e8e8c57a99f77bb48ec'
gravatarSdkVersion = '0.2.0'

// main
Expand Down Expand Up @@ -48,6 +48,12 @@ allprojects {
repositories {
google()
mavenCentral()
maven {
url "https://a8c-libs.s3.amazonaws.com/android/jcenter-mirror"
content {
includeVersion "com.android.volley", "volley", "1.1.1"
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to add this to make the CI work, I'm not sure what caused the new failures 😕, looking at maven listing of this dependency, we can see it's not available in neither the Google nor the Maven Central repositories, this line should have been needed even before 🫤.

cc @ParaskP7 any ideas.

Anyway, this PR is urgent, and this should block the merge regardless of the cause.

Copy link
Contributor

Choose a reason for hiding this comment

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

👋 @hichamboushaba and thanks for the ping!

Yea, I am not 100% sure what happened there, but this seems pretty similar to this other case of such a miss (p1723103849516809/1723070238.781829-slack-C036Y8QL4). 🤔

I think that jcenter [link] is just deleting artifacts in general, maybe more aggressively nowadays, and we just can’t download that specific one from anywhere atm (or at least from those 2 places that we know of: [1] + [2]). It is good that we at least have there available on our own jcenter-mirror repo. 🍀

PS: Thanks for making it work for yourselves by adding the jcenter-mirror configuration, just like it was done on multiple other repos of ours. 🥇

}
}
}

if (tasks.findByPath('checkstyle') == null) {
Expand Down
Loading