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

Conversation

@adrw
Copy link
Collaborator

commented Aug 9, 2019

  • New pattern of multibinding tabs to a DashboardTabProvider along with an AccessAnnotation

  • Allows for Misk tabs to be set with the authentication from the multibound AdminDashboardAccess access annotation

  • Also allows for service tabs to be easily set with service specific Access Annotations

  • Fixes small bug in Loader tab where dashboard doesn't load when no tabs are bound

@adrw

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 9, 2019

Thanks to @mmihic for help with Guice Provider pattern!

@adrw adrw force-pushed the adrw:adrw/20190807.1129 branch from bf146a4 to d424c22 Aug 12, 2019

@adrw adrw changed the title [CLOSES #1129] DashboardTabAuthenticatedProvider [CLOSES #1129] DashboardTabProvider Aug 12, 2019

@adrw adrw force-pushed the adrw:adrw/20190807.1129 branch from d424c22 to 43fb987 Aug 12, 2019

else -> false
}
fun isAuthenticated(caller: MiskCaller?): Boolean = when {
capabilities.isEmpty() && services.isEmpty() -> true // no capabilities/service requirement => unauthenticated requests allowed (including when caller is null)

This comment has been minimized.

Copy link
@swankjesse

swankjesse Aug 12, 2019

Collaborator

nit for next time: if ya gonna reformat, wrap to 100 please

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'])
val path = "/api/admindashboardtabs"
val empty = "{\"adminDashboardTabs\":[]}"
val tabs =
"{\"adminDashboardTabs\":[{\"slug\":\"config\",\"url_path_prefix\":\"/_admin/config/\",\"name\":\"Config\",\"category\":\"Container Admin\",\"capabilities\":[\"admin_access\"],\"services\":[]},{\"slug\":\"web-actions\",\"url_path_prefix\":\"/_admin/web-actions/\",\"name\":\"Web Actions\",\"category\":\"Container Admin\",\"capabilities\":[\"admin_access\"],\"services\":[]}]}"

This comment has been minimized.

Copy link
@swankjesse

swankjesse Aug 12, 2019

Collaborator

pretty print this JSON and use a multiline string?

This comment has been minimized.

Copy link
@swankjesse

swankjesse Aug 12, 2019

Collaborator

and then you can assert on the object, not on its string representation?

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

This comment has been minimized.

Copy link
@adrw

adrw Aug 12, 2019

Author Collaborator

Perfect! I wanted to do this but I didn't know how 👍

@@ -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!

@adrw adrw force-pushed the adrw:adrw/20190807.1129 branch 3 times, most recently from cb7e6dd to c41e449 Aug 12, 2019

[CLOSES #1129] DashboardTabProvider
* New pattern of multibinding tabs to a DashboardTabProvider along with an AccessAnnotation
* Allows for Misk tabs to be set with the authentication from the multibound `AdminDashboardAccess` access annotation
* Also allows for service tabs to be easily set with service specific Access Annotations

* Fixes small bug in Loader tab where dashboard doesn't load when no tabs are bound

@adrw adrw force-pushed the adrw:adrw/20190807.1129 branch from c41e449 to d2ea3f7 Aug 12, 2019

@adrw adrw merged commit 9621367 into cashapp:master Aug 12, 2019

2 checks passed

ci/circleci: java Your tests passed on CircleCI!
Details
ci/circleci: node Your tests passed on CircleCI!
Details
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.

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

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.

adrw added a commit to adrw/misk that referenced this pull request Aug 12, 2019

adrw added a commit to adrw/misk that referenced this pull request Aug 12, 2019

adrw added a commit to adrw/misk that referenced this pull request Aug 12, 2019

adrw added a commit to adrw/misk that referenced this pull request Aug 12, 2019

adrw added a commit to adrw/misk that referenced this pull request Aug 12, 2019

adrw added a commit to adrw/misk that referenced this pull request Aug 12, 2019

adrw added a commit that referenced this pull request Aug 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.