feat(cred): credential test table #920

tthvo · 2023-03-22T22:50:32Z

Welcome to Cryostat! 👋

Before contributing, make sure you have:

Read the contributing guidelines
Linked a relevant issue which this PR resolves
Linked any other relevant issues, PR's, or documentation, if any
Resolved all conflicts, if any
Rebased your branch PR on top of the latest upstream main branch
Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
Signed the last commit: git commit --amend --signoff

Fixes: #592
Fixes: #593
Related to #914
Depends on https://github.com/cryostatio/cryostat/issues/1431

Description of the change:

Added a table for testing credentials against matching targets.
Hide/Fade targets when match expression is empty.
Listview should be scrollable (allow sticky radio button)
Remove explicit mentioning of JMX Credentials (mostly internals that are not visible to the users).

Motivation for the change:

See #591

How to manually test:

Run CRYOSTAT_IMAGE=quay.io... sh smoketest.sh...
Go to Security -> Add Store Credentials -> Test tab.

Screenshots

EDIT: See #920 (comment)

github-actions · 2023-03-22T22:57:34Z

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-920-7e884f78ebb78118782b5484e0f38a429a1696cc sh smoketest.sh

github-actions · 2023-03-23T06:05:19Z

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-920-33d26258c30644b7d11489e7b47c1ae9cdaf996e sh smoketest.sh

github-actions · 2023-03-23T23:51:15Z

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-920-b538da5c82db7f30010c20a6f308dd225062b0b0 sh smoketest.sh

github-actions · 2023-03-24T00:04:20Z

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-920-3c7ed6a03152122e02e7a8916ff4b207617ac8cf sh smoketest.sh

github-actions · 2023-03-24T01:08:33Z

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-920-9ef46ad794df3f185504e7395fd2a1373d4a4f4e sh smoketest.sh

github-actions · 2023-03-24T01:21:02Z

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-920-74c4ac556267adc5e38a60b9ea75b4ff5f3a2667 sh smoketest.sh

github-actions · 2023-03-24T01:38:21Z

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-920-cf5c7d7413f4b103983ccbdf04abedcf72adabb9 sh smoketest.sh

maxcao13 · 2023-03-24T19:20:06Z

The UI looks great! Just a few questions:

Using a "true" matchExpr and a smoketest smoketest credentials:

Should there be an indication that a target does not need credentials? Right now the test will say that it is "valid" it doesn't mean the credentials are valid for that target.

tthvo · 2023-03-24T19:26:34Z

Should there be an indication that a target does not need credentials? Right now the test will say that it is "valid" it doesn't mean the credentials are valid for that target.

Try hover over the valid label. It should say some warning :)) Any suggestion for that?

Should there be an indication that a target does not need credentials?

Was thinking about it but it seems quite hard. The way it validates credentials now is to fetch active recordings while putting the credentials in X-JMX-Authentication. This means if the target already has a valid credential, in this context, its no different from not-requiring ones.

I could try sending an initial check with some very random bad username/password. However, no guarantee it is not being used, maybe we can utilize empty username/password?

maxcao13 · 2023-03-24T19:41:41Z

The only thing I can think of right now, is to send a graphql request to each of the targets with the form

query {
    targetNodes {
        mbeanMetrics {
            runtime {
                inputArguments
            }
        }
    }
}

and then parse the resulting array for the input argument:

"-Dcom.sun.management.jmxremote.authenticate=false"

and checking if it [exists, false, or true].

Not sure if there are better ways. The query should fail if the credentials are wrong anyways since we are calling the remote mxbean on the target, so it would be similar to getting the recordings in terms of checking if credentials work.

EDIT: Apprently that authenticate property is default true, so I assume if it doesn't exist, the jvm should need auth.

maxcao13 · 2023-03-24T19:57:19Z

I think the hint is wrong, there is no longer a target select dropdown here. There is also no context object. Should there be a context object for the user? I think I would find it hard to know what the match expression should include. The ? only includes two fields I could query (annotations, and alias).

tthvo · 2023-03-24T20:27:30Z

The only thing I can think of right now, is to send a graphql request to each of the targets with the form

Oh niceee! Sounds good I will redirect to this approach :))

I think the hint is wrong, there is no longer a target select dropdown here. There is also no context object. Should there be a context object for the user? I think I would find it hard to know what the match expression should include. The ? only includes two fields I could query (annotations, and alias).

Ahh the hint is basically constructed from any target (i.e. first one). It just serves as an overall and simple hint for arbitrary target similar to before. It is this way cuz now we don't have target dropdown anymore.

EDIT: I seee now! U mean the input helper text. Thanks!

The ? only includes two fields I could query (annotations, and alias).

We can include all fields but would it be too clustered? There is also a property path hint when hovering over the field name in the detail sidecar. We can always add a quickstart/guide for this? What do you think?

github-actions · 2023-03-24T21:38:40Z

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-920-57465567c7fe6c61dd1dc12edbba8d4e4cce3711 sh smoketest.sh

tthvo · 2023-03-24T22:27:09Z

Worked very nicee! Thanks a lot!

github-actions · 2023-03-24T22:33:23Z

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-920-f838681a924937bd7ae9765216906d910ef6a072 sh smoketest.sh

andrewazores · 2023-03-26T20:52:02Z

The only thing I can think of right now, is to send a graphql request to each of the targets with the form
query {
    targetNodes {
        mbeanMetrics {
            runtime {
                inputArguments
            }
        }
    }
}
and then parse the resulting array for the input argument:
"-Dcom.sun.management.jmxremote.authenticate=false"
and checking if it [exists, false, or true].

Not sure if there are better ways. The query should fail if the credentials are wrong anyways since we are calling the remote mxbean on the target, so it would be similar to getting the recordings in terms of checking if credentials work.

EDIT: Apprently that authenticate property is default true, so I assume if it doesn't exist, the jvm should need auth.

This feels a little fragile...

https://docs.oracle.com/javadb/10.10.1.2/adminguide/radminjmxenablepwd.html#radminjmxenablepwd

I would need to do a lot more checking of different docs for different JDK versions, but I would hesitate to say that -Dcom.sun.management.jmxremote.authenticate alone is sufficient to tell if the JVM is configured to use JMX credentials or not. Neglecting that property but setting -Dcom.sun.management.jmxremote.password.file would probably implicitly enable credentials too, I think, and there are probably other such combinations, and the behaviour could be different for different JDK versions. It also feels particularly fragile to do it this way since this is specifically the inputArguments property, which I believe is really just the command line passed to the process when it was forked, but what we're trying to examine is a Java System Property and those are actually mutable at runtime. So the value of that property within the target at the time that we read it may not be the same as the value that the target had as an input argument at the time the process forked.

If we are able to make this query and determine the inputArguments then this already means we're able to establish a connection to the target, so either it did not require credentials or we were successfully able to use stored ones.

Why don't we just have a connection checking endpoint that specifically does not load any stored credentials? It either uses only the session credentials header for the test, or it ignores the session credentials header and requires the credentials to test to be passed as part of the request body. We can use this endpoint and pass no credentials at first, and just try to open a JMX connection to read some basic MBean data. If that succeeds then we know the target did not require auth. Then the user can supply credentials and we can repeat the test using those.

src/app/Shared/Services/Api.service.tsx

github-actions · 2023-03-27T21:31:17Z

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-920-7c7071fd5df03d26e6a8d37806aaaecaee18b202 sh smoketest.sh

github-actions · 2023-03-28T02:01:07Z

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-920-62c37d8c07f85c2045adfa06e26c4332ba1ca5fc sh smoketest.sh

github-actions · 2023-03-28T02:11:17Z

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-920-f4bfac612a46d6a1f0f089c66f37c5bfdf844bdd sh smoketest.sh

github-actions · 2023-03-29T01:00:50Z

This PR/issue depends on:

~~cryostatio/cryostat#1431~~
By Dependent Issues (🤖). Happy coding!

Signed-off-by: Thuan Vo <thvo@redhat.com>

github-actions · 2023-03-29T04:26:52Z

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-920-697e6c78da23e7b8c92a6554072ac615a38cc366 sh smoketest.sh

Signed-off-by: Thuan Vo <thvo@redhat.com>

github-actions · 2023-03-29T07:13:41Z

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-920-c8c0a4d23d119aa94ad2bc6e90ec21f00a8e2e22 sh smoketest.sh

maxcao13 · 2023-03-29T19:42:16Z

The tooltip says "ignoring filters", does that mean the test all feature should ignore filters and test all anyways?

tthvo · 2023-03-29T20:51:54Z

~~Yehh it just means that all matching targets will start testing even if you filter it out (e.g. for example, by status or by name).~~

Opps I got myself confused. It was the old implementation. Will remove that

github-actions · 2023-03-29T22:34:18Z

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-920-edb9dd02cf433a0480d39e9a36ac3561f1d65ee1 sh smoketest.sh

maxcao13

Looks great to me! Awesome job.

andrewazores

Awesome indeed.

tthvo added the feat New feature or request label Mar 22, 2023

mergify bot added the safe-to-test label Mar 22, 2023

tthvo force-pushed the creds-test-table branch from 7e884f7 to 33d2625 Compare March 23, 2023 05:58

tthvo force-pushed the creds-test-table branch from a96e7c8 to b538da5 Compare March 23, 2023 23:45

tthvo force-pushed the creds-test-table branch from 4e9c4ce to 9ef46ad Compare March 24, 2023 01:02

tthvo requested review from maxcao13 and andrewazores March 24, 2023 01:16

tthvo marked this pull request as ready for review March 24, 2023 01:16

tthvo mentioned this pull request Mar 24, 2023

feat(tables): sortable tables #924

Merged

12 tasks

andrewazores reviewed Mar 26, 2023

View reviewed changes

src/app/Shared/Services/Api.service.tsx Outdated Show resolved Hide resolved

tthvo mentioned this pull request Mar 27, 2023

[Request] Credential test endpoint cryostatio/cryostat-legacy#1431

Closed

github-actions bot added the dependent label Mar 27, 2023

andrewazores mentioned this pull request Mar 28, 2023

[Task] Add GraphQL filter ability for node name from set cryostatio/cryostat-legacy#1433

Closed

github-actions bot removed the dependent label Mar 29, 2023

Thuan Vo added 13 commits March 29, 2023 00:19

feat(cred): credential test table

059fd33

Signed-off-by: Thuan Vo <thvo@redhat.com>

fix(matchExpr): fade targets when match expr is empty

142988b

chore(matchExpr): clean up empty state text

89006bb

feat(cred): enable test all buttons

10e21de

feat(cred): test pool

a81c38d

fix(cred): use 16ms/60fps interval checks

a20032d

chore(matchExpr): scrollable list view

3494c13

chore(cred): clean up explicit ref to jmx credentials

928aa13

chore(matchExpr): remove outdated helper text

719a1db

fix(cred): use mbean metrics to check credentials

7b9373c

chore(cred): mount menus to portalRoot

aa7bf39

fix(cred): use test endpoint for credential testing

b12853f

fix(cred): fix localstorage key

697e6c7

tthvo force-pushed the creds-test-table branch from f4bfac6 to 697e6c7 Compare March 29, 2023 04:19

chore(api): remove unused funcs

c8c0a4d

Signed-off-by: Thuan Vo <thvo@redhat.com>

tthvo force-pushed the creds-test-table branch from ae764e1 to c8c0a4d Compare March 29, 2023 07:07

fix(cred): disable test all if no targets are available to test

edb9dd0

maxcao13 approved these changes Mar 29, 2023

View reviewed changes

andrewazores approved these changes Mar 30, 2023

View reviewed changes

andrewazores merged commit 2acbe84 into cryostatio:main Mar 30, 2023

tthvo deleted the creds-test-table branch March 30, 2023 19:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cred): credential test table #920

feat(cred): credential test table #920

tthvo commented Mar 22, 2023 •

edited by andrewazores

Loading

github-actions bot commented Mar 22, 2023

github-actions bot commented Mar 23, 2023

github-actions bot commented Mar 23, 2023

github-actions bot commented Mar 24, 2023

github-actions bot commented Mar 24, 2023

github-actions bot commented Mar 24, 2023

github-actions bot commented Mar 24, 2023

maxcao13 commented Mar 24, 2023

tthvo commented Mar 24, 2023 •

edited

Loading

maxcao13 commented Mar 24, 2023 •

edited

Loading

maxcao13 commented Mar 24, 2023 •

edited

Loading

tthvo commented Mar 24, 2023 •

edited

Loading

github-actions bot commented Mar 24, 2023

tthvo commented Mar 24, 2023

github-actions bot commented Mar 24, 2023

andrewazores commented Mar 26, 2023 •

edited

Loading

github-actions bot commented Mar 27, 2023

github-actions bot commented Mar 28, 2023

github-actions bot commented Mar 28, 2023

github-actions bot commented Mar 29, 2023

github-actions bot commented Mar 29, 2023

github-actions bot commented Mar 29, 2023

maxcao13 commented Mar 29, 2023

tthvo commented Mar 29, 2023 •

edited

Loading

github-actions bot commented Mar 29, 2023

maxcao13 left a comment

andrewazores left a comment

feat(cred): credential test table #920

feat(cred): credential test table #920

Conversation

tthvo commented Mar 22, 2023 • edited by andrewazores Loading

Welcome to Cryostat! 👋

Before contributing, make sure you have:

Description of the change:

Motivation for the change:

How to manually test:

Screenshots

github-actions bot commented Mar 22, 2023

github-actions bot commented Mar 23, 2023

github-actions bot commented Mar 23, 2023

github-actions bot commented Mar 24, 2023

github-actions bot commented Mar 24, 2023

github-actions bot commented Mar 24, 2023

github-actions bot commented Mar 24, 2023

maxcao13 commented Mar 24, 2023

tthvo commented Mar 24, 2023 • edited Loading

maxcao13 commented Mar 24, 2023 • edited Loading

maxcao13 commented Mar 24, 2023 • edited Loading

tthvo commented Mar 24, 2023 • edited Loading

github-actions bot commented Mar 24, 2023

tthvo commented Mar 24, 2023

github-actions bot commented Mar 24, 2023

andrewazores commented Mar 26, 2023 • edited Loading

github-actions bot commented Mar 27, 2023

github-actions bot commented Mar 28, 2023

github-actions bot commented Mar 28, 2023

github-actions bot commented Mar 29, 2023

github-actions bot commented Mar 29, 2023

github-actions bot commented Mar 29, 2023

maxcao13 commented Mar 29, 2023

tthvo commented Mar 29, 2023 • edited Loading

github-actions bot commented Mar 29, 2023

maxcao13 left a comment

Choose a reason for hiding this comment

andrewazores left a comment

Choose a reason for hiding this comment

tthvo commented Mar 22, 2023 •

edited by andrewazores

Loading

tthvo commented Mar 24, 2023 •

edited

Loading

maxcao13 commented Mar 24, 2023 •

edited

Loading

maxcao13 commented Mar 24, 2023 •

edited

Loading

tthvo commented Mar 24, 2023 •

edited

Loading

andrewazores commented Mar 26, 2023 •

edited

Loading

tthvo commented Mar 29, 2023 •

edited

Loading