Support disabling certificate revocation checks on Windows#9188
Support disabling certificate revocation checks on Windows#9188outofambit merged 26 commits intodevelopmentfrom
Conversation
|
@niik Thanks! I'm curious whether this should live in "Advanced" or in "Git." It felt a bit odd to me that there would be a subheading of "Git" in the Advanced tab, as opposed to just putting it in the Git tab. But maybe we don't want to expose that unless you really should be changing it, and therefore Advanced makes sense. Additionally, I assume this option just becomes available for everyone, or should we scope it to only people who it's relevant for? |
Yeah, I went back and forth on that a few times. FWIW it felt pretty weird having an "Advanced" section in the Git tab as well 😛 I tried doing a optional section in the Git tab
Ultimately though I felt like I wanted to emphasize the advanced nature of the preference and sticking it in the tab which deals with something so high-level as name and email felt... wrong I guess? I'm not married to the current state but the above is the thought process leading up to it. If you feel strongly about not having it the advanced tab I'd be happy to revisit.
So right now it's only visible for Windows users but it is visible for all of them. I considered hiding it unless they've explicitly chosen to opt out. Considering that we've both had the same thought I think we should probably try that direction. I'll make it so |
|
Thanks @niik. I'm good with everything you said and the current location of "Advanced" given the limited scope of people who this will be relevant for. |
outofambit
left a comment
There was a problem hiding this comment.
This looks good. I've one question I'd like to talk about but I don't think its a blocker to getting this on beta.
| private scheduleResizeEvent = () => { | ||
| if (this.resizeDebounceId !== null) { | ||
| cancelAnimationFrame(this.resizeDebounceId) | ||
| this.resizeDebounceId = null | ||
| } | ||
| this.resizeDebounceId = requestAnimationFrame(this.onResized) | ||
| } | ||
|
|
||
| /** | ||
| * Attempt to ensure that the entire dialog is always visible. Chromium | ||
| * takes care of positioning the dialog when we initially show it but | ||
| * subsequent resizes of either the dialog (such as when switching tabs | ||
| * in the preferences dialog) or the Window doesn't affect positioning. | ||
| */ | ||
| private onResized = () => { | ||
| if (!this.dialogElement) { | ||
| return | ||
| } | ||
|
|
||
| const { offsetTop, offsetHeight } = this.dialogElement | ||
|
|
||
| // Not much we can do if the dialog is bigger than the window | ||
| if (offsetHeight > window.innerHeight - titleBarHeight) { | ||
| return | ||
| } | ||
|
|
||
| const padding = 10 | ||
| const overflow = offsetTop + offsetHeight + padding - window.innerHeight | ||
|
|
||
| if (overflow > 0) { | ||
| const top = Math.max(titleBarHeight, offsetTop - overflow) | ||
| this.dialogElement.style.top = `${top}px` | ||
| } |
There was a problem hiding this comment.
i would really like to solve this in our scss if possible. @niik is there something preventing us from writing roughly equivalent logic into our base dialog styles?
There was a problem hiding this comment.
is there something preventing us from writing roughly equivalent logic into our base dialog styles?
Yeah, this is something we talked about at when we initially implemented the dialogs. The chromium logic takes care of centering the dialog when it's first opened but doesn't move the dialog after that. That's the behavior we want. If we were to use CSS to position all dialogs at the vertical midpoint they'd start jumping around when switching between tabs in dialogs or when the contents of the dialog changes for other reasons (error messages dynamically appearing/disappearing for example).
The problem we're running into here is when a dialog can have wildly different heights depending on its configuration which is the case for the preferences dialog where the entry tab is very short:
And the advanced tab is much much taller
I see this change in this PR as a safe-guard to ensure that we don't ever end up in scenarios where the dialog ends up extending beyond the bounds of the window. A much better solution here would be to rebalance the tabs in the preferences dialog so that each tab is more uniform in height. We could for example add the "save/cancel" footer to the accounts and theme tabs and we could consider extracting the "show a confirmation dialog when..." section of the advanced tab into its own "Prompts" tab.
TL;DR; I don't see this as a solution in any way shape or form. Our dialogs aren't intended to differ wildly in height and we should ensure that the preferences dialog doesn't do that either. It is, however, a safe-guard.
Closes #3326, #8748
Description
A (hopefully temporary) workaround for the issues outlined in #9154 when using GitHub Desktop with a MITM proxy server which issues certificates without CRL Distribution Point attributes. This PR only affects Windows. See our known issues document for more details.
In addition to the dialog allowing users to disable certificate revocation checks there’s also some seemingly unrelated changes in this PR that deals with ensuring that a dialog is positioned within the bounds of the application window. This change was necessary because when adding the new checkbox to the advanced section of the preferences dialog the dialog became so tall that when switching to the advanced section with the app window at its minimum size the save and cancel buttons would be positioned below the window bounds.
Isn't this a terribly heavy hammer to use?
Yeah, it sure is. What we're ultimately striving towards if to have the same behavior as we do on macOS today, i.e. that we'll attempt to check the revocation status of a certificate but not error if the revocation check fails, i.e. a best-effort type of revocation check. This is coincidentally what all major browsers and operating systems do as well.
Getting to that point will require changes to several upstream projects and the cooperation of their maintainers so it's not something we can plan on achieving in time for the next release. @dscho has been absolutely wonderful in helping us get the ball rolling on this.
Screenshots
TODO