From 0577a5aaa2755663780ae6de56011f78682098ed Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Mon, 16 Oct 2023 11:27:43 -0300 Subject: [PATCH] Fix OpenApi vetting (#23974) * fix openApi vetting The underlying mattermost-govet tool effectively hasn't been called for some time, as we weren't checking out and building the spec files to pass. Now that hte API is in the monorepo, build it locally and check against it. Unfortunately, our API documentation isn't up-to-date, and this PR isn't fixing that. For now, add a discrete `make vet-api` and workflow that won't block the build until the API documentation is back in sync and can be merged into the existing `make vet` directive. * mattermost-govet: use upstream@new * fix missing /api/v4 prefix for commands autocomplete suggestion * document /api/v4/ldap/users/{user_id}/group_sync_memberships * document /api/v4/groups/{group_id}/restore * fix /files/{file_id}/public actually at root * document /api/v4/users/invalid_emails * fix SetThreadUnreadByPostId * Revert "fix SetThreadUnreadByPostId" This reverts commit b16bcc80442b4391c3e7472ce47b9da65809697b. * Revert "Revert "fix SetThreadUnreadByPostId"" This reverts commit 8bda05dc8aa8c57b20e2fd525431b43e21474859. * workaround undocumented API endpoints * remove unnecessary whiteline * ignore go tool output --------- Co-authored-by: Mattermost Build --- .github/workflows/server-ci-template.yml | 22 +++++++- api/v4/source/users.yaml | 6 +-- server/Makefile | 16 +++--- server/channels/api4/cloud.go | 2 +- server/channels/api4/hosted_customer.go | 2 +- server/scripts/vet-api-check.sh | 69 ++++++++++++++++++++++++ 6 files changed, 104 insertions(+), 13 deletions(-) create mode 100755 server/scripts/vet-api-check.sh diff --git a/.github/workflows/server-ci-template.yml b/.github/workflows/server-ci-template.yml index e15d565aff..0a2d05b0a7 100644 --- a/.github/workflows/server-ci-template.yml +++ b/.github/workflows/server-ci-template.yml @@ -118,7 +118,27 @@ jobs: - name: Run plugin-checker run: make plugin-checker - name: Run mattermost-vet - run: make vet BUILD_NUMBER='${GITHUB_HEAD_REF}' MM_NO_ENTERPRISE_LINT=true MM_VET_OPENSPEC_PATH='${PWD}/../../mattermost-api-reference/v4/html/static/mattermost-openapi-v4.yaml' + run: make vet BUILD_NUMBER='${GITHUB_HEAD_REF}' MM_NO_ENTERPRISE_LINT=true + check-mattermost-vet-api: + name: Vet API + runs-on: ubuntu-22.04 + defaults: + run: + working-directory: server + steps: + - name: Checkout mattermost project + uses: actions/checkout@ac593985615ec2ede58e132d2e21d2b1cbd6127c # v3.3.0 + - name: Setup Go + uses: actions/setup-go@4d34df0c2316fe8122ab82dc22947d607c0c91f9 # v4.0.0 + with: + go-version: ${{ env.go-version }} + cache-dependency-path: | + server/go.sum + server/public/go.sum + - name: Run setup-go-work + run: make setup-go-work + - name: Run mattermost-vet-api + run: make vet-api check-migrations: name: Check migration files runs-on: ubuntu-22.04 diff --git a/api/v4/source/users.yaml b/api/v4/source/users.yaml index dfaf7ffd38..44aaf4b72b 100644 --- a/api/v4/source/users.yaml +++ b/api/v4/source/users.yaml @@ -2937,7 +2937,7 @@ "404": $ref: "#/components/responses/NotFound" "/api/v4/users/{user_id}/teams/{team_id}/threads/{thread_id}/set_unread/{post_id}": - put: + post: tags: - threads summary: Mark a thread that user is following as unread based on a post id @@ -3213,12 +3213,12 @@ - users summary: Get users with invalid emails description: > - Get users whose emails are considered invalid. + Get users whose emails are considered invalid. It is an error to invoke this API if your team settings enable an open server. ##### Permissions - + Requires `sysconsole_read_user_management_users`. operationId: GetUsersWithInvalidEmails diff --git a/server/Makefile b/server/Makefile index f6d4c163b5..d48a0ade49 100644 --- a/server/Makefile +++ b/server/Makefile @@ -768,19 +768,21 @@ endif vet: ## Run mattermost go vet specific checks $(GO) install github.com/mattermost/mattermost-govet/v2@new - @VET_CMD="-license -structuredLogging -inconsistentReceiverName -inconsistentReceiverName.ignore=session_serial_gen.go,team_member_serial_gen.go,user_serial_gen.go -emptyStrCmp -tFatal -configtelemetry -errorAssertions"; \ - if ! [ -z "${MM_VET_OPENSPEC_PATH}" ] && [ -f "${MM_VET_OPENSPEC_PATH}" ]; then \ - VET_CMD="$$VET_CMD -openApiSync -openApiSync.spec=$$MM_VET_OPENSPEC_PATH"; \ - else \ - echo "MM_VET_OPENSPEC_PATH not set or spec yaml path in it is incorrect. Skipping API check"; \ - fi; \ - $(GO) vet -vettool=$(GOBIN)/mattermost-govet $$VET_CMD ./... + $(GO) vet -vettool=$(GOBIN)/mattermost-govet -license -structuredLogging -inconsistentReceiverName -inconsistentReceiverName.ignore=session_serial_gen.go,team_member_serial_gen.go,user_serial_gen.go -emptyStrCmp -tFatal -configtelemetry -errorAssertions ./... ifeq ($(BUILD_ENTERPRISE_READY),true) ifneq ($(MM_NO_ENTERPRISE_LINT),true) $(GO) vet -vettool=$(GOBIN)/mattermost-govet -enterpriseLicense -structuredLogging -tFatal $(BUILD_ENTERPRISE_DIR)/... endif endif +vet-api: export GO := $(GO) +vet-api: export GOBIN := $(GOBIN) +vet-api: export ROOT := $(ROOT) +vet-api: ## Run mattermost go vet to verify api4 documentation, currently not passing + $(GO) install github.com/mattermost/mattermost-govet/v2@new + make -C ../api build + ./scripts/vet-api-check.sh + gen-serialized: export LICENSE_HEADER:=$(LICENSE_HEADER) gen-serialized: ## Generates serialization methods for hot structs # This tool only works at a file level, not at a package level. diff --git a/server/channels/api4/cloud.go b/server/channels/api4/cloud.go index 5d80b4bc8b..ef67e38136 100644 --- a/server/channels/api4/cloud.go +++ b/server/channels/api4/cloud.go @@ -57,7 +57,7 @@ func (api *API) InitCloud() { // GET /api/v4/cloud/cws-health-check api.BaseRoutes.Cloud.Handle("/check-cws-connection", api.APIHandler(handleCheckCWSConnection)).Methods("GET") - api.BaseRoutes.Cloud.Handle("/delete-workspace", api.APISessionRequired(selfServeDeleteWorkspace)).Methods(http.MethodDelete) + api.BaseRoutes.Cloud.Handle("/delete-workspace", api.APISessionRequired(selfServeDeleteWorkspace)).Methods("DELETE") } func ensureCloudInterface(c *Context, where string) bool { diff --git a/server/channels/api4/hosted_customer.go b/server/channels/api4/hosted_customer.go index 1c962a663f..ba73dc0e25 100644 --- a/server/channels/api4/hosted_customer.go +++ b/server/channels/api4/hosted_customer.go @@ -38,7 +38,7 @@ func (api *API) InitHostedCustomer() { // GET /api/v4/hosted_customer/invoices/{invoice_id:in_[A-Za-z0-9]+}/pdf api.BaseRoutes.HostedCustomer.Handle("/invoices/{invoice_id:in_[A-Za-z0-9]+}/pdf", api.APISessionRequired(selfHostedInvoicePDF)).Methods("GET") - api.BaseRoutes.HostedCustomer.Handle("/subscribe-newsletter", api.APIHandler(handleSubscribeToNewsletter)).Methods(http.MethodPost) + api.BaseRoutes.HostedCustomer.Handle("/subscribe-newsletter", api.APIHandler(handleSubscribeToNewsletter)).Methods("POST") } func ensureSelfHostedAdmin(c *Context, where string) { diff --git a/server/scripts/vet-api-check.sh b/server/scripts/vet-api-check.sh new file mode 100755 index 0000000000..91823c6461 --- /dev/null +++ b/server/scripts/vet-api-check.sh @@ -0,0 +1,69 @@ +#!/bin/bash +set -euo pipefail +IFS=$'\n\t' + +# Our API vet checks haven't been running for a long time, and there are lots of undocumented APIs. +# To stem the introduction of new, undocumented APIs while we find time to document the old ones, +# filter out all the "known issues" to support the automated CI check. + +API_YAML=$ROOT../api/v4/html/static/mattermost-openapi-v4.yaml +OUTPUT=$($GO vet -vettool=$GOBIN/mattermost-govet -openApiSync -openApiSync.spec=$API_YAML ./... 2>&1 || true) + +echo "All output, some ignored" +echo "========================" +echo "$OUTPUT" + +OUTPUT_EXCLUDING_IGNORED=$(echo "$OUTPUT" | grep -Fv \ + -e 'go: downloading' \ + -e 'github.com/mattermost/mattermost/server/v8/channels/api4' \ + -e 'Cannot find /api/v4/channels/members/{user_id}/mark_read method: POST in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/channels/members/{user_id}/mark_read method: POST in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/channels/stats/member_count method: POST in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/channels/{channel_id}/common_teams method: GET in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/channels/{channel_id}/convert_to_channel method: POST in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/cloud/products/selfhosted method: GET in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/cloud/subscription/self-serve-status method: GET in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/cloud/request-trial method: PUT in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/cloud/validate-business-email method: POST in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/cloud/validate-workspace-business-email method: POST in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/cloud/check-cws-connection method: GET in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/cloud/delete-workspace method: DELETE in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/drafts method: POST in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/users/{user_id}/teams/{team_id}/drafts method: GET in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/users/{user_id}/channels/{channel_id}/drafts/{thread_id} method: DELETE in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/users/{user_id}/channels/{channel_id}/drafts method: DELETE in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/exports/{export_name:.+\\.zip}/presign-url method: POST in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/hosted_customer/signup_available method: GET in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/hosted_customer/bootstrap method: POST in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/hosted_customer/customer method: POST in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/hosted_customer/confirm method: POST in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/hosted_customer/confirm-expand method: POST in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/hosted_customer/invoices method: GET in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/hosted_customer/invoices/{invoice_id:in_[A-Za-z0-9]+}/pdf method: GET in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/hosted_customer/subscribe-newsletter method: POST in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/license/review method: POST in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/license/review/status method: GET in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/posts/{post_id}/edit_history method: GET in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/posts/{post_id}/info method: GET in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/posts/search method: POST in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/logs/query method: POST in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/latest_version method: GET in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/system/onboarding/complete method: GET in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/system/onboarding/complete method: POST in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/system/schema/version method: GET in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/usage/teams method: GET in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/users/login/desktop_token method: POST in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/users/notify-admin method: POST in OpenAPI 3 spec.' \ + -e 'Cannot find /api/v4/users/trigger-notify-admin-posts method: POST in OpenAPI 3 spec.' \ + -e "Handler /api/v4/cloud/subscription is defined with method PUT, but it's not in the spec" \ +2>&1 || true) + +if [[ ! -z "${OUTPUT_EXCLUDING_IGNORED// }" ]]; then + echo "Failing vet output" + echo "==================" + echo "$OUTPUT_EXCLUDING_IGNORED" + exit 1 +else + echo "Ignoring above errors." + exit 0 +fi