Fixed issue: Comment notification not working after implementing OAuth2 and Added PKCE code for OAuth2 #953
Conversation
* Migrate Docs from GitBook * Update readme.md * Update readme.md Co-authored-by: Katie Wiersgalla <39744472+wiersgallak@users.noreply.github.com> * Update readme.md Co-authored-by: Katie Wiersgalla <39744472+wiersgallak@users.noreply.github.com> * Update readme.md Co-authored-by: Katie Wiersgalla <39744472+wiersgallak@users.noreply.github.com> --------- Co-authored-by: Katie Wiersgalla <39744472+wiersgallak@users.noreply.github.com>
…lose after user is connected
[MI-3153] Fixed issue: comment webhook notification not working and added PKCE for authorization
[MI-3159] Implement OAuth for jira cloud instances
|
Hello @raghavaggarwal2308, Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here. |
|
@raghavaggarwal2308 FWIW - I'll try and do a test from this PR branch and see if the subscription bug is solved. Maybe @hanzei can help you push this back up into #949 shortly. |
|
@raghavaggarwal2308 I'm a bit confused. I compiled this branch and upgraded the same server where I found the issue and I see no change in behavior. Updating several fields in the ticket is delivering the events but comment events do not get delivered. I double checked my commit The error is now different. I don't know what token we're looking for here. I also tried on a fresh server with a new installation and comments still don't work. Please let me know if you can repro this. |
|
@DHaussermann We tried to reproduce this on MM server version v7.8.2 but we were unable to do so. Can you please share the server version you are using? Also, it would be helpful if you can share your Mattermost and Jira test cloud server credentials so that we can adequately debug the issue here. |
|
I will DM you a set of credentials now. I can also include an invite to a server where I see the issue. |
|
@raghavaggarwal2308 after re-testing this. I do see comment events being delivered after the last commit. The issue I see now is that unlike other events, comment events don't work with security levels for me. (On a project without security levels, this would not be something you can find as the case is not applicable) Scenario A - The Jira issue has no security level set
Scenario B - The Jira issue has a security level set
|
|
@DHaussermann @mickmister We explored the above issue. The reason for that is the webhook body for comment notification contains the issue details now, but they are incomplete. The custom/additional fields present on the issue are not listed in the issue details. So, when we are unable to match the filters for comment notifications when the "Security level" filter is set. To resolve this issue we need to make an extra API call to fetch the issue details separately for comment notification, but we need either the user's JiraAccountID or the MattermostUserID to get the Jira client and make an API call. The problem is that the webhook body contains only the account ID of the commenter, but that user may or may not be connected to Mattermost, so if we use that ID we won't be able to get issue details in case the user is not connected and the notification will not be posted. Can you suggest us a way to get the connected user's ID? |
|
@jasonblais heads-up Michael is likely the person with the most knowledge to provide guidance here but he's still away. @hanzei may have some context but has not work on this functionality as much. |
|
I don't have context knowledge about the webhook handling in JIRA to share. |
@raghavaggarwal2308 This may be a limitation we need to accept. Can you please provide what information is missing from the payload, and what is present? Is there any way to have a superuser token / service account for Jira Cloud? If not, we may need to be clear that "any subscription post made from a comment will need to be made by a connected user." |
Issue details in "comment created" webhook body: Issue details in "issue created" webhook body: |
|
@raghavaggarwal2308 In that case, I think we'll need to accept the fact that any subscription post made from a comment will need to be made by a connected user. DM mentions should still work in this case though right? |
@raghavaggarwal2308 Sorry I don't know the answer to this. I would say if Atlassian recommends this, and it all works when making the change, then we should just change all of them. |
…for comment webhook notification (#58) * [MI-3282] Added logic to expand issue in comment webhook notification * [MI-3282] Fix lint errors * [MI-3315] Fixed issue: comment DM notification not working due to invalid API call * [MI-3282] Review fixes * [MI-3282] Review fix
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## MM-51310_oauth2_authentication #953 +/- ##
==================================================================
- Coverage 31.16% 29.66% -1.51%
==================================================================
Files 50 52 +2
Lines 7409 7777 +368
==================================================================
- Hits 2309 2307 -2
- Misses 4909 5278 +369
- Partials 191 192 +1
☔ View full report in Codecov by Sentry. |
|
@mickmister Fixed both issues with the comment webhook notification:
|
There was a problem hiding this comment.
LGTM, though I want to make sure the comment fetching works for Jira Server on this line
mattermost-plugin-jira/server/webhook.go
Line 143 in 0186841
server/webhook.go
Outdated
| @@ -140,7 +140,7 @@ func (wh *webhook) PostNotifications(p *Plugin, instanceID types.ID) ([]*model.P | |||
|
|
|||
| isCommentEvent := wh.Events().Intersection(commentEvents).Len() > 0 | |||
| if isCommentEvent { | |||
| err = client.RESTGet(notification.commentSelf, nil, &struct{}{}) | |||
| err = client.RESTGet(fmt.Sprintf("/2/issue/%s/comment/%s", wh.Issue.ID, wh.Comment.ID), nil, &struct{}{}) | |||
There was a problem hiding this comment.
Wondering if this works for all 3 Jira instance types. This is my main remaining concern here
There was a problem hiding this comment.
@mickmister I have tested this code on Jira cloud-oauth and server instances but since the Jira JWT is not working for us, we were not able to test it there.
There was a problem hiding this comment.
Okay, I'm hesitant to introduce this change for Jira server. We need to make sure it works on Jira server v7 before release. If it works for Jira 9, it likely works for Jira 7, but we should avoid a regression on Jira server for something related to Jira cloud. Maybe we can wrap this in a conditional block "if cloud" etc. Please let me know your thoughts on risks of both options if you can
There was a problem hiding this comment.
@mickmister Added the conditional block
| func (p *Plugin) getIssueDataForCloudWebhook(instance Instance, issueKey string) (*jira.Issue, error) { | ||
| ci, ok := instance.(*cloudInstance) | ||
| if !ok { | ||
| return nil, errors.Errorf("must be a Jira cloud instance, is %s", instance.Common().Type) | ||
| } | ||
|
|
||
| jiraClient, err := ci.getClientForBot() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| issue, resp, err := jiraClient.Issue.Get(issueKey, nil) | ||
| if err != nil { | ||
| switch { | ||
| case resp == nil: | ||
| return nil, errors.WithMessage(userFriendlyJiraError(nil, err), "request to Jira failed") | ||
| case resp.StatusCode == http.StatusNotFound || resp.StatusCode == http.StatusUnauthorized: | ||
| return nil, errors.New(`we couldn't find the issue key, or the cloud "bot" client does not have the appropriate permissions to view the issue`) | ||
| } | ||
| } | ||
|
|
||
| return issue, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Did this function exist somewhere else earlier? Was it deleted at some point? I'm asking because there is no red diff related to this function in this commit
There was a problem hiding this comment.
@mickmister Yes, it was removed earlier.
…ue) (#60) * [MI-3331] Review fixes on PR mattermost#953 (Comment notification issue) * [MI-3331] Added separate condition to check if the instance is a cloud instance or not
|
@mickmister FIxed the review comments mentioned by you |
mickmister
added
3: QA Review
|
@DHaussermann Are you testing this PR? If not, can Brightscout team QA test it? |
|
@raghavaggarwal2308: @DHaussermann is currently on vacation so he won't be able to respond Also there's an unresolved discussion here #953 (comment). I'm thinking we should keep it the way it was for Jira Server, to avoid having to test with Jira Server v7. However, Jira Server v9 should be regression tested before OAuth is released. |
@mickmister Sure, we will be making this today after that as @DHaussermann is not available, so should brightscout QA do the QA review if this is urgent or should we wait for Dylan to come back ? |
|
@Kshitij-Katiyar Brightscout can do QA on this PR. FYI Dylan will be back on Monday |
… issue) 1. Added conditional for separation out the logic for jira cloud and server
… issue) (#62) 1. Added conditional for separation out the logic for jira cloud and server
mickmister
merged commit 6df75bc
into
mattermost:MM-51310_oauth2_authentication
|
@DHaussermann Note that I've merged this into the main OAuth PR #949 |
|
Tested and approved, LGTM
|
* Initial commit to support OAuth2 authentication * Implemented modal to configure OAuth2 credentials * Made the field names consistent * Removed install_cloud_oauth.md template * Added telemetry tracking for oauth2 setup flow * Fixed typo * Implemented few review comments * Reverted webapp changes * Reverted package-lock.json file * Fixed depreciation * Using refresh token to get another access token * Implemented review comments & QA findings * Implemented few additional review comments * fixed enterprise check while updating instances * Renamed function name for easy understanding * Fixed failing instances test * fix merge issue * fix merge issue 2 * check for cloud oauth instance for comment webhooks * Fixed issue: Comment notification not working after implementing OAuth2 and Added PKCE code for OAuth2 (#953) * Migrate Docs from GitBook (#948) * Migrate Docs from GitBook * Update readme.md * Update readme.md Co-authored-by: Katie Wiersgalla <39744472+wiersgallak@users.noreply.github.com> * Update readme.md Co-authored-by: Katie Wiersgalla <39744472+wiersgallak@users.noreply.github.com> * Update readme.md Co-authored-by: Katie Wiersgalla <39744472+wiersgallak@users.noreply.github.com> --------- Co-authored-by: Katie Wiersgalla <39744472+wiersgallak@users.noreply.github.com> * [MI-3153] Fixed issue: comment webhook notification not working * [MI-3153] Added PKCE along with authorization code for getting the access token * [MI-3153] Review fixes * [MI-3159] Review fixes of comments given by Kshitij * [MI-3159] Review fixes given by Ayush * [MI-3159] Removed the code which was forcing the template window to close after user is connected * [MI-3153] Review fixes * [MI-3159] Review fixes * [MI-3159] Review fix * [MI-3282] Added logic to expand issue and issue with DM notification for comment webhook notification (#58) * [MI-3282] Added logic to expand issue in comment webhook notification * [MI-3282] Fix lint errors * [MI-3315] Fixed issue: comment DM notification not working due to invalid API call * [MI-3282] Review fixes * [MI-3282] Review fix * [MI-3331] Review fixes on PR #953 (Comment notification issue) (#60) * [MI-3331] Review fixes on PR #953 (Comment notification issue) * [MI-3331] Added separate condition to check if the instance is a cloud instance or not * [MI-3335] Review fixes on Jira PR #953(Comment notification issue) (#62) 1. Added conditional for separation out the logic for jira cloud and server --------- Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> Co-authored-by: Katie Wiersgalla <39744472+wiersgallak@users.noreply.github.com> --------- Co-authored-by: Rohitesh Gupta <1429138+srkgupta@users.noreply.github.com> Co-authored-by: Mattermost Build <build@mattermost.com> Co-authored-by: Raghav Aggarwal <raghav.aggarwal@brightscout.com> Co-authored-by: Carrie Warner (Mattermost) <74422101+cwarnermm@users.noreply.github.com> Co-authored-by: Katie Wiersgalla <39744472+wiersgallak@users.noreply.github.com>
Summary
Issue/Enhancement Links:
Fixes PR
#949 (Implement OAuth2 Authentication (reopened))