-
Notifications
You must be signed in to change notification settings - Fork 87
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
clientlogin + 2FA #260
base: master
Are you sure you want to change the base?
clientlogin + 2FA #260
Conversation
5b2bc26
to
85f0612
Compare
To be able to login with 2FA enabled, we need to use clientlogin instead of login. In case of 2fa, a second http request is needed to send user, pass, totp (2fa code), fake loginreturnurl, rememberMe boolean and our classical token. It's the first commit, currently the login form works as usual for normal users. users with 2fa enabled want be connected yet. **So this commit is not ready to be merged for now.** Inspired by https://github.com/commons-app/apps-android-commons/blob/b0e8175003a686789474238dd293aa89d1e925c7/app/src/main/java/fr/free/nrw/commons/mwapi/ApacheHttpClientMediaWikiApi.java#L93 Bug: https://phabricator.wikimedia.org/T180279
85f0612
to
af06a68
Compare
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.
@benapetr I still have two main problems. Can you help me solve them? Thanks !
huggle/login.cpp
Outdated
query->UsingPOST = true; | ||
query->Process(); | ||
ApiQueryResult *result = query->GetApiQueryResult(); | ||
ApiQueryResultNode *ln = result->GetNode("clientlogin"); |
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 need to execute this second request, but I can't finish it correctly without spending it with other connection requests.
huggle/login.cpp
Outdated
if (status == "UI") { | ||
// Need a user interaction like captacha or 2FA | ||
//QString v_id = ln->ChildNodes.at(0)->GetAttribute("id", "unknown"); | ||
//if (v_id == "TOTPAuthenticationRequest"){ |
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.
I can't parse the xml tree to get back the TOTPAuthenticationRequest
value... Here is what contain the answer.
Isn't action=login deprecated anyway? I think we need to replace it with "clientlogin" at some point anyway or is that one also deprecated? |
Can you maybe create some diagram of how is this supposed to work? It's hard to review the code if I have no clue on how 2FA login is working within MW. Why is there even a need for callback URL, this isn't OAuth. |
huggle/apiquery.cpp
Outdated
@@ -416,6 +417,10 @@ void ApiQuery::SetAction(const Action action) | |||
this->ActionPart = "login"; | |||
this->EnforceLogin = false; | |||
return; | |||
case ClientLogin: |
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.
This needs to be called ActionClientLogin
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.
done
huggle/login.cpp
Outdated
@@ -554,21 +555,20 @@ void Login::PerformLoginPart2(WikiSite *site) | |||
this->Statuses[site] = WaitingForToken; | |||
this->LoginQueries.remove(site); | |||
query->DecRef(); | |||
query = new ApiQuery(ActionLogin, site); | |||
query = new ApiQuery(ClientLogin, site); |
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.
Are you sure that Bot login can use "ClientLogin"?
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, i can't log me in with botpassword... so it looks like that we need to leave both connection methods available in parallel :(
@addshore can you confirm that ClientLogin doesn't support botpassword ?
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.
I actually think that classic login was deprecated for everything except for bot passwords, basically in future it should replace classic login with bot password login only.
huggle/login.cpp
Outdated
query->IncRef(); | ||
query->Parameters = "username=" + QUrl::toPercentEncoding(hcfg->SystemConfig_BotLogin) | ||
+ "&password=" + QUrl::toPercentEncoding(hcfg->TemporaryConfig_Password) | ||
+ "&OATHToken=" + totp + "&loginreturnurl=http://example.com/&rememberMe=1&logintoken=" + QUrl::toPercentEncoding(this->Tokens[site]); |
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.
Why is loginreturnurl needed at all here
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.
yeah I'll use logincontinue
action=login looks depreciated, see https://www.mediawiki.org/w/api.php?action=help&modules=login :
Please look https://www.mediawiki.org/w/api.php?action=help&modules=clientlogin too. |
huggle/apiquery.cpp
Outdated
@@ -417,7 +417,7 @@ void ApiQuery::SetAction(const Action action) | |||
this->ActionPart = "login"; | |||
this->EnforceLogin = false; | |||
return; | |||
case ClientLogin: |
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.
I think you forgot to renamed it elsewhere because now it doesn't compile at all
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.
fixed
0ef6b2c
to
3e68481
Compare
This is how I imagine it should work, but it now doesn't:
|
To be able to login with 2FA enabled, we need to use clientlogin instead of login, and at least to correctly detect users with this security enabled.
In case of 2fa, a second http request is needed to send user, pass, totp (2fa code), fake loginreturnurl, rememberMe boolean and our classical token.
It's the first commit, currently the login form works as usual for normal users. users with 2fa enabled can' t be connected yet.
So this commit is not ready to be merged for now.
Inspired by https://github.com/commons-app/apps-android-commons/blob/b0e8175003a686789474238dd293aa89d1e925c7/app/src/main/java/fr/free/nrw/commons/mwapi/ApacheHttpClientMediaWikiApi.java#L93
Bug: https://phabricator.wikimedia.org/T180279