GitHub Two Factor Authentication
ClosedPublic

Authored by zhigalin on Aug 29 2016, 2:20 PM.

Details

Summary

I have added support for GitHub's two factor authentication when authorizing accounts, I have not yet looked at account revocation.

This has only been tested with my personal GitHub account.

Diff Detail

Repository
R32 KDevelop
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
ial0 updated this revision to Diff 6351.Aug 29 2016, 2:20 PM
ial0 retitled this revision from to GitHub Two Factor Authentication.
ial0 updated this object.
ial0 edited the test plan for this revision. (Show Details)
ial0 set the repository for this revision to R32 KDevelop.
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptAug 29 2016, 2:20 PM
kfunk requested changes to this revision.Aug 29 2016, 7:57 PM
kfunk added a reviewer: kfunk.
kfunk added a subscriber: kfunk.

@Other devs: If someone has 2FA enabled: Please give it a run and check whether it works for you.

providers/ghprovider/ghresource.cpp
93

Newline before {

168

const ...

170

const auto&

171

Isn't the casing fixed? Can it really vary?

This revision now requires changes to proceed.Aug 29 2016, 7:57 PM
ial0 updated this revision to Diff 6402.Sep 2 2016, 1:06 PM
ial0 edited edge metadata.

I've added const as suggested and the string test is now case sensitive as well.

mwolff requested changes to this revision.Feb 18 2017, 10:55 PM
mwolff added a subscriber: mwolff.

some nitpicks, otherwise this looks good to me.

I also don't have two factor authentication enabled, but if this works for you ial0, I think we should merge it

providers/ghprovider/ghdialog.cpp
157

rs -> resource

providers/ghprovider/ghresource.cpp
39

get -> create

43

+ QLatin1String("//authorizations")

is the double-slash really needed?

44

QByteArrayLiteral

46

QStringLiteral

87

QLatin1String("Authorization: Basic ") + QString::fromUtf8((name.toUtf8() + ':' + password.toUtf8()).toBase64())

94

QLatin1String for the string literal in the middle

169

QStringLiteral for the responsecode

170

QSL

171

QSL

172

QSL

This revision now requires changes to proceed.Feb 18 2017, 10:55 PM
kfunk added a comment.Apr 4 2017, 9:07 PM

Bump. Any future plans to work on this?

zhigalin commandeered this revision.Aug 1 2017, 1:09 PM
zhigalin added a reviewer: ial0.
zhigalin marked 13 inline comments as done.Aug 1 2017, 2:34 PM
zhigalin added inline comments.
providers/ghprovider/ghdialog.cpp
157

It's called rs everywhere...

zhigalin updated this revision to Diff 17503.Aug 1 2017, 2:39 PM
zhigalin edited edge metadata.

New version

kfunk requested changes to this revision.Aug 2 2017, 7:44 AM

Rest LGTM

providers/ghprovider/ghresource.cpp
171

"x-github-otp: required: Still incorrect casing. Does this work?

https://developer.github.com/v3/auth/ says it's X-GitHub-OTP: required;

172

Please pass m_tfHttpHeader via twoFactorRequested() instead I.e. add a parameter to that function. That way you can save the extra member in this class for m_tfHttpHeader.

And in general: Please refrain from adding less known abbreviations to symbol names; it makes reading code harder.

providers/ghprovider/ghresource.h
145

Rename: twoFactorRequested -> twoFactorAuthRequested

This revision now requires changes to proceed.Aug 2 2017, 7:44 AM
zhigalin planned changes to this revision.Aug 2 2017, 8:52 AM
zhigalin marked 3 inline comments as done.
zhigalin added inline comments.
providers/ghprovider/ghresource.cpp
171

https://developer.github.com/v3/auth/ says it's X-GitHub-OTP: required;

Than it lies :)
It's actually lowercase.
Anyway, just making it case-insensitive...

172

And in general: Please refrain from adding less known abbreviations to symbol names; it makes reading code harder.

m_tfHttpHeader was created by the previous author, not me

zhigalin updated this revision to Diff 17544.Aug 2 2017, 8:57 AM
zhigalin edited edge metadata.
zhigalin marked an inline comment as done.

Update

kfunk accepted this revision.Aug 2 2017, 9:59 AM

LGTM. Though I can't test myself, I don't have 2FA activated on Github...

Would be grateful if another person could test this patch.

providers/ghprovider/ghresource.cpp
171

Ugh.

Please add a note then :)

This revision was automatically updated to reflect the committed changes.