-
Notifications
You must be signed in to change notification settings - Fork 94
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
Quality of Life Updates for bad TLS versions #142
Conversation
…on handling if TLS is incorrect
I think this is a good idea and will help the SDK developers, but my only comment is that what happens when we move to TLS1.3? We then have to update this SDK to validate against the new TLS version. I think it makes sense though since it's just creating a more descriptive error for developers. |
The validation step is unobtrusive and simply checks to see if you're using below TLS 1.2 after a request has already failed, When we move to require 1.3 this would have to be updated to reflect that. We were going to have to do that anyway as the README tells folks to make sure they are targeting 1.2 already. |
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.
LGTM, just want to get clarity on the documentation
@@ -158,5 +159,21 @@ public static int GetPartnerIdFromSessionId(string sessionId) | |||
|
|||
return Convert.ToInt32(sessionParameters[1]); | |||
} | |||
|
|||
/// <summary> |
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.
@jeffswartz I noticed that the rest of the project uses a non-standard docblock (one more akin to PHP and Java). will using the proper documentation blocks break anything in regards to documentation building? The XML <summary>
format would be preferred, but before we merge this I want to make sure we won't break anything.
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.
@jeffswartz - will using standard .NET summary docs cause issues with our docs generator?
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.
@slorello89 Please remove the docs for this ValidateTlsVersion()
method. It is in OpenTokUtils, which does not include methods that developers will call directly.
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.
@slorello89 Our docs build script (which uses Doxygen) works with the JavaDocs comments. We should consider cleaning that up later. But for now, it works.
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.
Also, note that we do not document the other OpenTokUtils methods (which are used internally by the SDK).
As it stands now if someone uses a bad version of TLS the .NET SDK will fail with a generic error (previously it was a Null reference exception but that was fixed). This PR makes the error more explicit the developers seeing it. I've also changed the way the suggested patch is worded for TLS 1.2 support to inform folks that there is a way to set the TLS version via bitwise OR, which will not disable their other TLS versions (in case their app is consuming a other APIs that don't support TLS 1.2). Addresses #108