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

[CLOSES #1129] DashboardTabProvider #1142

Merged
merged 1 commit into from Aug 12, 2019
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -0,0 +1,54 @@
package misk.web

import misk.security.authz.AccessAnnotationEntry
import javax.inject.Inject
import javax.inject.Provider
import kotlin.reflect.KClass

class DashboardTab(
slug: String,
url_path_prefix: String,
val name: String,
val category: String = "Admin",
capabilities: Set<String> = setOf(),

This comment has been minimized.

Copy link
@wesleyk

wesleyk Aug 12, 2019

Collaborator

do we actually want defaults for this?

This comment has been minimized.

Copy link
@adrw

adrw Aug 12, 2019

Author Collaborator

I think so, though I'd be open to removing the default and forcing developers to deliberately choose authentication vs. being open by default. Is that what you'd recommend?

This comment has been minimized.

Copy link
@wesleyk

wesleyk Aug 12, 2019

Collaborator

deliberately choose authentication vs. being open by default

I think being open by default seems like a dangerous default.

services: Set<String> = setOf()
) : WebTab(slug, url_path_prefix, capabilities, services)

/**
* Sets the tab's authentication based on the injected AdminDashboardAccess access annotation entry
*/
class DashboardTabProvider(
val slug: String,
val url_path_prefix: String,
val name: String,
val category: String = "Admin",
val accessAnnotation: KClass<out Annotation>
) : Provider<DashboardTab> {
@Inject
lateinit var registeredEntries: List<AccessAnnotationEntry>

override fun get(): DashboardTab {
val accessAnnotationEntry = registeredEntries.find { it.annotation == accessAnnotation }!!
return DashboardTab(
slug = slug,
url_path_prefix = url_path_prefix,
name = name,
category = category,
capabilities = accessAnnotationEntry.capabilities.toSet(),
services = accessAnnotationEntry.services.toSet()
)
}
}

inline fun <reified A : Annotation> DashboardTabProvider(
slug: String,
url_path_prefix: String,
name: String,
category: String = "Admin"
) = DashboardTabProvider(
slug = slug,
url_path_prefix = url_path_prefix,

This comment has been minimized.

Copy link
@wesleyk

wesleyk Aug 12, 2019

Collaborator

camelcase?

This comment has been minimized.

Copy link
@adrw

adrw Aug 12, 2019

Author Collaborator

Snakecase was recommended in early work on tabs to signify that the variable was exposed as part of the API. I'm open to changing it to camelcase if we decide to set that recommendation aside. @swankjesse

This comment has been minimized.

Copy link
@wesleyk

wesleyk Aug 12, 2019

Collaborator

mm gotcha. I guess in my mind that makes sense for the API object itself, which I don't think this provider is

name = name,
category = category,
accessAnnotation = A::class
)
@@ -10,22 +10,19 @@ abstract class WebTab(
val capabilities: Set<String> = setOf(),
val services: Set<String> = setOf()
) : ValidWebEntry(slug = slug, url_path_prefix = url_path_prefix) {
fun isAuthenticated(caller: MiskCaller?): Boolean {
return when {
capabilities.isEmpty() && services.isEmpty() -> true // no capabilities/service requirement => unauthenticated requests allowed (including when caller is null)
caller == null -> false // capability/service requirement present but caller null => assume authentication broken
capabilities.any { caller.capabilities.contains(it) } -> true // matching capability
services.any { caller.service == it } -> true // matching service
else -> false
}
fun isAuthenticated(caller: MiskCaller?): Boolean = when {

This comment has been minimized.

Copy link
@wesleyk

wesleyk Aug 12, 2019

Collaborator

any particular reason we have to re-implement our authentication logic for web tabs vs web actions? Would be good to have a single source of truth for what is pretty critical security logic

This comment has been minimized.

Copy link
@adrw

adrw Aug 12, 2019

Author Collaborator

Good point, we can reused the isAllowed function from AccessInterceptor. Will do so in a follow up PR.

// no capabilities/service requirement => unauthenticated and null caller requests allowed
capabilities.isEmpty() && services.isEmpty() -> true

// capability/service requirement present but caller null => assume authentication broken
caller == null -> false

// matching capability
capabilities.any { caller.capabilities.contains(it) } -> true

// matching service
services.any { caller.service == it } -> true

else -> false
}
}

class DashboardTab(
slug: String,
url_path_prefix: String,
val name: String,
val category: String = "Container Admin",
capabilities: Set<String> = setOf(),
services: Set<String> = setOf()
) : WebTab(slug, url_path_prefix, capabilities, services)
@@ -5,6 +5,7 @@ import misk.environment.Environment
import misk.inject.KAbstractModule
import misk.security.authz.AccessAnnotationEntry
import misk.web.DashboardTab
import misk.web.DashboardTabProvider
import misk.web.NetworkInterceptor
import misk.web.WebActionModule
import misk.web.actions.AdminDashboardTab
@@ -30,63 +31,66 @@ class AdminDashboardModule(val environment: Environment) : KAbstractModule() {
install(WebActionModule.create<AdminDashboardTabAction>())
install(WebActionModule.create<ServiceMetadataAction>())
install(WebTabResourceModule(
environment = environment,
slug = "loader",
web_proxy_url = "http://localhost:3100/"
environment = environment,
slug = "loader",
web_proxy_url = "http://localhost:3100/"
))
install(WebTabResourceModule(
environment = environment,
slug = "loader",
web_proxy_url = "http://localhost:3100/",
url_path_prefix = "/_admin/",
resourcePath = "classpath:/web/_tab/loader/"
environment = environment,

This comment has been minimized.

Copy link
@swankjesse

swankjesse Aug 12, 2019

Collaborator

this reformatting is wrong

This comment has been minimized.

Copy link
@adrw

adrw Aug 12, 2019

Author Collaborator

Our KtLint configuration changes the continuation_indent_size from the Kotlin default 4 to 2:

ktlint(dep.ktlintVersion).userData(['indent_size': '2', 'continuation_indent_size' : '2'])
slug = "loader",
web_proxy_url = "http://localhost:3100/",
url_path_prefix = "/_admin/",
resourcePath = "classpath:/web/_tab/loader/"
))

// @misk packages
install(WebTabResourceModule(
environment = environment,
slug = "@misk",
web_proxy_url = "http://localhost:9100/",
url_path_prefix = "/@misk/",
resourcePath = "classpath:/web/_tab/loader/@misk/"
environment = environment,
slug = "@misk",
web_proxy_url = "http://localhost:9100/",
url_path_prefix = "/@misk/",
resourcePath = "classpath:/web/_tab/loader/@misk/"
))

// Config
install(WebActionModule.create<ConfigMetadataAction>())
multibind<DashboardTab, AdminDashboardTab>().toInstance(DashboardTab(
name = "Config",
multibind<DashboardTab, AdminDashboardTab>().toProvider(
DashboardTabProvider<AdminDashboardAccess>(
slug = "config",
url_path_prefix = "/_admin/config/",
name = "Config",
category = "Container Admin"
))
))
install(WebTabResourceModule(
environment = environment,
slug = "config",
web_proxy_url = "http://localhost:3200/"
environment = environment,
slug = "config",
web_proxy_url = "http://localhost:3200/"
))

// Web Actions
install(WebActionModule.create<WebActionMetadataAction>())
multibind<DashboardTab, AdminDashboardTab>().toInstance(DashboardTab(
name = "Web Actions",
multibind<DashboardTab, AdminDashboardTab>().toProvider(
DashboardTabProvider<AdminDashboardAccess>(
slug = "web-actions",
url_path_prefix = "/_admin/web-actions/"
))
url_path_prefix = "/_admin/web-actions/",
name = "Web Actions",
category = "Container Admin"
))
install(WebTabResourceModule(
environment = environment,
slug = "web-actions",
web_proxy_url = "http://localhost:3201/"
environment = environment,
slug = "web-actions",
web_proxy_url = "http://localhost:3201/"
))
}
}

// Module that allows testing/development environments to bind up the admin dashboard
class AdminDashboardTestingModule(val environment: Environment) : KAbstractModule() {
override fun configure() {
install(AdminDashboardModule(environment))
multibind<AccessAnnotationEntry>()
// Set dummy values for access, these shouldn't matter,
// as test environments should prefer to use the FakeCallerAuthenticator.
.toInstance(AccessAnnotationEntry<AdminDashboardAccess>(capabilities = listOf("admin_access")))
// Set dummy values for access, these shouldn't matter,
// as test environments should prefer to use the FakeCallerAuthenticator.
.toInstance(AccessAnnotationEntry<AdminDashboardAccess>(capabilities = listOf("admin_access")))
install(AdminDashboardModule(environment))
}
}
@@ -7,7 +7,7 @@ import misk.inject.KAbstractModule
import misk.web.metadata.AdminDashboardTestingModule

// Common test module used to be able to test admin dashboard WebActions
class TestAdminDashboardActionModule : KAbstractModule() {
class AdminDashboardActionTestingModule : KAbstractModule() {
override fun configure() {
install(TestWebActionModule())
install(AdminDashboardTestingModule(Environment.TESTING))
@@ -17,4 +17,4 @@ class TestAdminDashboardActionModule : KAbstractModule() {
}
}

class TestAdminDashboardConfig : Config
class TestAdminDashboardConfig : Config
@@ -0,0 +1,86 @@
package misk.web.actions

import com.squareup.moshi.Moshi
import misk.client.HttpClientEndpointConfig
import misk.client.HttpClientFactory
import misk.moshi.adapter
import misk.security.authz.FakeCallerAuthenticator
import misk.testing.MiskTest
import misk.testing.MiskTestModule
import misk.web.jetty.JettyService
import okhttp3.OkHttpClient
import okhttp3.Request
import org.assertj.core.api.Assertions.assertThat
import org.junit.jupiter.api.Test
import javax.inject.Inject
import kotlin.test.assertEquals
import kotlin.test.assertNotNull

@MiskTest(startService = true)
class AdminDashboardTabActionTest {
@MiskTestModule
val module = AdminDashboardActionTestingModule()

@Inject private lateinit var jetty: JettyService
@Inject private lateinit var httpClientFactory: HttpClientFactory

val path = "/api/admindashboardtabs"

@Test fun customCapabilityAccess_unauthenticated() {
val response = executeRequest(path = path)
assertEquals(0, response.adminDashboardTabs.size)
}

@Test fun customCapabilityAccess_unauthorized() {
val response = executeRequest(
path = path,
user = "sandy",
capabilities = "guest"
)
assertEquals(0, response.adminDashboardTabs.size)
}

@Test fun customCapabilityAccess_authorized() {
val response = executeRequest(
path = path,
user = "sandy",
capabilities = "admin_access"
)
assertEquals(2, response.adminDashboardTabs.size)
assertNotNull(response.adminDashboardTabs.find { it.name == "Config" })
assertNotNull(response.adminDashboardTabs.find { it.name == "Web Actions" })
}

private fun executeRequest(
path: String = "/",
service: String? = null,
user: String? = null,
capabilities: String? = null
): AdminDashboardTabAction.Response {
val client = createOkHttpClient()

val baseUrl = jetty.httpServerUrl
val requestBuilder = Request.Builder()
.url(baseUrl.resolve(path)!!)
service?.let {
requestBuilder.header(FakeCallerAuthenticator.SERVICE_HEADER, service)
}
user?.let {
requestBuilder.header(FakeCallerAuthenticator.USER_HEADER, user)
}
capabilities?.let {
requestBuilder.header(FakeCallerAuthenticator.CAPABILITIES_HEADER, capabilities)
}
val call = client.newCall(requestBuilder.build())
val response = call.execute()

val moshi = Moshi.Builder().build()
val responseAdaptor = moshi.adapter<AdminDashboardTabAction.Response>()
return responseAdaptor.fromJson(response.body!!.source())!!
}

private fun createOkHttpClient(): OkHttpClient {
val config = HttpClientEndpointConfig(jetty.httpServerUrl.toString())
return httpClientFactory.create(config)
}
}
@@ -12,7 +12,7 @@ import org.junit.jupiter.api.Test
@MiskTest(startService = true)
class ConfigMetadataActionTest {
@MiskTestModule
val module = TestAdminDashboardActionModule()
val module = AdminDashboardActionTestingModule()

val testConfig = TestConfig(
IncludedConfig("foo"),
@@ -1,38 +1,10 @@
package misk.web.actions

import misk.inject.KAbstractModule
import misk.logging.getLogger
import misk.testing.MiskTest
import misk.testing.MiskTestModule
import misk.web.DashboardTab
import org.junit.jupiter.api.Test
import javax.inject.Inject
import kotlin.test.assertFailsWith

@MiskTest
internal class DashboardTabTest {
@MiskTestModule
val module = TestModule()

@Inject lateinit var dashboardTabs: List<DashboardTab>

private val logger = getLogger<DashboardTabTest>()

class TestModule : KAbstractModule() {
override fun configure() {
multibind<DashboardTab>().toInstance(DashboardTab(
slug = "dashboard",
url_path_prefix = "/_admin/dashboard/",
name = "Dashboard"
))
}
}

@Test
internal fun testBindings() {
logger.info(dashboardTabs.toString())
}

@Test
internal fun tabGoodSlug() {
DashboardTab("good-1-slug-test", url_path_prefix = "/a/path/", name = "Name")
@@ -54,20 +26,23 @@ internal class DashboardTabTest {

@Test
internal fun tabGoodCategory() {
DashboardTab(slug = "slug", url_path_prefix = "/a/path/", name = "Name", category = "@tea-pot_418")
DashboardTab(slug = "slug", url_path_prefix = "/a/path/", name = "Name",
category = "@tea-pot_418")

This comment has been minimized.

Copy link
@swankjesse

swankjesse Aug 12, 2019

Collaborator

wrap at +4!

}

@Test
internal fun tabCategoryWithSpace() {
assertFailsWith<IllegalArgumentException> {
DashboardTab(slug = "bad slug", url_path_prefix = "/a/path/", name = "Name", category = "bad icon")
DashboardTab(slug = "bad slug", url_path_prefix = "/a/path/", name = "Name",
category = "bad icon")
}
}

@Test
internal fun tabCategoryWithUpperCase() {
assertFailsWith<IllegalArgumentException> {
DashboardTab(slug = "BadSlug", url_path_prefix = "/a/path/", name = "Name", category = "Bad-Icon")
DashboardTab(slug = "BadSlug", url_path_prefix = "/a/path/", name = "Name",
category = "Bad-Icon")
}
}

@@ -11,7 +11,7 @@ import javax.inject.Inject
@MiskTest(startService = true)
class WebActionMetadataActionTest {
@MiskTestModule
val module = TestAdminDashboardActionModule()
val module = AdminDashboardActionTestingModule()

@Inject lateinit var webActionMetadataAction: WebActionMetadataAction

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.