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.
mwolff | |
kfunk | |
ial0 |
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.
No Linters Available |
No Unit Test Coverage |
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 | ||
---|---|---|
140 | rs -> resource | |
providers/ghprovider/ghresource.cpp | ||
38 | get -> create | |
42 | + QLatin1String("//authorizations") is the double-slash really needed? | |
43 | QByteArrayLiteral | |
45 | QStringLiteral | |
72 | QLatin1String("Authorization: Basic ") + QString::fromUtf8((name.toUtf8() + ':' + password.toUtf8()).toBase64()) | |
79 | QLatin1String for the string literal in the middle | |
156 | QStringLiteral for the responsecode | |
157 | QSL | |
158 | QSL | |
159 | QSL |
providers/ghprovider/ghdialog.cpp | ||
---|---|---|
140 | It's called rs everywhere... |
Rest LGTM
providers/ghprovider/ghresource.cpp | ||
---|---|---|
158 | "x-github-otp: required: Still incorrect casing. Does this work? https://developer.github.com/v3/auth/ says it's X-GitHub-OTP: required; | |
159 | 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 | ||
142 | Rename: twoFactorRequested -> twoFactorAuthRequested |
providers/ghprovider/ghresource.cpp | ||
---|---|---|
158 |
Than it lies :) | |
159 |
m_tfHttpHeader was created by the previous author, not me |
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 | ||
---|---|---|
158 | Ugh. Please add a note then :) |