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.

Sign up for GitHub

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

Jump to bottom

Added support for OAuth2 scopes #367

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Added support for OAuth2 scopes #367

wants to merge 9 commits into from

Conversation

levb
Copy link
Contributor

@levb levb commented Sep 25, 2022

Summary

Adds support for OAuth2 scopes for apps. This complements (and depends on) mattermost/mattermost#20941

Ticket Link

https://mattermost.atlassian.net/browse/MM-36818

@levb levb added Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) Docs/Needed Requires documentation labels Sep 25, 2022
@levb levb requested review from hanzei and catalintomai September 25, 2022 02:06
@codecov-commenter
Copy link

codecov-commenter commented Sep 25, 2022 •
edited
Loading

Codecov Report

Base: 21.13% // Head: 21.07% // Decreases project coverage by -0.05% ⚠️

Coverage data is based on head (341bcaa) compared to base (95d2107).
Patch coverage: 3.03% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #367      +/-   ##
==========================================
- Coverage   21.13%   21.07%   -0.06%     
==========================================
  Files          81       81              
  Lines        5215     5228      +13     
==========================================
  Hits         1102     1102              
- Misses       3999     4012      +13     
  Partials      114      114
Impacted Files Coverage Δ
apps/app.go 0.00% <0.00%> (ø)
apps/permissions.go 40.62% <0.00%> (+0.62%) ⬆️
server/proxy/install.go 0.00% <0.00%> (ø)
apps/manifest.go 94.36% <100.00%> (+0.08%) ⬆️

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@levb levb added the 2: Dev Review Requires review by a core committer label Sep 26, 2022
@levb levb marked this pull request as ready for review September 26, 2022 16:32
javaguirre
javaguirre reviewed Sep 29, 2022
View reviewed changes
Copy link
Contributor

@javaguirre javaguirre left a comment

Choose a reason for hiding this comment

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

Just a small comment to know a bit more about Go. Thank!

apps/permissions.go Show resolved Hide resolved
mickmister
mickmister reviewed Sep 30, 2022
View reviewed changes
Copy link
Contributor

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

Looks good 👍 Agreed that we should phase out Permissions for Scopes

Blocked by pending changes to mattermost/mattermost#20941 so not approving atm

examples/goapp/hello-world/app.go Outdated Show resolved Hide resolved
@levb levb requested a review from asatkinson October 10, 2022 17:19
@levb levb added the 1: PM Review Requires review by a product manager label Oct 10, 2022
@levb levb mentioned this pull request Oct 10, 2022
Merged
6 tasks
@levb levb requested a review from cwarnermm October 10, 2022 17:51
@hanzei hanzei modified the milestones: v1.2.0, v1.3.0 Nov 2, 2022
hanzei
hanzei approved these changes Feb 23, 2023
View reviewed changes
Copy link
Contributor

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

Awesome 🚀

apps/manifest.go
// Requested Access
// TODO: replace permissions with scopes?
Copy link
Contributor

Choose a reason for hiding this comment

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

In the future? Yes. But not in this PR.

go.mod
@@ -2,25 +2,27 @@ module github.com/mattermost/mattermost-plugin-apps

go 1.18

// replace github.com/mattermost/mattermost-server/v6 => ../mattermost-server
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be removed

@hanzei hanzei removed the request for review from asatkinson February 23, 2023 20:46
@hanzei hanzei removed the request for review from catalintomai February 23, 2023 20:46
@hanzei hanzei removed the 1: PM Review Requires review by a product manager label Feb 23, 2023
@mattermost-build
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mickmister mickmister mickmister left review comments

@javaguirre javaguirre javaguirre left review comments

@cwarnermm cwarnermm cwarnermm approved these changes

@hanzei hanzei hanzei approved these changes

Assignees
No one assigned
Labels
2: Dev Review Requires review by a core committer Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) Docs/Needed Requires documentation Lifecycle/1:stale
Projects
None yet
Milestone
v1.3.0
Development

Successfully merging this pull request may close these issues.
7 participants
@levb @codecov-commenter @mattermost-build @javaguirre @mickmister @hanzei @cwarnermm