Skip to content
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

Entire code base PR #5

Closed
wants to merge 8 commits into from
Closed

Entire code base PR #5

wants to merge 8 commits into from

Conversation

kosgrz
Copy link
Collaborator

@kosgrz kosgrz commented Nov 5, 2020

No description provided.

@hanzei
Copy link
Collaborator

hanzei commented Nov 9, 2020

Hey @kosgrz,

Thanks for opening this PR! @jfrerich will review it in time.

Copy link
Contributor

@jfrerich jfrerich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kosgrz, I just finished the code review and only have a couple of suggestions. These are non-blocking and could be converted to issues in the repo.

First off, thanks for all of this hard work!!

I am going to dive in and test the plugin now and verify all the features work as advertised! Awesome job!

plugin.json Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
server/plugin.go Outdated Show resolved Hide resolved
server/plugin.go Outdated Show resolved Hide resolved
server/plugin.go Outdated Show resolved Hide resolved
server/plugin.go Outdated Show resolved Hide resolved
server/testutils/http.go Outdated Show resolved Hide resolved
server/webhook/issue.go Outdated Show resolved Hide resolved
server/webhookpayload/payload.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@jfrerich
Copy link
Contributor

jfrerich commented Jan 13, 2021

Items discovered during plugin testing with dylan:

  • create issue to add autocomplete
  • when disconnect and reconnect, the token is still value and the approval is not required again
    • should the oauth token be deleted when a user disconnects?
  • /bitbucket help owner should be workspaceID

Bugs

  • committing a file to a branch gives a Bot subscription notice of branch deleted.

@DHaussermann
Copy link

DHaussermann commented Jan 14, 2021

Hi @kosgrz,

I have done a round of testing with Bitbucket cloud. Here is a list of issues and questions I have so far. Testing with Jira integration and on server instance of BitBucket is ongoing.

Sorry the list is long as this is quite a large undertaking as the plugin has alot of functionality. I have tried to organize the issues loosely by priority.

  1. Attach and create post menu option are visible when no Jira instance has been added to BitBucket Trying to use the functionality results in errors showing in the modal.
  2. Not sure Enable Private Repositories is doing anything. My repo is only available to users I have invited and regardless of this setting, my connected user can freely create subscriptions. This could have broad security implications. Should the security work the same as GitHub? It's unclear to me if the exact same design would work.
  3. To expand on @jfrerich's point above, There are numerous cases where subscription events will post to the target channel saying the target brach has been created and deleted even though the branch status has not changed on the Bitbucket side. Note: When actually deleting a branch the feature for deletes when you would expect
    3A - When making a commit (without a PR) - The subscription also shows that the branch in question was created and deleted these 2 events are confusing as the status of the branch has not changed.
    3B - PR merge does not show an event for PR being merged. It only shows that master branch was created and deleted
  4. Creating a new branch is very noisy when pull_reviews is enabled. All initial commits are shown as pushes and merges.
    Screen Shot 2021-01-14 at 3 16 04 PM
  5. todo is not working "Encountered an error getting your to do items." note: LHS data looks correct and refresh is working.
  6. pull-reviews feature will deliver an event for all comments on a PR. Is this intentional?
  7. Notification DMs for at mentions are sent twice
  8. Notifications DM for at mentions are still sent when they are off
  9. I'm not sure what the Bitbucket equivalent is "closed" status on a PR. But neither decline or merge deliver an event
  10. Creates and deletes show Mattermost username instead of corresponding Bitbucker username

Other questions...
A. Approve and Unapprove on a PR send a DM but Request Changes does not seem to. No sure if this is a large feature to add?
B. PR subscription features does not include PR description text. This is a relatively minor issue since the titles are included.

@aaronrothschild maybe these features could wait for a later release?

@DHaussermann
Copy link

@kosgrz one quick follow-up on this. After integrating a Jira instance into my Bitbucket repo, I still see the same errors when trying to use the create and attach functionality. I have tried disconnecting and recxconnecting my user but, this did not change the behavior.

{"id":"","message":"failed to create issue: Sorry, either you don't have access to the repo DHaussermann/helloworld with the user dkh-sysadmin or it is no longer available","status_code":404}

Please let me know if you have any thoughts?

@aaronrothschild
Copy link

Thanks @DHaussermann for the review!

  1. Not sure Enable Private Repositories is doing anything. My repo is only available to users I have invited and regardless of this setting, my connected user can freely create subscriptions. This could have broad security implications. Should the security work the same as GitHub? It's unclear to me if the exact same design would work.

We should lock this capability down to ensure it is being observed.

  1. To expand on @jfrerich's point above, There are numerous cases where subscription events will post to the target channel saying the target brach has been created and deleted even though the branch status has not changed on the Bitbucket side. Note: When actually deleting a branch the feature for deletes when you would expect

It's important to get this working so the user can trust the system's messages to be accurate.

3A - When making a commit (without a PR) - The subscription also shows that the branch in question was created and deleted these 2 events are confusing as the status of the branch has not changed.
3B - PR merge does not show an event for PR being merged. It only shows that master branch was created and deleted

Yes, we should fix these before release.

  1. Creating a new branch is very noisy when pull_reviews is enabled. All initial commits are shown as pushes and merges.
    Screen Shot 2021-01-14 at 3 16 04 PM

I'm 1/5, but this can be cleaned up in a future revision.

  1. todo is not working "Encountered an error getting your to do items." note: LHS data looks correct and refresh is working.

We need to fix this before shipping.

@DHaussermann For #6-10 I created issues in this repo. @kosgrz sorry, I created them before I realized I couldn't label them with a "release" version. some of these are more important than others. We wanted to have the first release of this plugin by the end of this month hopefully.

1 similar comment
@aaronrothschild
Copy link

Thanks @DHaussermann for the review!

  1. Not sure Enable Private Repositories is doing anything. My repo is only available to users I have invited and regardless of this setting, my connected user can freely create subscriptions. This could have broad security implications. Should the security work the same as GitHub? It's unclear to me if the exact same design would work.

We should lock this capability down to ensure it is being observed.

  1. To expand on @jfrerich's point above, There are numerous cases where subscription events will post to the target channel saying the target brach has been created and deleted even though the branch status has not changed on the Bitbucket side. Note: When actually deleting a branch the feature for deletes when you would expect

It's important to get this working so the user can trust the system's messages to be accurate.

3A - When making a commit (without a PR) - The subscription also shows that the branch in question was created and deleted these 2 events are confusing as the status of the branch has not changed.
3B - PR merge does not show an event for PR being merged. It only shows that master branch was created and deleted

Yes, we should fix these before release.

  1. Creating a new branch is very noisy when pull_reviews is enabled. All initial commits are shown as pushes and merges.
    Screen Shot 2021-01-14 at 3 16 04 PM

I'm 1/5, but this can be cleaned up in a future revision.

  1. todo is not working "Encountered an error getting your to do items." note: LHS data looks correct and refresh is working.

We need to fix this before shipping.

@DHaussermann For #6-10 I created issues in this repo. @kosgrz sorry, I created them before I realized I couldn't label them with a "release" version. some of these are more important than others. We wanted to have the first release of this plugin by the end of this month hopefully.

@kosgrz
Copy link
Collaborator Author

kosgrz commented Jan 20, 2021

Hi, thank you for the review and testing!

@aaronrothschild, I will start fixing these bugs from today. I think I will finish it before the end of the week 👍 .

@kosgrz
Copy link
Collaborator Author

kosgrz commented Jan 25, 2021

todo is not working "Encountered an error getting your to do items."

@DHaussermann, it's strange, because it works on my instance. Could you send me logs with this error? It will help me with debugging.

@DHaussermann
Copy link

@kosgrz I have started another round of testing with your latest commit. I notice issues 8. and 9. Seem top be resolved but I'm not sure what else has changed.

Regarding your question on 5. When I have no issues assigned to me in the paired jira instance (I do have PR to review) I see ERRO[2021-01-25T17:01:49.364203-05:00] Encountered an error getting your to do items caller="mlog/sugar.go:25" err="error occurred while searching for assignments: error occurred while fetching issues: error occurred while fetching issues: Status: 404 Not Found, Body: {\"type\": \"error\", \"error\": {\"message\": \"Repository has no issue tracker.\"}}" plugin_id=bitbucket

To take a bit of a tangent here...
This may be related to what I'm seeing for issue 1. above. I have a Bitbucket repo paired to a Jira cloud instance but the functionality that would expose the Jira issues does not seem to be working. In addition to issue 1. Even when I have issues assigned to me in my corresponding Jira cloud instance, I still see this error and a 0 count of issue in the LHS

How should this work exactly? My connected user has a Workspace with 2 Repositories and 1 of the repositories is configured to use Jira for issues. Should I just be able to create a Bitbucket issue by selecting a repository

cc @aaronrothschild Just a heads up as 1. did not show up on your must fix list. For me the functionality where BitBucket is aware of a corresponding Jira project does not seem to be working.
Also, this is actually kind of weird... As the Issue tracker for Bitbucket is Jira, we would actually have 2 plugins capable of creating Jira issues and attaching comments to them?

@jfrerich
Copy link
Contributor

@kosgrz! Thanks for the prompt PR changes!

@aaronrothschild, I'm approving, from a code review standpoint. We need a Security Review and the final QA review and we have a great bitbucket plugin

@kosgrz
Copy link
Collaborator Author

kosgrz commented Jan 26, 2021

@DHaussermann, thank you for the logs. It was very helpful. Command todo didn't work when a repo doesn't have any issue tracker enabled. I've just fixed that in my latest commit. Issues 7. and 10. are also fixed 👍 .

@DHaussermann
Copy link

Thank you for the last commit @kosgrz I will do another round of testing as soon as I get a chance.

The strange thing is. My Bitbucket repo does have a Jira instance attached so, I'm not sure why the plugin is not aware of this or if there is a config change I can make to resolve it. On a similar not. The post menu options should not be available if the pluging does not see a Jira instance associated to the Repo. Any thoughts on what's happening here?

@DHaussermann
Copy link

Hi @kosgrz I have done a round of testing on the last commit. Just to add clarity here - I have re-tested all the issues and summarized below.

1. Not able to use create and attach options even when I have a Jira instance connected
2. Subscribe is not respecting the flag to allow private repos
3. Subscriptions post on commits that the branch was created and deleted. They also post on merged that the destination branch such as master was created and deleted
4. The create event shows the commit history of the origin branch you created from. This makes the subscription very noisy.
5. The error seems fixed and the command returns data now 👍 However, when I click the list icon from the LHS something is null and it breaks the CSS so the browser goes blank. Can you let me know if you can repro this?
Screen Shot 2021-01-27 at 6 08 16 PM
6. Regarding pull-reviews feature may not be needed for release but it is a bit noisy
7. and 8. are both resolved 👍
9. Is maybe a enhancement we can add later. Certainly not needed for release.
10. Yes - thak you this has also now been resolved.

@kosgrz
Copy link
Collaborator Author

kosgrz commented Jan 29, 2021

@DHaussermann, thanks for testing! I have some questions below:

Not able to use create and attach options even when I have a Jira instance connected

Unfortunately, I could not reproduce that. I had errors, only when I wanted to create an issue in a repository that does not have a Jira instance connected. What happens exactly when you try to create an issue?

Subscribe is not respecting the flag to allow private repos

It is true that this flag does not work. Sorry, I somehow missed that feature. I can add it quickly, but I would need to disable the option to subscribe to a whole organization when the flag is enabled. If a user wants to subscribe to a specific repository, I can check whether they have access there. But, if they want to subscribe to a whole organization, I cannot. Would this solution be ok for now?

Are points 4 and 5 needed to be fixed before release?

However, when I click the list icon from the LHS something is null and it breaks the CSS so the browser goes blank. Can you let me know if you can repro this?

Fixed 👍 .

@DHaussermann
Copy link

DHaussermann commented Jan 31, 2021

Confirmed 5. is fixed. Todo and issues icon on Team sidebar both work as as expected. No breaking CSS 👍

For issue 1., we may need a couple changes. One to resolve whatever issue I'm seeing and another to remove the post menu options where there is no Jira instance associated or the connected Bitbucket user does not have access. Here are some details of what I see happen...

  • Ensue there is a Jira instance associated to the Bitbucket repo
  • Click ... on a post and select and select Create Bitbucket Issue
  • When the modal open note that the repo with Jira instance is visible in the repo
  • Click Submit
    Observed
    The request to createissue returns a 404 with"id":"","message":"failed to create issue: Sorry, either you don't have access to the repo DHaussermann/helloworld with the user dkh-sysadmin or it is no longer available","status_code":404}
    Screen Shot 2021-01-31 at 2 03 35 PM

I double checked the mapped user and when visiting Bitbucket with the same user I have access to the issue. Tried to disconnect and reconnect to see if anything would change. I also tried with the 2nd user on my Bitbucket. When I connect with the plugin, I see that the repos change (and repos without issues are listed which seems like an issue). But when I use create, I see the same problem but with the other Mattermost username.
What type of issues does the plugin attempt to create? I notice this can't be defined in the modal. 0/5 Maybe it's because the specific issue type cannot be created in my Jira project or a similar issue which makes the error a bit misleading?

Are points 4 and 5 needed to be fixed before release?

To my understand 5. is resolved but, 4. is still needed. @aaronrothschild may also have some thoughts on this.

@kosgrz
Copy link
Collaborator Author

kosgrz commented Feb 2, 2021

@DHaussermann, thank you for explaining the issue in detail. It was very helpful 👍.

For issue 1., we may need a couple changes. One to resolve whatever issue I'm seeing and another to remove the post menu options where there is no Jira instance associated or the connected Bitbucket user does not have access.

Unfortunately, according to my knowledge, this is not possible with the current Mattermost Plugin API.

I added some extra logging when creating an issue. Now, in the logs you should see something like this:
ERRO[2021-02-02T19:59:26.348181+01:00] failed to create issue: Status: 404 Not Found, Body: {"type": "error", "error": {"message": "Repository has no issue tracker."}} caller="mlog/sugar.go:25" plugin_id=bitbucket
Could you try to create an issue again and send me logs?

I see that the repos change (and repos without issues are listed which seems like an issue)

Fixed. Now you should see only repos with issue tracker enabled 👍.

To my understand 5. is resolved but, 4. is still needed.

Ok, I will try to push a fix for 5. tomorrow 👍.

@DHaussermann
Copy link

DHaussermann commented Feb 3, 2021

@kosgrz thank you for the logging. Interestingly, when I deployed this build now, the behavior changed which may shed some light on the issue. In the modal, I no longer see the repos listed to attempt ticket creation.

When the user opens the create modal everything looks fine expect that it behaves as though there are simply no repos to return see screen.
Screen Shot 2021-02-03 at 11 29 53 AM
The request to /plugins/bitbucket/api/v1/repositories gets a response of []

So then I looked at what happens earlier when the user connects....
From the UI it looks successful but... I see these 2 errors happen 6 times in the logs (all within a couple seconds of connecting):

ERRO[2021-02-03T11:48:34.233195-05:00] Error occurred while searching for repositories  caller="mlog/sugar.go:25" err="error occurred while fetching repositories: error occurred while fetching repositories: Status: 429 Too Many Requests, Body: Too Many Requests" plugin_id=bitbucket`
ERRO[2021-02-03T11:48:34.268189-05:00] error occurred while searching for repositories  caller="mlog/sugar.go:25" err="Status: 429 Too Many Requests, Body: Too Many Requests\nerror occurred while fetching repositories\nmain.(*Plugin).fetchRepositoriesWithNextPagesIfAny\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/plugin.go:391\nmain.(*Plugin).getUserRepositories\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/plugin.go:359\nmain.(*Plugin).getYourPrs\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/api.go:486\nmain.(*Plugin).extractUserMiddleWare.func1\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/api.go:142\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2042\ngithub.com/gorilla/mux.(*Router).ServeHTTP\n\t/Users/dylanhaussermann/go/pkg/mod/github.com/gorilla/[email protected]/mux.go:210\nmain.(*Plugin).ServeHTTP\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/api.go:170\ngithub.com/mattermost/mattermost-server/v5/plugin.(*hooksRPCServer).ServeHTTP\n\t/Users/dylanhaussermann/go/pkg/mod/github.com/mattermost/mattermost-server/[email protected]/plugin/client_rpc.go:365\nreflect.Value.call\n\t/usr/local/go/src/reflect/value.go:476\nreflect.Value.Call\n\t/usr/local/go/src/reflect/value.go:337\nnet/rpc.(*service).call\n\t/usr/local/go/src/net/rpc/server.go:377\nruntime.goexit\n\t/usr/local/go/src/runtime/asm_amd64.s:1374\nerror occurred while fetching repositories\nmain.(*Plugin).getUserRepositories\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/plugin.go:361\nmain.(*Plugin).getYourPrs\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/api.go:486\nmain.(*Plugin).extractUserMiddleWare.func1\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/api.go:142\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2042\ngithub.com/gorilla/mux.(*Router).ServeHTTP\n\t/Users/dylanhaussermann/go/pkg/mod/github.com/gorilla/[email protected]/mux.go:210\nmain.(*Plugin).ServeHTTP\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/api.go:170\ngithub.com/mattermost/mattermost-server/v5/plugin.(*hooksRPCServer).ServeHTTP\n\t/Users/dylanhaussermann/go/pkg/mod/github.com/mattermost/mattermost-server/[email protected]/plugin/client_rpc.go:365\nreflect.Value.call\n\t/usr/local/go/src/reflect/value.go:476\nreflect.Value.Call\n\t/usr/local/go/src/reflect/value.go:337\nnet/rpc.(*service).call\n\t/usr/local/go/src/net/rpc/server.go:377\nruntime.goexit\n\t/usr/local/go/src/runtime/asm_amd64.s:1374" plugin_id=bitbucket
ERRO[2021-02-03T11:48:34.304404-05:00] Error occurred while searching for repositories  caller="mlog/sugar.go:25" err="Status: 429 Too Many Requests, Body: Too Many Requests\nerror occurred while fetching repositories\nmain.(*Plugin).fetchRepositoriesWithNextPagesIfAny\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/plugin.go:391\nmain.(*Plugin).getUserRepositories\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/plugin.go:359\nmain.(*Plugin).getUserRepositoriesWithIssueTracker\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/plugin.go:368\nmain.(*Plugin).getYourAssignments\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/api.go:734\nmain.(*Plugin).extractUserMiddleWare.func1\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/api.go:142\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2042\ngithub.com/gorilla/mux.(*Router).ServeHTTP\n\t/Users/dylanhaussermann/go/pkg/mod/github.com/gorilla/[email protected]/mux.go:210\nmain.(*Plugin).ServeHTTP\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/api.go:170\ngithub.com/mattermost/mattermost-server/v5/plugin.(*hooksRPCServer).ServeHTTP\n\t/Users/dylanhaussermann/go/pkg/mod/github.com/mattermost/mattermost-server/[email protected]/plugin/client_rpc.go:365\nreflect.Value.call\n\t/usr/local/go/src/reflect/value.go:476\nreflect.Value.Call\n\t/usr/local/go/src/reflect/value.go:337\nnet/rpc.(*service).call\n\t/usr/local/go/src/net/rpc/server.go:377\nruntime.goexit\n\t/usr/local/go/src/runtime/asm_amd64.s:1374\nerror occurred while fetching repositories\nmain.(*Plugin).getUserRepositories\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/plugin.go:361\nmain.(*Plugin).getUserRepositoriesWithIssueTracker\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/plugin.go:368\nmain.(*Plugin).getYourAssignments\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/api.go:734\nmain.(*Plugin).extractUserMiddleWare.func1\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/api.go:142\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2042\ngithub.com/gorilla/mux.(*Router).ServeHTTP\n\t/Users/dylanhaussermann/go/pkg/mod/github.com/gorilla/[email protected]/mux.go:210\nmain.(*Plugin).ServeHTTP\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/api.go:170\ngithub.com/mattermost/mattermost-server/v5/plugin.(*hooksRPCServer).ServeHTTP\n\t/Users/dylanhaussermann/go/pkg/mod/github.com/mattermost/mattermost-server/[email protected]/plugin/client_rpc.go:365\nreflect.Value.call\n\t/usr/local/go/src/reflect/value.go:476\nreflect.Value.Call\n\t/usr/local/go/src/reflect/value.go:337\nnet/rpc.(*service).call\n\t/usr/local/go/src/net/rpc/server.go:377\nruntime.goexit\n\t/usr/local/go/src/runtime/asm_amd64.s:1374" plugin_id=bitbucket

Strangely on an earlier build I was seeing the repos in my modal. Can you take a look and see why this may be occurring?

@kosgrz
Copy link
Collaborator Author

kosgrz commented Feb 8, 2021

So then I looked at what happens earlier when the user connects....
From the UI it looks successful but... I see these 2 errors happen 6 times in the logs

Thank you for notifying me about this issue. I fixed it in the latest commit. You should now see error information when fetching repos failed.

Strangely on an earlier build I was seeing the repos in my modal. Can you take a look and see why this may be occurring?

I think you just reached the limit of requests to the Bitbucket API. You can send 1,000 requests per hour: https://support.atlassian.com/bitbucket-cloud/docs/api-request-limits/.

I've tried to fix the issue:

  1. The create event shows the commit history of the origin branch you created from. This makes the subscription very noisy.

but there is no simple way to do it. The push event contains also commits that were not pushed, but are the most recent commits of the original branch. According to the documentation, there is no option available to turn off that. More info here:
https://support.atlassian.com/bitbucket-cloud/docs/event-payloads/#Push

If someone created the reference recently, it could have commits that have been pushed to the repository, but they have not been pushed to this specific reference. If that is the case, the array includes these previously pushed commits as well. For example, if you create a new feature branch off of a master branch with a lot of commits, and then you push a single commit to it, the array in the payload contains 5 commits that are all new to the feature branch. These commits include the single commit you just made and the 4 most recent commits of the master branch.

@DHaussermann
Copy link

Hi @kosgrz Thanks for the last commit. Sorry for the late reply. I have some time to focus on this in the next few days and would like to see it wrapped up :) So, Just to outline where we are....

On 1. One of the error is resolved but, I still see ...

ERRO[2021-02-10T09:06:54.8696692-05:00] error occurred while searching for repositories  caller="mlog/sugar.go:25" err="Status: 429 Too Many Requests, Body: Too Many Requests\nerror occurred while fetching repositories\nmain.(*Plugin).fetchRepositoriesWithNextPagesIfAny\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/plugin.go:391\nmain.(*Plugin).getUserRepositories\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/plugin.go:359\nmain.(*Plugin).getYourPrs\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/api.go:486\nmain.(*Plugin).extractUserMiddleWare.func1\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/api.go:142\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2042\ngithub.com/gorilla/mux.(*Router).ServeHTTP\n\t/Users/dylanhaussermann/go/pkg/mod/github.com/gorilla/[email protected]/mux.go:210\nmain.(*Plugin).ServeHTTP\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/api.go:170\ngithub.com/mattermost/mattermost-server/v5/plugin.(*hooksRPCServer).ServeHTTP\n\t/Users/dylanhaussermann/go/pkg/mod/github.com/mattermost/mattermost-server/[email protected]/plugin/client_rpc.go:365\nreflect.Value.call\n\t/usr/local/go/src/reflect/value.go:476\nreflect.Value.Call\n\t/usr/local/go/src/reflect/value.go:337\nnet/rpc.(*service).call\n\t/usr/local/go/src/net/rpc/server.go:377\nruntime.goexit\n\t/usr/local/go/src/runtime/asm_amd64.s:1374\nerror occurred while fetching repositories\nmain.(*Plugin).getUserRepositories\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/plugin.go:361\nmain.(*Plugin).getYourPrs\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/api.go:486\nmain.(*Plugin).extractUserMiddleWare.func1\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/api.go:142\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2042\ngithub.com/gorilla/mux.(*Router).ServeHTTP\n\t/Users/dylanhaussermann/go/pkg/mod/github.com/gorilla/[email protected]/mux.go:210\nmain.(*Plugin).ServeHTTP\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/api.go:170\ngithub.com/mattermost/mattermost-server/v5/plugin.(*hooksRPCServer).ServeHTTP\n\t/Users/dylanhaussermann/go/pkg/mod/github.com/mattermost/mattermost-server/[email protected]/plugin/client_rpc.go:365\nreflect.Value.call\n\t/usr/local/go/src/reflect/value.go:476\nreflect.Value.Call\n\t/usr/local/go/src/reflect/value.go:337\nnet/rpc.(*service).call\n\t/usr/local/go/src/net/rpc/server.go:377\nruntime.goexit\n\t/usr/local/go/src/runtime/asm_amd64.s:1374" plugin_id=bitbucket

@kosgrz, I'm not sure hitting the request limit is the issue since it's quite high and resets daily. So, you would think I sometimes see my repo? Can you clarify a couple things please:
A. Could this error ☝️ be the reason I don't see my repos in the modal?
B. Is this part err="Status: 429 Too Many Requests, Body: Too Many Requests\nerror occurred while fetching repositories happening in your code or a response from the Bitbucket side? I'm not clear on what's happening here.

@aaronrothschild 0/5 if we can't get this part tested soon, how would you feel about removing issue and comment creation for the initial release in the interest of getting this wrapped up?

On 2. This was already answered above...

It is true that this flag does not work. Sorry, I somehow missed that feature. I can add it quickly, but I would need to disable the option to subscribe to a whole organization when the flag is enabled. If a user wants to subscribe to a specific repository, I can check whether they have access there. But, if they want to subscribe to a whole organization, I cannot. Would this solution be ok for now?

@aaronrothschild We have 2 options here. 0/5 can we just remove the flag from the UI and document that this release will not prevent user from making subscriptions based on Public / Private repo? The alternative provided seems quite limiting.

On 3. This seems to have been reolves by one of the last couple commits. I have all features tunred on and did some testing. I never see confusing subscription data about branches being deleted and recreated. 👍

On 4. Yes, I've done some testing and you're correct! it only shows a max of 5 commit in the branch creation payload. I misunderstood and thought it would just keep adding the entire history. That being said - This is far less serious an issue. We should address this if needed after we ship it. 👍

@kosgrz
Copy link
Collaborator Author

kosgrz commented Feb 11, 2021

Hi @DHaussermann,

A. Could this error ☝️ be the reason I don't see my repos in the modal?

Yes, this is the reason.

B. Is this part err="Status: 429 Too Many Requests, Body: Too Many Requests\nerror occurred while fetching repositories happening in your code or a response from the Bitbucket side? I'm not clear on what's happening here.

It's not happening on my machine. To how many repos you do have access to on your test account in Bitbucket? So, you cannot see your repos in the modal at all? Even when you use the plugin for the first time after a few days? It sounds very strange :/

@DHaussermann
Copy link

Hi @kosgrz,

  • I only have one repo in the Bitbucket
  • No, I never see any repo in the modal even when I have not tried in a couple days.

Strangely, today I don't see any error when connecting the user but, I still see the same behavior where there is no repo returned in the list of reps.

Seems you need to be able to see the issue before you can help further with this :)
Just a thought... Would it help if I invited you to my test instance of Bitbucket and corresponding test instance of Jira Cloud?

@kosgrz
Copy link
Collaborator Author

kosgrz commented Feb 17, 2021

@DHaussermann, sure. You can invite me. It will be much easier to investigate the issue :)

@DHaussermann
Copy link

@kosgrz sure, I would need to do it via email. Seems I can't get an email for you by looking at your GitHub handle. Can you please share an email address? You could DM it to me on the Mattermost Community server maybe?

@aaronrothschild
Copy link

Hey @kosgrz Propose:

for #1: @aaronrothschild 0/5 if we can't get this part tested soon, how would you feel about removing issue and comment creation for the initial release in the interest of getting this wrapped up?

Yes, let's removing issue and comment creation to get this out to beta while we try to fix this.

@aaronrothschild We have 2 options here. 0/5 can we just remove the flag from the UI and document that this release will not prevent user from making subscriptions based on Public / Private repo? The alternative provided seems quite limiting.

Agreed, Propose we cut this functionality.

@DHaussermann
Copy link

@aaronrothschild, Good news for the issue creation and comment creation. I was confused on what type of Issue tracker this works with on the Bitbucket side. This does seem to be working as expected when using the out-of-the-box issue tracker. @kosgrz helped me sort this out.

So we only only be missing a change that removes the flag for Provate repos (as it is non-functional)

@DHaussermann
Copy link

@kosgrz I have done some testing with your latest commit.
I still see this error.

ERRO[2021-02-25T09:20:44.05155-05:00] Error occurred while searching for repositories  caller="mlog/sugar.go:25" err="Status: 429 Too Many Requests, Body: Too Many Requests\nerror occurred while fetching repositories\nmain.(*Plugin).fetchRepositoriesWithNextPagesIfAny\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/plugin.go:392\nmain.(*Plugin).getUserRepositories\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/plugin.go:360\nmain.(*Plugin).getUserRepositoriesWithIssueTracker\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/plugin.go:369\nmain.(*Plugin).getYourAssignments\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/api.go:734\nmain.(*Plugin).extractUserMiddleWare.func1\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/api.go:142\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2042\ngithub.com/gorilla/mux.(*Router).ServeHTTP\n\t/Users/dylanhaussermann/go/pkg/mod/github.com/gorilla/[email protected]/mux.go:210\nmain.(*Plugin).ServeHTTP\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/api.go:170\ngithub.com/mattermost/mattermost-server/v5/plugin.(*hooksRPCServer).ServeHTTP\n\t/Users/dylanhaussermann/go/pkg/mod/github.com/mattermost/mattermost-server/[email protected]/plugin/client_rpc.go:365\nreflect.Value.call\n\t/usr/local/go/src/reflect/value.go:476\nreflect.Value.Call\n\t/usr/local/go/src/reflect/value.go:337\nnet/rpc.(*service).call\n\t/usr/local/go/src/net/rpc/server.go:377\nruntime.goexit\n\t/usr/local/go/src/runtime/asm_amd64.s:1374\nerror occurred while fetching repositories\nmain.(*Plugin).getUserRepositories\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/plugin.go:362\nmain.(*Plugin).getUserRepositoriesWithIssueTracker\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/plugin.go:369\nmain.(*Plugin).getYourAssignments\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/api.go:734\nmain.(*Plugin).extractUserMiddleWare.func1\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/api.go:142\nnet/http.HandlerFunc.ServeHTTP\n\t/usr/local/go/src/net/http/server.go:2042\ngithub.com/gorilla/mux.(*Router).ServeHTTP\n\t/Users/dylanhaussermann/go/pkg/mod/github.com/gorilla/[email protected]/mux.go:210\nmain.(*Plugin).ServeHTTP\n\t/Users/dylanhaussermann/go/src/github.com/mattermost-plugin-bitbucket/server/api.go:170\ngithub.com/mattermost/mattermost-server/v5/plugin.(*hooksRPCServer).ServeHTTP\n\t/Users/dylanhaussermann/go/pkg/mod/github.com/mattermost/mattermost-server/[email protected]/plugin/client_rpc.go:365\nreflect.Value.call\n\t/usr/local/go/src/reflect/value.go:476\nreflect.Value.Call\n\t/usr/local/go/src/reflect/value.go:337\nnet/rpc.(*service).call\n\t/usr/local/go/src/net/rpc/server.go:377\nruntime.goexit\n\t/usr/local/go/src/runtime/asm_amd64.s:1374" plugin_id=bitbucket

Though I don't see it anymore when connecting the user. I see it a few seconds after. I'm not sure if there are any functional implications.

Not sure if this could be related... while testing this I did notice something else, even when I have 2 repositories with the non-jira BitBucket issue tracker enabled, I only see one repo. Is this something you can repro? I added you to a 2nd repo in my workspace called token it would be helpful if you could check and see if you can see it along with hello-world. I tried disconnecting and reconnecting. I also tried making the issue tracker public but this has no affect.

Lately, we still have the issue that the post menu items will show even when if there is no issue tracker enabled in the available repos. This is not a solvable issue correct?
If so, can we at least add some text when no repos are returned that says something to the affect of Please ensure the Bitbucket issue tracker is enabled By selecting "Repository settings" and selecting "Issue tracker" I feel like this really would have help me above ☝️ when I was trying to make it work with a Jira instance :)
cc @aaronrothschild thoughts on this being the behavior at least for V1?

@aaronrothschild
Copy link

@kosgrz @DHaussermann could we do something similar to jira, where we allow admins to disable the functionality of Attach/Create from the dropdown?

image

Copy link

@srkgupta srkgupta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @kosgrz

Following additional security issues have been identified while reviewing the plugin. Can you please fix the same. Please let me know if you need any additional information.

1) The Bitbucket refresh tokens are not encrypted and stored in DB

Severity: High

Steps:

  • When a user connects to a bitbucket account, the connection refresh_token is stored as plain text in DB. This value is not encrypted and stored.
  • As a result, anyone can obtain a new access_token and the point of having an encrypted access_token in DB will be lost.

curl -X POST -u "key:secret" https://bitbucket.org/site/oauth2/access_token -d grant_type=refresh_token -d refresh_token=refresh_token

  • Since a Bitbucket comprises of a user's personal account also, anyone who possesses this info from DB, can access user's other private repositories not supposed to be shared.

2) Bitbucket webhook secret isn't used and hence the incoming bitbucket webhook is vulnerable and data can be tampered.

Severity: Moderate

Steps:

  • Currently the incoming webhook URL is static and looks like https://<INSTANCE_URL>/plugins/bitbucket/webhook
  • Since the URL is static, anyone can send a POST request to this URL mimicking the Bitbucket and post contents with phishing URL etc
    Suggestions: Make the incoming webhook URL dynamic and allow user to regenerate this URL, for example:
    https://<INSTANCE_URL>/plugins/bitbucket/webhook/<SOME_RANDOM_UNIQUE_UUID>

3) A note should be added on the Plugin configuration page and inform Administrators to whitelist valid IP addresses of Bitbucket:

https://support.atlassian.com/bitbucket-cloud/docs/manage-webhooks/
https://support.atlassian.com/bitbucket-cloud/docs/what-are-the-bitbucket-cloud-ip-addresses-i-should-use-to-configure-my-corporate-firewall/

Severity: Low

4) When the Bitbucket plugin is removed from System Console, all the previous values like OAuth Client, Secret, Encryption Key etc should also be removed

Severity: Moderate

Steps:

  • Login as a sysadmin user and install the bitbucket plugin
  • Visit the bitbucket configuration page and enter all the details and save.
  • Now remove the bitbucket plugin.
  • Install the plugin again and visit the bitbucket configuration page.
  • Notice that the values are still being displayed even after being removed.

5) The Bitbucket OAuth Client Secret is displayed as plain text in the System Console. When the config is saved and page is reloaded, this should not be exposed. We should mask it.

Severity: Low

Steps:

  • Login as a sysadmin user and install the bitbucket plugin.
  • Visit the bitbucket configuration page and enter Oauth client ID and secret and save.
  • Reload the page and notice that the secret is still displayed in plain text.
  • Like other config pages, i.e say OAuth config page on System console, the secret key should be truncated and displayed as ******. It should not be returned as plain text in config API

6) Regular User is allowed to view and manage list of subscriptions belonging to a different user.

Severity: Low

Steps:

  • Login as User1 on Mattermost.
  • Connect to Bitbucket as User1.
  • Subscribe to few private repositories using the command /bitbucket subscribe user1/repo1
  • On another browser, login as User2 with low priviliges and visit the same channel.
  • Check subscriptions /bitbucket subscribe list and notice that it still displays repo1 which is a private repo of user1.
  • Unsubscribe using the command /bitbucket unsubscribe user1/repo1 and notice that user is allowed to change the subscription belonging to a different user.

Expected: Subscriptions should be user based. Only the owner of the subscription should be allowed to view or unsubscribe.

@hanzei
Copy link
Collaborator

hanzei commented Apr 13, 2021

@srkgupta

  1. Would we store the key also in the DB?
  2. For context, that is the behavior of all plugins right now. AFAIK keeping the data is the desired behavior.

@srkgupta
Copy link

srkgupta commented Apr 13, 2021

@hanzei regarding issue 1, yes currently the key is also stored in the DB. We may have to find a better way to manage this and store it in a different place other than DB (also applicable for all other plugins which behave the same way)

Regarding issue 4, okay got it. In that case, we can ignore this for now. Will have a separate discussion on how can this be addressed in future. We may have to display a message in UI informing the same to sysadmin when they remove a plugin.

@jfrerich
Copy link
Contributor

@DHaussermann, @srkgupta, I've created tickets for all remaining Security items and would like to merge this PR and address the issues in individual PRs. I'll be working on the fixes

@jfrerich
Copy link
Contributor

Note that I rebased this PR to merge onto master.

@jfrerich
Copy link
Contributor

jfrerich commented Jun 1, 2021

Merging and will address the remaining issues in individual PRs

@jfrerich jfrerich enabled auto-merge (squash) June 1, 2021 18:56
@cpanato
Copy link

cpanato commented Oct 20, 2021

/check-cla

@hanzei hanzei closed this Oct 21, 2021
auto-merge was automatically disabled October 21, 2021 08:29

Pull request was closed

@hanzei hanzei deleted the compare branch October 21, 2021 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants