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

Display HTTP URL in /apps list #325

Merged
merged 5 commits into from Apr 27, 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions assets/i18n/active.en.json
Expand Up @@ -75,6 +75,7 @@
"command.list.submit.listed": "Listed",
"command.list.submit.status.disabled": "Installed, Disabled",
"command.list.submit.status.installed": "**Installed**",
"command.list.submit.status.unreachable": "Installed, **Unreachable**",
"command.list.submit.version": "{{.CurrentVersion}}, {{.MarketplaceVersion}} in marketplace",
"command.uninstall.description": "Uninstall an App",
"command.uninstall.hint": "[ App ID ]",
Expand Down
16 changes: 14 additions & 2 deletions server/builtin/list.go
Expand Up @@ -56,7 +56,7 @@ func (a *builtinApp) list(r *incoming.Request, creq apps.CallRequest) apps.CallR
includePluginApps := creq.BoolValue("plugin-apps")

listed := a.proxy.GetListedApps(r, "", includePluginApps)
installed := a.proxy.GetInstalledApps(r)
installed, reachable := a.proxy.GetInstalledApps(r, true)

// All of this information is non sensitive.
// Checks for the user's permissions might be needed in the future.
Expand Down Expand Up @@ -87,6 +87,13 @@ func (a *builtinApp) list(r *incoming.Request, creq apps.CallRequest) apps.CallR
ID: "command.list.submit.status.disabled",
Other: "Installed, Disabled",
})
} else {
if !reachable[app.AppID] {
status = a.conf.I18N().LocalizeDefaultMessage(loc, &i18n.Message{
ID: "command.list.submit.status.unreachable",
Other: "Installed, **Unreachable**",
})
}
}

version := string(app.Version)
Expand Down Expand Up @@ -121,8 +128,13 @@ func (a *builtinApp) list(r *incoming.Request, creq apps.CallRequest) apps.CallR
name := fmt.Sprintf("**[%s](%s)** (`%s`)",
app.DisplayName, app.HomepageURL, app.AppID)

deployType := string(app.DeployType)
if app.DeployType == apps.DeployHTTP && app.HTTP != nil {
deployType = app.HTTP.RootURL
}

txt += fmt.Sprintf("|%s|%s|%s|%s|%s|%s|%s|\n",
name, status, app.DeployType, version, account, app.GrantedLocations, app.GrantedPermissions)
name, status, deployType, version, account, app.GrantedLocations, app.GrantedPermissions)
}

listedString := a.conf.I18N().LocalizeDefaultMessage(loc, &i18n.Message{
Expand Down
8 changes: 4 additions & 4 deletions server/httpin/restapi/ux_get_ids.go
Expand Up @@ -24,8 +24,8 @@ func (a *restapi) initGetOAuthAppIDs(h *httpin.Handler) {
// Method: GET
// Input: none
// Output: []string - the list of Bot user IDs for all installed Apps.
func (a *restapi) GetBotIDs(r *incoming.Request, w http.ResponseWriter, _ *http.Request) {
apps := a.proxy.GetInstalledApps(r)
func (a *restapi) GetBotIDs(r *incoming.Request, w http.ResponseWriter, req *http.Request) {
apps, _ := a.proxy.GetInstalledApps(r, false)
ids := []string{}
for _, app := range apps {
if app.BotUserID != "" {
Expand All @@ -41,8 +41,8 @@ func (a *restapi) GetBotIDs(r *incoming.Request, w http.ResponseWriter, _ *http.
// Method: GET
// Input: none
// Output: []string - the list of OAuth ClientIDs for all installed Apps.
func (a *restapi) GetOAuthAppIDs(r *incoming.Request, w http.ResponseWriter, _ *http.Request) {
apps := a.proxy.GetInstalledApps(r)
func (a *restapi) GetOAuthAppIDs(r *incoming.Request, w http.ResponseWriter, req *http.Request) {
apps, _ := a.proxy.GetInstalledApps(r, false)
ids := []string{}
for _, app := range apps {
if app.MattermostOAuth2 != nil {
Expand Down
11 changes: 6 additions & 5 deletions server/mocks/mock_proxy/mock_proxy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 18 additions & 0 deletions server/proxy/call.go
Expand Up @@ -4,9 +4,11 @@
package proxy

import (
"context"
"io"
"net/http"
"strings"
"time"

"github.com/pkg/errors"

Expand Down Expand Up @@ -178,3 +180,19 @@ func (p *Proxy) getStatic(r *incoming.Request, app apps.App, path string) (io.Re
}
return up.GetStatic(r.Ctx(), app, path)
}

// pingApp checks if the app is accessible. Call its ping path with nothing
// expanded, ignore 404 errors coming back and consider everything else a
// "success".
func (p *Proxy) pingApp(r *incoming.Request, app apps.App) (reachable bool) {
_, err := p.callApp(r, app, apps.CallRequest{Call: apps.DefaultPing})

return err == nil || errors.Cause(err) == utils.ErrNotFound
}

func (p *Proxy) timeoutRequest(r *incoming.Request, timeout time.Duration) (*incoming.Request, context.CancelFunc) {
r = r.Clone()
ctx, cancel := context.WithTimeout(r.Ctx(), timeout)
incoming.WithCtx(ctx)(r)
return r, cancel
}
Comment on lines +193 to +198
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a method of incoming.Request? What does it have to do with Proxy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #331

14 changes: 3 additions & 11 deletions server/proxy/install.go
Expand Up @@ -21,6 +21,8 @@ import (
"github.com/mattermost/mattermost-plugin-apps/utils"
)

const pingAppTimeout = 1 * time.Second

// InstallApp installs an App.
// - cc is the Context that will be passed down to the App's OnInstall callback.
func (p *Proxy) InstallApp(r *incoming.Request, cc apps.Context, appID apps.AppID, deployType apps.DeployType, trusted bool, secret string) (*apps.App, string, error) {
Expand Down Expand Up @@ -69,17 +71,7 @@ func (p *Proxy) InstallApp(r *incoming.Request, cc apps.Context, appID apps.AppI
defer icon.Close()
}

// See if the app is inaaccessible. Call its ping path with nothing
// expanded, ignore 404 errors coming back and consider everything else a
// "success".
//
// Note that this check is often ineffective, but "the best we can do"
// before we start the diffcult-to-revert install process.
_, err = p.callApp(r, *app, apps.CallRequest{
Call: apps.DefaultPing,
Context: cc,
})
if err != nil && errors.Cause(err) != utils.ErrNotFound {
if !p.pingApp(r, *app) {
return nil, "", errors.Wrapf(err, "failed to install, %s path is not accessible", apps.DefaultPing.Path)
}

Expand Down
39 changes: 31 additions & 8 deletions server/proxy/list.go
Expand Up @@ -21,19 +21,42 @@ func (p *Proxy) GetInstalledApp(_ *incoming.Request, appID apps.AppID) (*apps.Ap
return p.store.App.Get(appID)
}

func (p *Proxy) GetInstalledApps(_ *incoming.Request) []apps.App {
installed := p.store.App.AsMap()
out := []apps.App{}
for _, app := range installed {
out = append(out, app)
func (p *Proxy) GetInstalledApps(r *incoming.Request, ping bool) (installed []apps.App, reachable map[apps.AppID]bool) {
all := p.store.App.AsMap()

// all ping requests must respond, unreachable respond with "".
reachableCh := make(chan apps.AppID)
for _, app := range all {
rr, cancel := p.timeoutRequest(r, pingAppTimeout)
Copy link
Member

Choose a reason for hiding this comment

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

Can we utilize this timeout logic when fetching bindings from all apps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, when the time for it comes? I am not sure we want to be timing out more aggressively than the default, yet, since the binding fetch should be in theory async to the user, right?

go func(a apps.App) {
var response apps.AppID
if !a.Disabled {
if p.pingApp(rr, a) {
response = a.AppID
}
}
reachableCh <- response
cancel()
}(app)
}

for _, app := range all {
installed = append(installed, app)
appID := <-reachableCh
if appID != "" {
if reachable == nil {
reachable = map[apps.AppID]bool{}
}
reachable[appID] = true
}
}

// Sort result alphabetically, by display name.
sort.SliceStable(out, func(i, j int) bool {
return strings.ToLower(out[i].DisplayName) < strings.ToLower(out[j].DisplayName)
sort.SliceStable(installed, func(i, j int) bool {
return strings.ToLower(installed[i].DisplayName) < strings.ToLower(installed[j].DisplayName)
})

return out
return installed, reachable
}

func (p *Proxy) GetListedApps(_ *incoming.Request, filter string, includePluginApps bool) []apps.ListedApp {
Expand Down
2 changes: 1 addition & 1 deletion server/proxy/service.go
Expand Up @@ -81,7 +81,7 @@ type Internal interface {
CanDeploy(apps.DeployType) (allowed, usable bool)
GetAppBindings(*incoming.Request, apps.Context, apps.App) ([]apps.Binding, error)
GetInstalledApp(*incoming.Request, apps.AppID) (*apps.App, error)
GetInstalledApps(*incoming.Request) []apps.App
GetInstalledApps(_ *incoming.Request, ping bool) (installed []apps.App, reachable map[apps.AppID]bool)
GetListedApps(_ *incoming.Request, filter string, includePluginApps bool) []apps.ListedApp
GetManifest(*incoming.Request, apps.AppID) (*apps.Manifest, error)
SynchronizeInstalledApps() error
Expand Down