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

714-rules-to-aos--AvoidLinkHeadersRule #763

Merged
5 commits merged into from
Jul 26, 2018
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
44 changes: 30 additions & 14 deletions server/src/main/java/de/zalando/zally/rule/Context.kt
Original file line number Diff line number Diff line change
Expand Up @@ -122,30 +122,46 @@ class Context(openApi: OpenAPI, swagger: Swagger? = null) {
private val log = LoggerFactory.getLogger(Context::class.java)
val extensionNames = arrayOf("getVendorExtensions", "getExtensions")

fun createOpenApiContext(content: String): Context? {
fun createOpenApiContext(content: String, failOnParseErrors: Boolean = false): Context? {
val parseOptions = ParseOptions()
parseOptions.isResolve = true
// parseOptions.isResolveFully = true // https://github.com/swagger-api/swagger-parser/issues/682

return OpenAPIV3Parser().readContents(content, null, parseOptions)?.openAPI?.let {
val parseResult = OpenAPIV3Parser().readContents(content, null, parseOptions)
if (failOnParseErrors && parseResult.messages.orEmpty().isNotEmpty()) {
val sep = "\n - "
val messageBulletList = parseResult.messages.joinToString(sep)
throw RuntimeException("Swagger parsing failed with those errors:$sep$messageBulletList")
}
return parseResult?.openAPI?.let {
ResolverFully(true).resolveFully(it) // workaround for NPE bug in swagger-parser
Context(it)
}
}

fun createSwaggerContext(content: String): Context? =
SwaggerParser().readWithInfo(content, true)?.let {
val swagger = it.swagger ?: return null
val openApi = SwaggerConverter().convert(it)?.openAPI

openApi?.let {
try {
ResolverFully(true).resolveFully(it)
} catch (e: NullPointerException) {
log.warn("Failed to fully resolve Swagger schema.", e)
}
Context(openApi, swagger)
fun createSwaggerContext(content: String, failOnParseErrors: Boolean = false): Context? =
SwaggerParser().readWithInfo(content, true)?.let { parseResult ->
if (failOnParseErrors && parseResult.messages.orEmpty().isNotEmpty()) {
val sep = "\n - "
val messageBulletList = parseResult.messages.joinToString(sep)
throw RuntimeException("Swagger parsing failed with those errors:$sep$messageBulletList")
}
val swagger = parseResult.swagger ?: return null
val convertResult = SwaggerConverter().convert(parseResult)
if (failOnParseErrors && convertResult.messages.orEmpty().isNotEmpty()) {
val sep = "\n - "
val messageBulletList = parseResult.messages.joinToString(sep)
throw RuntimeException("Swagger conversion to OpenAPI 3 failed with those errors:$sep$messageBulletList")
}
convertResult?.openAPI?.let {
try {
ResolverFully(true).resolveFully(it)
} catch (e: NullPointerException) {
log.warn("Failed to fully resolve Swagger schema.", e)
if (failOnParseErrors) throw e
}
Context(it, swagger)
}
}
}
}
Original file line number Diff line number Diff line change
@@ -1,31 +1,32 @@
package de.zalando.zally.rule.zalando

import com.typesafe.config.Config
import de.zalando.zally.rule.Context
import de.zalando.zally.rule.HttpHeadersRule
import de.zalando.zally.rule.api.Check
import de.zalando.zally.rule.api.Rule
import de.zalando.zally.rule.api.Severity
import de.zalando.zally.rule.api.Violation
import de.zalando.zally.rule.api.Rule
import io.swagger.models.Swagger
import de.zalando.zally.util.getAllHeaders
import org.springframework.beans.factory.annotation.Autowired

@Rule(
ruleSet = ZalandoRuleSet::class,
id = "166",
severity = Severity.MUST,
title = "Avoid Link in Header Rule"
ruleSet = ZalandoRuleSet::class,
id = "166",
severity = Severity.MUST,
title = "Avoid Link in Header Rule"
)
class AvoidLinkHeadersRule(@Autowired rulesConfig: Config) : HttpHeadersRule(rulesConfig) {
private val description = "Do Not Use Link Headers with JSON entities"
class AvoidLinkHeadersRule(@Autowired rulesConfig: Config) {

@Check(severity = Severity.MUST)
override fun validate(swagger: Swagger): Violation? {
return super.validate(swagger)
}
private val headersWhitelist = rulesConfig.getStringList(HttpHeadersRule::class.simpleName + ".whitelist").toSet()

override fun isViolation(header: String) = header == "Link"
private val description = "Do Not Use Link Headers with JSON entities"

override fun createViolation(paths: List<String>): Violation {
return Violation(description, paths)
@Check(severity = Severity.MUST)
fun validate(context: Context): List<Violation> {
val allHeaders = context.api.getAllHeaders()
return allHeaders
.filter { it.name !in headersWhitelist && it.name == "Link" }
.map { context.violation(description, it.element) } // createViolation(context, it) }
}
}
37 changes: 37 additions & 0 deletions server/src/main/java/de/zalando/zally/util/OpenApiUtil.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
package de.zalando.zally.util

import io.swagger.v3.oas.models.OpenAPI
import io.swagger.v3.oas.models.parameters.Parameter
import io.swagger.v3.oas.models.responses.ApiResponse

data class HeaderElement(
val name: String,
val element: Any
)

fun OpenAPI.getAllHeaders(): Set<HeaderElement> {

fun Collection<Parameter>?.extractHeaders() = orEmpty()
.filter { it.`in` == "header" }
.map { HeaderElement(it.name, it) }
.toSet()

fun Collection<ApiResponse>?.extractHeaders() = orEmpty()
.flatMap { it.headers.orEmpty().entries }
.map { HeaderElement(it.key, it.value) }
.toSet()

val fromParams = components.parameters.orEmpty().values.extractHeaders()

val fromPaths = paths.orEmpty().flatMap { (_, path) ->
val fromPathParameters = path.parameters.extractHeaders()
val fromOperations = path.readOperations().flatMap { operation ->
val fromOpParams = operation.parameters.extractHeaders()
val fromOpResponses = operation.responses.orEmpty().values.extractHeaders()
fromOpParams + fromOpResponses
}
fromPathParameters + fromOperations
}

return fromParams + fromPaths
}
6 changes: 4 additions & 2 deletions server/src/test/java/de/zalando/zally/TestUtil.kt
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,11 @@ val testConfig: Config by lazy {

fun getFixture(fileName: String): Swagger = SwaggerParser().read("fixtures/$fileName")

fun getContextFromFixture(fileName: String): Context? {
fun getContextFromFixture(fileName: String): Context {
val content = getResourceContent(fileName)
return Context.createOpenApiContext(content) ?: Context.createSwaggerContext(content)
return Context.createOpenApiContext(content)
?: Context.createSwaggerContext(content)
?: throw RuntimeException("Unable to create context.")
}

fun getResourceContent(fileName: String): String = ClasspathHelper.loadFileFromClasspath("fixtures/$fileName")
Expand Down
Original file line number Diff line number Diff line change
@@ -1,31 +1,135 @@
package de.zalando.zally.rule.zalando

import de.zalando.zally.getFixture
import de.zalando.zally.rule.Context
import de.zalando.zally.rule.ZallyAssertions.Companion.assertThat
import de.zalando.zally.testConfig
import org.assertj.core.api.Assertions.assertThat
import org.intellij.lang.annotations.Language
import org.junit.Test

class AvoidLinkHeadersRuleTest {

private val rule = AvoidLinkHeadersRule(testConfig)

@Test
fun positiveCaseSpp() {
val swagger = getFixture("api_spp.json")
assertThat(rule.validate(swagger)).isNull()
fun `a Swagger API with no header called Link produces no violation`() {
@Language("YAML")
val context = Context.createSwaggerContext("""
swagger: 2.0
Copy link
Contributor

Choose a reason for hiding this comment

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

You can have an even smaller example, like:

swagger: 2.0
paths:
  /foo:
    get:
      parameters:
        FlowId:
          name: X-Flow-Id
          in: header
          type: string

BTW, the parameters are wrongly intended in your example (should be under get:).

Copy link
Author

Choose a reason for hiding this comment

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

Those parameters are for all of the API, not just for that path/verb.

Copy link
Author

Choose a reason for hiding this comment

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

I wanted the example to include:

  • a header in a path/verb
  • a header in the global parameters
  • another parameter that is not a header

to make sure the test does not return false negatives.

I can remove one of the header parameter for shortening it.

I can also add one that is called Link but is not a header, again to shield against false negatives in the test.

info:
title: Clean Swagger API
paths:
/foo:
get:
description: Lorem Ipsum
responses:
202:
description: Lorem Ipsum
headers:
Location: # should not violate since not called `Link`
type: string
format: url
parameters:
FlowId: # should not violate since not named `Link`
name: X-Flow-Id
in: header
type: string
Link: # should not violate since not a header
name: Link
in: query
type: string
ProductId: # should not violate since not a header nor named `Link`
name: product_id
in: path
type: string
""".trimIndent(), failOnParseErrors = true)!!
val violations = rule.validate(context)
assertThat(violations).isEmpty()
}

@Test
fun positiveCaseSpa() {
val swagger = getFixture("api_spa.yaml")
assertThat(rule.validate(swagger)).isNull()
fun `an OpenAPI 3 API with no header called Link produces no violation`() {
@Language("YAML")
val context = Context.createOpenApiContext("""
openapi: 3.0.0
info:
title: Clean Swagger API
version: 1.0.0
paths:
/foo:
get:
description: Lorem Ipsum
responses:
202:
description: Lorem Ipsum
headers:
Location:
schema:
type: string
format: url
components:
parameters:
FlowId:
name: X-Flow-Id
in: header
required: false
schema:
type: string
Authorization:
name: Authorization
in: header
required: true
schema:
type: string
ProductId:
name: product_id
in: path
required: true
schema:
type: string
""".trimIndent(), failOnParseErrors = true)!!
val violations = rule.validate(context)
assertThat(violations).isEmpty()
}

@Test
fun negativeCase() {
val swagger = getFixture("avoidLinkHeaderRuleInvalid.json")
val violation = rule.validate(swagger)!!
assertThat(violation.paths).hasSameElementsAs(
listOf("/product-put-requests/{product_path} Link", "/products Link"))
fun `an API with Link headers causes violations`() {
@Language("YAML")
val context = Context.createSwaggerContext("""
swagger: 2.0
info:
title: Clean Swagger API
paths:
/foo:
get:
parameters:
- name: Authorization
in: header
type: string
- name: Link
in: header
type: string
responses:
202:
description: Lorem Ipsum
headers:
Location:
type: string
format: url
post:
responses:
202:
description: Lorem Ipsum
headers:
Link:
type: string
format: url
""".trimIndent(), failOnParseErrors = true)!!
val violations = rule.validate(context)
assertThat(violations)
.descriptionsAllEqualTo("Do Not Use Link Headers with JSON entities")
.pointersEqualTo(
"/paths/~1foo/get/parameters/1",
"/paths/~1foo/post/responses/202/headers/Link"
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ class MediaTypesRuleTest {

@Test
fun `the SPP API generates violations`() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would replace tests based on a complete API specification with test cases describing the behavior of the check functions.

Copy link
Author

Choose a reason for hiding this comment

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

This has not changed since the PR for this rule. Only the !! have been removed because of a change in getContextFromFixture.

val context = getContextFromFixture("api_spp.json")!!
val context = getContextFromFixture("api_spp.json")
val result = rule.validate(context)
assertThat(result).hasSameElementsAs(listOf(
// --- consumes ---
Expand All @@ -137,7 +137,7 @@ class MediaTypesRuleTest {

@Test
fun `the SPA API generates no violations`() {
val context = getContextFromFixture("api_spa.yaml")!!
val context = getContextFromFixture("api_spa.yaml")
assertThat(rule.validate(context)).isEmpty()
}

Expand Down