-
Notifications
You must be signed in to change notification settings - Fork 129
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
[GH-626]: Supported filtering on comment visibility for subscriptions #894
base: master
Are you sure you want to change the base?
Changes from 3 commits
4802fb0
444352c
1064968
2edf22e
649d520
ff40054
279ed3d
8929dfe
1d1b82a
8fbe0e9
70efc7e
5e21750
1095514
2b316c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,8 @@ | |
package main | ||
|
||
import ( | ||
jira "github.com/andygrunwald/go-jira" | ||
|
||
"github.com/mattermost/mattermost-plugin-jira/server/utils/types" | ||
) | ||
|
||
|
@@ -27,6 +29,30 @@ func (ww webhookWorker) work() { | |
} | ||
} | ||
|
||
func isCommentRelatedWebhook(wh Webhook) bool { | ||
return wh.Events().Intersection(commentEvents).Len() > 0 | ||
} | ||
|
||
func (ww webhookWorker) getVisibilityAttribute(msg *webhookMessage, v *webhook) (string, error) { | ||
raghavaggarwal2308 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
mattermostUserID, err := ww.p.userStore.LoadMattermostUserID(msg.InstanceID, v.JiraWebhook.Comment.Author.AccountID) | ||
if err != nil { | ||
ww.p.API.LogInfo("Commentator is not connected with the mattermost", "Error", err.Error()) | ||
raghavaggarwal2308 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return "", err | ||
} | ||
|
||
client, _, _, err := ww.p.getClient(msg.InstanceID, mattermostUserID) | ||
if err != nil { | ||
return "", err | ||
} | ||
|
||
comment := jira.Comment{} | ||
if err = client.RESTGet(v.JiraWebhook.Comment.Self, nil, &comment); err != nil { | ||
return "", err | ||
} | ||
|
||
return comment.Visibility.Value, nil | ||
} | ||
|
||
func (ww webhookWorker) process(msg *webhookMessage) (err error) { | ||
defer func() { | ||
if err == ErrWebhookIgnored { | ||
|
@@ -49,7 +75,14 @@ func (ww webhookWorker) process(msg *webhookMessage) (err error) { | |
return err | ||
} | ||
|
||
channelsSubscribed, err := ww.p.getChannelsSubscribed(v, msg.InstanceID) | ||
visibilityAttribute := "" | ||
if isCommentRelatedWebhook(wh) { | ||
if visibilityAttribute, err = ww.getVisibilityAttribute(msg, v); err != nil { | ||
return err | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the comment author is not connected to Mattermost, we end up not processing the webhook event no matter what. I think there's a design gap in the security model of this feature. This is a difficult problem to solve, the case of a comment author not connected to Mattermost. Let's say a subscription is set to "comment visibility must be empty". If we can't fetch the comment because the author is not connected to MM, then we can't know the visibility of the comment. And so we would then always be avoiding the webhook events with every comment made by a user that is not connected. That's not really behavior that we want. Though we can only assume that it is a sensitive comment if we can't fetch its visibility. Otherwise the feature would create posts for sensitive comments. @esarafianou Do you have any thoughts on this? Essentially, this feature allows subscriptions to filter out sensitive comments, but if the comment author is not connected to MM, then we have no way of knowing whether the comment is sensitive or not. Defaulting to secure avoids unwanted data leakage, but it then requires all comment authors to be connected to MM for any comments to be posted ever. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mickmister Seems I had missed this. Is this still relevant? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mickmister I have removed the extra API call for getting the comment visibility data as in the current implementation of Jira plugin we are always making an API call to get the comment event data. So, the user have to be connected in every case of comment events. This will be fixed with the API token approach we discussed earlier. So, maybe we can proeed with this for now and it will be fixed later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. API token approch implemented via #1102 |
||
|
||
channelsSubscribed, err := ww.p.getChannelsSubscribed(v, msg.InstanceID, visibilityAttribute) | ||
if err != nil { | ||
return err | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. | ||
// See LICENSE.txt for license information. | ||
|
||
import {connect} from 'react-redux'; | ||
import {bindActionCreators} from 'redux'; | ||
|
||
import {searchCommentVisibilityFields} from 'actions'; | ||
|
||
import JiraCommentVisibilitySelector from './jira_commentvisibility_selector'; | ||
|
||
const mapDispatchToProps = (dispatch) => bindActionCreators({searchCommentVisibilityFields}, dispatch); | ||
|
||
export default connect(null, mapDispatchToProps)(JiraCommentVisibilitySelector); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. | ||
// See LICENSE.txt for license information. | ||
|
||
import React from 'react'; | ||
|
||
import {ReactSelectOption} from 'types/model'; | ||
|
||
import BackendSelector, {Props as BackendSelectorProps} from '../backend_selector'; | ||
|
||
const stripHTML = (text: string) => { | ||
if (!text) { | ||
return text; | ||
} | ||
|
||
const doc = new DOMParser().parseFromString(text, 'text/html'); | ||
return doc.body.textContent || ''; | ||
}; | ||
|
||
type Props = BackendSelectorProps & { | ||
searchCommentVisibilityFields: (params: {fieldValue: string}) => ( | ||
Promise<{data: {groups: {items: {name: string}[]}}; error?: Error}> | ||
); | ||
fieldName: string; | ||
}; | ||
|
||
const JiraCommentVisibilitySelector = (props: Props) => { | ||
const {value, isMulti, instanceID, searchCommentVisibilityFields} = props; | ||
const fetchInitialSelectedValues = async (): Promise<ReactSelectOption[]> => | ||
((!value || (isMulti && !value.length)) ? [] : commentVisibilityFields('')); | ||
raghavaggarwal2308 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
const commentVisibilityFields = async (inputValue: string): Promise<ReactSelectOption[]> => { | ||
const params = { | ||
fieldValue: inputValue, | ||
instance_id: instanceID, | ||
expand: 'groups', | ||
raghavaggarwal2308 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}; | ||
return searchCommentVisibilityFields(params).then(({data}) => { | ||
return data.groups.items.map((suggestion) => ({ | ||
value: suggestion.name, | ||
label: stripHTML(suggestion.name), | ||
})); | ||
}); | ||
}; | ||
|
||
return ( | ||
<BackendSelector | ||
{...props} | ||
fetchInitialSelectedValues={fetchInitialSelectedValues} | ||
search={commentVisibilityFields} | ||
/> | ||
); | ||
}; | ||
|
||
export default JiraCommentVisibilitySelector; |
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.
I'm thinking we want to use project roles for this
I have a WIP here that implements the data fetching piece, but not the comment filtering piece master...comment-security
mattermost-plugin-jira/server/autocomplete_search.go
Lines 108 to 121 in a86928d
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.
@mickmister We are getting the user
groups
here. I don't think the project roles API returns that response