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

For non-cloud plugin settings, set hosting property to on-prem #269

Merged
merged 19 commits into from
Jul 26, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion build/manifest/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ func findManifest() (*model.Manifest, error) {
// we don't want to accidentally clobber anything we won't preserve.
var manifest model.Manifest
decoder := json.NewDecoder(manifestFile)
decoder.DisallowUnknownFields()
Copy link
Contributor Author

@mickmister mickmister Jul 21, 2022

Choose a reason for hiding this comment

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

This is commented out to support compiling the plugin with an "unknown" property on the plugin settings. Updating the server version to otherwise support this is not trivial because:

  • The newer mattermost-server commit to import requires Go 1.18 to compile
  • circleci then needs to be updated to use Go 1.18
  • Updating circleci's Go image is blocked by Move to cimg/go circleci-orbs#29
  • I tried to continue the work on that PR, but getting Go 1.18 working on there was non-trivial
  • The golang:1.18-node image also updates the node version, making it so it is a newer version than this plugin currently compiles with

The reason for this DisallowUnknownFields call is to prevent the manifest field structure from being out of sync, which is something that's occurring in this PR. We are purposely "breaking the rules here", while the circleci update is still underway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @hanzei for discussion on this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Details on the error I was getting in CI mattermost/circleci-orbs#29 (comment)

// decoder.DisallowUnknownFields() // Commented out until circleci image is updated: https://github.com/mattermost/mattermost-plugin-zoom/pull/269#discussion_r927075701
if err = decoder.Decode(&manifest); err != nil {
return nil, errors.Wrap(err, "failed to parse manifest")
}
Expand Down
9 changes: 6 additions & 3 deletions plugin.json
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@
"type": "bool",
"help_text": "When true, OAuth will be used as the authentication means with Zoom. \n When false, JWT will be used as the authentication means with Zoom. \n If you're currently using a JWT Zoom application and switch to OAuth, all users will need to connect their Zoom account using OAuth the next time they try to start a meeting. [More information](https://mattermost.gitbook.io/plugin-zoom/installation/zoom-configuration).",
"placeholder": "",
"default": false
"default": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just double checking this default value change is intended.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@larkox Yes it's intended, since the OAuth feature is now our "main" feature offering, and we want to discourage using the JWT method.

"hosting": "on-prem"
},
{
"key": "AccountLevelApp",
Expand Down Expand Up @@ -88,15 +89,17 @@
"type": "text",
"help_text": "The API key generated by Zoom, used to create meetings and pull user data.",
"placeholder": "",
"default": null
"default": null,
"hosting": "on-prem"
},
{
"key": "APISecret",
"display_name": "API Secret:",
"type": "text",
"help_text": "The API secret generated by Zoom for your API key.",
"placeholder": "",
"default": null
"default": null,
"hosting": "on-prem"
},
{
"key": "WebhookSecret",
Expand Down
6 changes: 3 additions & 3 deletions server/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ func (p *Plugin) executeCommand(c *plugin.Context, args *model.CommandArgs) (str
}

func (p *Plugin) canConnect(user *model.User) bool {
return p.configuration.EnableOAuth && // we are not on JWT
return p.OAuthEnabled() && // we are not on JWT
(!p.configuration.AccountLevelApp || // we are on user managed app
user.IsSystemAdmin()) // admins can connect Account level apps
}
Expand Down Expand Up @@ -232,7 +232,7 @@ func (p *Plugin) runHelpCommand(user *model.User) (string, error) {
// getAutocompleteData retrieves auto-complete data for the "/zoom" command
func (p *Plugin) getAutocompleteData() *model.AutocompleteData {
available := "start, help"
if p.configuration.EnableOAuth && !p.configuration.AccountLevelApp {
if p.OAuthEnabled() && !p.configuration.AccountLevelApp {
available = "start, connect, disconnect, help"
}
zoom := model.NewAutocompleteData("zoom", "[command]", fmt.Sprintf("Available commands: %s", available))
Expand All @@ -241,7 +241,7 @@ func (p *Plugin) getAutocompleteData() *model.AutocompleteData {
zoom.AddCommand(start)

// no point in showing the 'disconnect' option if OAuth is not enabled
if p.configuration.EnableOAuth && !p.configuration.AccountLevelApp {
if p.OAuthEnabled() && !p.configuration.AccountLevelApp {
connect := model.NewAutocompleteData("connect", "", "Connect to Zoom")
disconnect := model.NewAutocompleteData("disconnect", "", "Disonnects from Zoom")
zoom.AddCommand(connect)
Expand Down
8 changes: 3 additions & 5 deletions server/configuration.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,17 +51,17 @@ func (c *configuration) Clone() *configuration {
}

// IsValid checks if all needed fields are set.
func (c *configuration) IsValid() error {
func (c *configuration) IsValid(isCloud bool) error {
switch {
case !c.EnableOAuth:
case !c.EnableOAuth && !isCloud: // JWT for on-prem
switch {
case len(c.APIKey) == 0:
return errors.New("please configure APIKey")

case len(c.APISecret) == 0:
return errors.New("please configure APISecret")
}
case c.EnableOAuth:
case c.EnableOAuth || isCloud: // OAuth for either platform
switch {
case len(c.OAuthClientSecret) == 0:
return errors.New("please configure OAuthClientSecret")
Expand All @@ -72,8 +72,6 @@ func (c *configuration) IsValid() error {
case len(c.EncryptionKey) == 0:
return errors.New("please generate EncryptionKey from Zoom plugin settings")
}
default:
return errors.New("please select either OAuth or Password based authentication")
}

if len(c.WebhookSecret) == 0 {
Expand Down
2 changes: 1 addition & 1 deletion server/http.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ type startMeetingRequest struct {

func (p *Plugin) ServeHTTP(c *plugin.Context, w http.ResponseWriter, r *http.Request) {
config := p.getConfiguration()
if err := config.IsValid(); err != nil {
if err := config.IsValid(p.isCloudLicense()); err != nil {
http.Error(w, "This plugin is not configured.", http.StatusNotImplemented)
return
}
Expand Down
18 changes: 16 additions & 2 deletions server/plugin.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ func (p *Plugin) OnActivate() error {
p.client = pluginapi.NewClient(p.API, p.Driver)

config := p.getConfiguration()
if err := config.IsValid(); err != nil {
if err := config.IsValid(p.isCloudLicense()); err != nil {
return err
}

Expand Down Expand Up @@ -138,7 +138,7 @@ func (p *Plugin) getActiveClient(user *model.User) (zoom.Client, string, error)
config := p.getConfiguration()

// JWT
if !config.EnableOAuth {
if !p.OAuthEnabled() {
return p.jwtClient, "", nil
}

Expand Down Expand Up @@ -240,3 +240,17 @@ func (p *Plugin) SetZoomSuperUserToken(token *oauth2.Token) error {
}
return nil
}

func (p *Plugin) isCloudLicense() bool {
license := p.API.GetLicense()
return license != nil && *license.Features.Cloud
}

func (p *Plugin) OAuthEnabled() bool {
config := p.getConfiguration()
if config.EnableOAuth {
return true
}

return p.isCloudLicense()
}