[MI-51272]: Refactored and removed unused code. #150
Conversation
… to "Close" on filter popover. (#17) (#21) * [MI-2504][webapp]: Changed Hide to Close on filter popover * [MI-2504][server]: Generated manifest files * [MI-2504][server]: Updated version in manifest Co-authored-by: Abhishek Verma <[email protected]> Co-authored-by: Abhishek Verma <[email protected]>
…API and fixed Boards update subscription. (#22) * [MI-2505][server]: Added logic to protect subscriptions notification webhook API and fixed Boards update subscription. * [MI-2505][MI-2518] Fix failing testcases * [MI-2505]:Added webhook secret encoding and review fixes * [MI-2505]:Added webhook secret encryption * [MI-2505]: Fixed CI * [MI-2505]: Reverted change of auth scopes * [MI-2505]: Fixed CI * [MI-2505][MI-2603] Fixed testcases * [MI-2505]: Used constant for path * [MI-2505]: Refinded message * [MI-2505]: Minor review fixes * [MI-2505][MI-2603] Review fix Co-authored-by: Abhishek Verma <[email protected]> Co-authored-by: raghavaggarwal2308 <[email protected]>
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #150 +/- ##
==========================================
+ Coverage 65.65% 65.84% +0.19%
==========================================
Files 15 15
Lines 3313 3300 -13
==========================================
- Hits 2175 2173 -2
+ Misses 1013 1003 -10
+ Partials 125 124 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
server/plugin/client.go
Outdated
| } | ||
| encodedWebhookSecret := c.plugin.Encode(encryptedWebhookSecret) | ||
|
|
||
| encodedWebhookSecret := c.plugin.Encode([]byte(c.plugin.getConfiguration().WebhookSecret)) |
There was a problem hiding this comment.
Just so I understand, is this base64 encoding required?
There was a problem hiding this comment.
+1 on @mickmister comment, is the encoding/decoding needed?
There was a problem hiding this comment.
we are generating webhookSecret using generated type in the plugin's setting which adds some characters that are not accepted by Azure DevOps APIs so we are encoding them before sending
There was a problem hiding this comment.
Doesn't the generated value only use the base64 charset to begin with? https://github.com/mattermost/mattermost-webapp/blob/780bbe87b2b40de0cbb6359648a90aae672f5ff5/components/admin_console/generated_setting.tsx#L38-L42
There was a problem hiding this comment.
Not sure if this is working as expected as we are getting - in the generated field value which is not a base64 character and so the URL is breaking
for example, this is a randomly generated field value Y3081salbfxIcUZgx1sFoBwLjwAx5-Rk which has -
There was a problem hiding this comment.
Right. It's supposed to be the URL-friendly variant of Base64, but there is a bug there.
I've filed MM-51451.
Regardless of the bug, a better solution here would be coercing the value back to the standard base64-encoding rather than re-encoding it:
strings.ReplaceAll(strings.ReplaceAll(c.plugin.getConfiguration().WebhookSecret, "_", "/"), "-", "+")There was a problem hiding this comment.
Sure it's better to coerce
There was a problem hiding this comment.
Some of the characters are not valid as query-param like + is replaced with (space) when parsing the query-param so to avoid such edge cases have escaped the string so it can be safely placed inside a URL query using method url.QueryEscape which is working correctly for all such cases.
Example of some generated values that creates issue gc6y-Uayt+aufBbwZ73NCCsAYZH62aiP, x3NI6UxUIxifIaUkQqBpU5b+oiX+YKKF
There was a problem hiding this comment.
url.QueryEscape does not escape - which you previously mentioned as problematic. And + is part of the standard Base64 charset, which is what you were originally using but now that doesn't work either?
Are you testing this properly? If you know which characters the API accepts and which it doesn't, can you add an unit test for that?
Also is there any reason to wrap the url.QueryEscape in another function call? Seems like it's just unnecessarily complicating things.
There was a problem hiding this comment.
There is no written doc for supported chars but I tried for multiple chars seems like API doesn't have a problem with accepting them, it was the query-param parsing that was giving errors while reading the secret returned from the API, if we pass escaped string everything seems to be working now, have tested this manually.
Regarding the test case, it seems like all encoded characters are sent and received properly, let me know if we still need to add a separate test case for it as I could not get any char generated randomly in the plugin's generated field causing the issue now so, if we add test case it will be mostly valid ones.
m1lt0n
added
4: Reviews Complete
|
@DHaussermann based on the planning discussion today then, we can merge this right? cc: @mickmister |
See [MI-51272]