PR: Add authentication dialog to submit issues to Github

[dalthviz]

Fixes #6617

New design:

[pep8speaks]

Hello @dalthviz! Thanks for updating the PR.

• In the file spyder/utils/external/github.py, following are the PEP8 issues :

Line 32:80: E501 line too long (87 > 79 characters)
Line 41:80: E501 line too long (103 > 79 characters)
Line 61:5: E301 expected 1 blank line, found 0
Line 63:1: E722 do not use bare except'
Line 69:10: E401 multiple imports on one line
Line 73:8: E225 missing whitespace around operator
Line 86:1: E302 expected 2 blank lines, found 1
Line 95:9: E722 do not use bare except'
Line 100:1: E302 expected 2 blank lines, found 1
Line 114:1: E302 expected 2 blank lines, found 1
Line 122:1: E302 expected 2 blank lines, found 1
Line 137:1: E302 expected 2 blank lines, found 1
Line 144:21: E225 missing whitespace around operator
Line 150:16: E225 missing whitespace around operator
Line 152:16: E225 missing whitespace around operator
Line 154:16: E225 missing whitespace around operator
Line 156:16: E225 missing whitespace around operator
Line 158:16: E225 missing whitespace around operator
Line 168:1: E302 expected 2 blank lines, found 1
Line 174:80: E501 line too long (139 > 79 characters)
Line 181:80: E501 line too long (90 > 79 characters)
Line 207:80: E501 line too long (81 > 79 characters)
Line 215:80: E501 line too long (90 > 79 characters)
Line 221:80: E501 line too long (97 > 79 characters)
Line 239:19: E225 missing whitespace around operator
Line 250:80: E501 line too long (83 > 79 characters)
Line 264:25: E225 missing whitespace around operator
Line 273:21: E225 missing whitespace around operator
Line 275:23: E225 missing whitespace around operator
Line 277:23: E225 missing whitespace around operator
Line 279:23: E225 missing whitespace around operator
Line 283:1: E302 expected 2 blank lines, found 1
Line 296:1: E302 expected 2 blank lines, found 1
Line 303:1: E302 expected 2 blank lines, found 1
Line 308:1: E302 expected 2 blank lines, found 1
Line 311:1: E305 expected 2 blank lines after class or function definition, found 1

• In the file spyder/widgets/github/backend.py, following are the PEP8 issues :

Line 73:80: E501 line too long (80 > 79 characters)
Line 138:80: E501 line too long (83 > 79 characters)
Line 161:36: W291 trailing whitespace

• In the file spyder/widgets/tests/test_reporterror.py, following are the PEP8 issues :

Line 71:80: E501 line too long (94 > 79 characters)

Comment last updated on March 26, 2018 at 15:03 Hours UTC

[CAM-Gerlach]

Maybe I'm just dumb, but I tried and tried and could not get it to work with my account; I just got "Failed to create github issue, [sic], invalid credentials" every time. I tried my github username, email, lowercase, uppercase, etc but all to no avail. Either I'm making some silly mistake; something is terribly broken either with the backend or with the Ux, or it doesn't support any Github account with 2FA which increasingly should be becoming most of them these days.

Of course, its not good that around 1/10 of the issues we get are blank with the recently implemented copy/paste system, but if the issues I'm experiencing indeed hinder a significant proportion of users from reporting, particularly those smart enough to use 2FA (and thus likely to produce the most valuable/useful bug reports), then as slick as this is, it would likely be better to keep the current system. To be quite frank, missing reports presumably tend to be from users too disinterested, incompetent or limited in English literacy to be able to read and follow a simple instruction, and thus least likely to produce useful information relative to the time wasted.

Can we have others test this as well to get an idea of whether this problem isn't just something on my end?

EDIT: I see in the internal console:

WARNING:spyder.widgets.github.backend:failed to send bug report on github. response={'code': 401, 'json': {'message': 'Must specify two-factor authentication OTP code.', 'documentation_url': 'https://developer.github.com/v3/auth#working-with-two-factor-authentication'}}

Therefore, it would seem it doesn't support github accounts with 2FA, or at the very least easily. As such, I'm not sure its a wise idea to implement this if it bars a substantial and increasingly proportion of our most valuable users from submitting reports, relative to whatever fraction of low-quality reports we'd be missing out on with the current system, however imperfect. Thoughts?

EDIT 2: If there's no easy 2FA integration (or even if so, given the minor but non-trivial hassle of entering a password and possibly a two factor code vs. piggybacking off an existing logged-in browser session), maybe we can catch failed authentication attempts due to that error (or others, even) and fall back to the current method as a backup, or even have an interstitial dialog asking the user how they'd like to report it/whether they have a 2FA, etc. Then, we get nearly foolproof convenience and a slicker Ux for our less experienced users likely to not have 2FA but more probable to have an issue going through the "manual" process correctly, while old hands (more likely to have 2FA) comfortable with the latter procedure and want to save time logging in (if they can at all) can opt for the current system.

There's also the issue of privacy/security for those concerned about that, in that only offering the new system with a build in username/password dialog could accidentally or purposely lead to the compromise of their github account—they have no way to be sure we aren't sending their github credentials to our own server or malicious actors modified Spyder to do the same, at least without advanced packet sniffers or locked down granular firewalls, so they might want a more trustworthy option.

[ccordoba12]

Therefore, it would seem it doesn't support github accounts with 2FA, or at the very least easily. As such, I'm not sure its a wise idea to implement this if it bars a substantial and increasingly proportion of our most valuable users from submitting reports, relative to whatever fraction of low-quality reports we'd be missing out on with the current system, however imperfect. Thoughts?

Right, the dialog doesn't support 2 factor authentication. I'm moving this to 3.2.9 because I think you're right @CAM-Gerlach: we need to add it to the dialog (even if it's not present in QCrash).

We also have to decide the UI for this, but in principle I'd say we should show a text field called Token (instead of User and Password), and instruct users how to create a token on Github so they can enter it there.

There's also the issue of privacy/security for those concerned about that, in that only offering the new system with a build in username/password dialog could accidentally or purposely lead to the compromise of their github account—they have no way to be sure we aren't sending their github credentials to our own server or malicious actors modified Spyder to do the same, at least without advanced packet sniffers or locked down granular firewalls, so they might want a more trustworthy option.

The only thing we can do about this is adding a message to the dialog mentioning that we don't save nor send users private information anywhere and that they can use two factor authentication for increased security, if they prefer so.

[ccordoba12]

@dalthviz, our tests are hanging in test_report_issue_url. This is probably because the new dialog blocks the test. If that's the case, please avoid the dialog to be created when running_under_pytest is True.

[ccordoba12]

@dalthviz, please don't add more commits here. I'll finish this one next weekend.

[CAM-Gerlach]

@ccordoba12

We also have to decide the UI for this, but in principle I'd say we should show a text field called Token (instead of User and Password), and instruct users how to create a token on Github so they can enter it there.

I guess my concern is that on top of the existing overhead of having to manually log in to Github in Spyder (vs. likely already being logged in via their browser, with easier password manager/autofill/etc. integration, and that being persistent), this adds a fairly non-trivial procedure for the user to complete before being able to simply report an error, vs. the current mechanism of just copy and pasting. Furthermore, furthermore, this effort just goes toward tedious overhead, rather than writing a more helpful issue report. Therefore, I find it quite plausible that for all the attractive elegance of this approach, it would be likely turn off more users, particularly in terms of valuable ones invested/experienced enough in open source to use 2FA, vs. the present copy/paste system, and thus net us fewer quality reports.

This is why I'm suggesting we mitigate this (and probably for less effort than implementing 2FA, though we could still do that also) by offering the prominent option to fall back to the current submission if entering the user's credentials fails for any reason.

The only thing we can do about this is adding a message to the dialog mentioning that we don't save nor send users private information anywhere

Well, except to Github...or what we claim is Github, anyway. And wouldn't that be exactly what a malicious program would say? :) I would think if users are concerned about us "tracking" them by simply increment a counter on a server if they chose to check for updates and cannot trust we won't do anything malicious with that, I can't imagine how they'd react to inducing them to all but give us full access to their github accounts, other than choosing not to report at all without an alternative option being presented (which is what I'm suggesting.

and that they can use two factor authentication for increased security, if they prefer so.

2FA doesn't do much to defend against the immediate attack vector I'm suggesting (basically an evil maid attack of some sort) because not only the communications channel but the entire client and UI itself is untrusted. With a few lines of code, we could simply send the username and password to a remote machine to try to login, then just send the 2FA token the user entered the same way, and tell the user their login attempt failed for made up reason. Now, we have all the users details and are logged in indefinitely on a remote machine, and have full access to their account at which point we can easily lock them out and do whatever we like, or silently snoop until a later date. Of course, they can always look at the source code and trace it for themselves, but there's always the possibility of clever obfuscation.

[ccordoba12]

I can't imagine how they'd react to inducing them to all but give us full access to their github accounts, other than choosing not to report at all without an alternative option being presented (which is what I'm suggesting.

Sorry @CAM-Gerlach, I disagree with you on this one. If people don't want to enter their Github credentials, that's fine, they can choose to hide future errors. If we give them an alternative method to report, they'd most probably choose it instead, and we'd be in the exact same situation as we are now.

They will have to trust us on this one or not to report at all. But at least that will probably deter people from just pressing Submit to Github and forget about the rest, which is happening too often right now.

[ccordoba12]

Therefore, I find it quite plausible that for all the attractive elegance of this approach, it would be likely turn off more users, particularly in terms of valuable ones invested/experienced enough in open source to use 2FA, vs. the present copy/paste system, and thus net us fewer quality reports.

We receive very few quality reports right now, so I don't find your point valid.

And, there's also a manual way of opening an issue and fill all the info we require by using the report and dependencies dialogs. So that's the alternative method you're asking for.

[CAM-Gerlach]

@ccordoba12 Side question: without keyring, does storing the credentials and auth token not work, and they both need to be re-entered every time a report is filed?

Sorry @CAM-Gerlach, I disagree with you on this one. If people don't want to enter their Github credentials, that's fine, they can choose to hide future errors. If we give them an alternative method to report, they'd most probably choose it instead, and we'd be in the exact same situation as we are now.

We could only offer the option to do so if there was a problem/error with their login, or put it as a third button in the github credential dialog so they have to look carefully and deliberately to see it, rather than the much more rote/common Okay or Cancel interaction patterns that the user would be used to automatically clicking on. Therefore, the option would be there if the user was sufficiently motivated to look for it, while only being a backup unless something went wrong with the default flow. I just don't see where removing the option entirely really solves our actual problem versus the potential for missing a greater proportion of good reports and increased user frustration.

But at least that will probably deter people from just pressing Submit to Github and forget about the rest, which is happening too often right now.

Its quite likely I just don't fully understand the implications of this PR, but I'm curious what exactly this buys us over that status quo. It adds the extra busywork of entering their username and password, which could certainly deter some users of all stripes, but doesn't necessarily add to the effort they put into their description. Those desperate for help or a fix without bothering to read the existing text will still blindly click through (and are the least likely to have 2FA, password managers, long passwords, etc to worry about), while those more involved with github and open source are impacted more due to the greater value of their time and are more likely to have to go through all the extra steps for 2FA, etc.

If this is really just about setting the effort bar higher, which I'm all for, I would think that goal would be much better served by more direct means that more closely correlate with, and contribute to, a high quality report. For example, we could significantly increasing the length requirement for the description (to ~>100 characters, i.e. ~ >10-20 words), and thus the extra effort is both directly a proxy for and productive for the purpose of making the actual report better and more detailed, rather than a considerable amount of extraneous tedious work just to achieve the same final result. Also, other repos have a checklist that must be filled out as part of their issue template or the issue is closed; we could include something right in the UI as well and disable reporting by any means until all the boxes are checked.

• Read the troubleshooting guide
• Searched for other issues with the same error
• Updated Spyder to the latest version with conda update spyder
• Confirmed the issue is not related to my code, a specific external package or plugin, or my Anaconda installation, and does not occur in a standalone jupyter qtconsole (if console-related)

etc.

In fact, we could even disable reporting if the update check detected they weren't using the latest version and ask them to update first, or at least require them to click through a special warning dialog if so. These would all much better directly serve the purpose at hand than artificially adding unrelated effort that disproportionately affects those who are more involved.

To be quite frank, as much as like you I am not a fan of a lack of care , thought and attention i.e. taking the time to comprehensively investigate the problem and write a detailed issue report, but I'm afraid when it comes to grunt physical effort I am a rather lazy person with a lot of inertia, If I had to go through the hassle of manually extracting my github credentials from my password manager and copy-pasting them, then logging in, generating an authentication key and entering that all to report a single issue, I'm not entirely sure I would have done so, and in turn become involved in the project at all (ironically, with the specific purpose of helping cut down on your mundane issue load :). Not to say everyone fits that same profile, though...but something to consider.

We receive very few quality reports right now, so I don't find your point valid.

That is true, but I'm not sure I understand how the simple fact that we are receiving a low percentage of good reports right now supports the claim that adding extra difficulty for those most likely to send a high quality report (2FA users/password manager users/those with long password), and reduce the relative bar for those that are least likely to (lacking the English proficiency/competence/care to read and follow basic instructions to copy/paste the report) will likely lead to a meaningfully higher average report quality. Perhaps there is there another component of this system that I'm not considering that achieves that?

And, there's also a manual way of opening an issue and fill all the info we require by using the report and dependencies dialogs. So that's the alternative method you're asking for.

That, and also the "Report to Github" menu option that still uses the old system; fair point. However, this also increases the change they forget to include the traceback, a long enough description, the version information, etc where at least that much is guaranteed with the automatic report.

[ccordoba12]

@CAM-Gerlach, this should be ready. Please test it in your end by running:

PYTHONPATH=. python spyder/widgets/reporterror.py

By providing the minimum number of title and description characters, you'll be able to press Submit to Github, then authenticate to send a fake issue. It'll be opened on my Spyder fork instead of here, to no pollute our own tracker with garbage.

---

Regarding your last comments:

We could only offer the option to do so if there was a problem/error with their login

Yes, good point. I added our current method to report an issue (i.e. paste the issue contents on GH) as fallback in case users have any error (other than authentication issues) when trying to send the issue automatically.

Also, other repos have a checklist that must be filled out as part of their issue template or the issue is closed; we could include something right in the UI as well and disable reporting by any means until all the boxes are checked.

I agree with this. Please create a new PR to improve our current template. The problem is very few people create reports manually now. But it could be worth the effort.

If I had to go through the hassle of manually extracting my github credentials from my password manager and copy-pasting them, then logging in, generating an authentication key and entering that all to report a single issue, I'm not entirely sure I would have done so

I'll add the possibility to save the user credentials using the keyring package on Windows and macOS in another PR (I'm updating the deps needed for that in conda-forge right now). That's not easy to do on Linux for now. It'll be in the short term, but only for Python 3.5+. But at least we'll cover the majority of users (very few people report from Linux).

[ccordoba12]

@CAM-Gerlach or @spyder-ide/core-developers, @spyder-ide/junior-developers, any word about this? I'd like to merge it soon so I can finish the integration with the keyring package (which is finally updated in conda-forge).

[CAM-Gerlach]

@ccordoba12 Sorry; I must have missed your comment to me in the flurry of commits. Testing now.

[CAM-Gerlach]

Looking slick @ccordoba12 . My minor feedback—

On the error report dialog itself:

• Title and Steps to reproduce need colons after them
• Steps to reproduce and the "Please enter below..." block should be the same size as the "Before reporting it" text, as it is very small, hard to read/notice, and inconsistent.
• Grammar (possibly my mistake from prev version): "Please enter below a detailed" -> "Please enter a detailed"
• The tab order needs to be fixed; naturally the user is going to want to tab from the title field to the description, but instead of it being next, they have to tab through all the other UI elements first.

UX on the sign in dialog could be improved, especially for token authentication, as even though I knew what to look for, I found it unclear and confusing as to what to do. It still was a non-trivial process to remember where to go, what to do, and which permissions to assign, as well as a bit of confusion on the dialog itself. Therefore, I have the following suggestions:

• It would be clearer, IMO, to title the two tabs "Password Only" and "Access Token (2FA Users)" or something along those lines, to A. Make it clear who should use which (as basic/token don't obviously map to (password/2FA), and B. Imply that 2FA users only need to complete the second tab, not both (I wasn't sure about this point myself until I tried it.
• Additionally, maybe include a line of text in each tab reading "For users without two-factor authentication enabled" and "For users with two-factor authentication, or who prefer a per-app token" to further make the distinction clear?
• I had to Google both the location of the page to create the correct type of token (I looked under Security, 2FA settings, SSH keys, and Account to no avail, not realizing it was hidden under "Developer Settings), and also the exact permission setting out of the dozens present to enable; a less determined user would stand a good chance of doing something incorrectly or just giving up. Therefore, you could include a line of text like "Go here and click "Generate" to create a new personal access token with the appropriate permissions." which will automatically open the page to create the token with the correct settings and an appropriate name all pre-filled, only requiring them to click through and copy/paste the new token.

Finally, in order to make sure the full report (with traceback, environment/dep info, etc) works and looks good, if I trigger a report from normal Spyder can I get it to go to your repo as well for testing?

[CAM-Gerlach]

@ccordoba12 When testing the fallback functionality, there is a typo in the dialog text: a missing space between "it" and "manually". Also, you could add a line to the effect of "If so, make sure to paste your clipboard into the issue report box that will appear in a new browser tab before clicking submit on that page." Finally, are we still keeping the legacy/manual reporting only for the Help > Report Issue menu item?

I agree with this. Please create a new PR to improve our current template. The problem is very few people create reports manually now. But it could be worth the effort.

This should be fairly straightforward to do wrt just the template; we can put them on the report sent via the "Report Issue" menu item as well. The other part of my suggestion was actually implementing at least a few of the checkboxes on the Report Error dialog itself, and not enabling submission until they were checked. However, if we did that, we'd at a minimum want to make the dialog non-modal, to make sure the user can actually complete all those steps, and there might be other UX considerations to think about. If you like the idea, I can prepare a followup PR once this gets merged.

[ccordoba12]

On the error report dialog itself:

I implemented all your suggestions.

UX on the sign in dialog could be improved, especially for token authentication

I totally agree with this. You could implement your suggestions after I merge this PR.

Finally, in order to make sure the full report (with traceback, environment/dep info, etc) works and looks good, if I trigger a report from normal Spyder can I get it to go to your repo as well for testing?

If the error dialog is called from the main window then issues will be opened here. But I see no major problems with that. Just try to not open too many fake issues :-)

Finally, are we still keeping the legacy/manual reporting only for the Help > Report Issue menu item?

Sure, why not? It's working as expected and it's already implemented.

The other part of my suggestion was actually implementing at least a few of the checkboxes on the Report Error dialog itself

I wouldn't want to put too many things in the error dialog to not confuse people. I think demanding a title and description is good enough to give us better issues.

If you like the idea, I can prepare a followup PR once this gets merged.

I'd have to see something implemented, but I can't promise you that I'll merge it. I mean, I think it's better to discuss things around a PR than simply in the abstract. So, it's up to you.

---

By the way, thanks for the very good review!

[ccordoba12]

Ok, let's merge this one so we can continue iterating over this.

[CAM-Gerlach]

I implemented all your suggestions.

Thanks! Glad they were helpful.

I totally agree with this. You could implement your suggestions after I merge this PR.

Will do, hopefully this evening, or if not tomorrow.

If the error dialog is called from the main window then issues will be opened here. But I see no major problems with that. Just try to not open too many fake issues :-)

I'll open one at some point, just to check. Hopefully won't need more than that, assuming its working as expected.

Sure, why not? It's working as expected and it's already implemented.

Makes good sense.

I wouldn't want to put too many things in the error dialog to not confuse people. I think demanding a title and description is good enough to give us better issues.

Hmm, okay. As long as its alright to modify the error dialog once Spyder 3 goes into LTS mode if we still get a lot of bogus issues, we can certainly adopt a "try and see" approach. Assuming users are meaningfully filling out the dialog, the biggest gap I see it not really addressing (unless users actually click through to view other issues) are duplicate issues, that form a large majority of the non-invalid issue reports. Further, many/most could have been solved by just updating Spyder since they are already fixed in a released version,

Therefore, my thought is that some sort of UI element(s) to address that could be helpful; being UI text it will be translated to their native language, so assuming the user is literate/capable of submitting a decent issue and the prose is clear, I'm not sure how they would be likely to be confused. In any case, I'll play around with something and submit a PR, and you can see what you think.