implementation of WFLY-2510 #89

Merged
merged 9 commits into from Sep 2, 2014

Conversation

Projects
None yet
6 participants
@claudio4j
Contributor

claudio4j commented Aug 10, 2014

Add CLI command to display information about current connection.

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Aug 11, 2014

Can one of the admins verify this patch?

Can one of the admins verify this patch?

@ctomc

This comment has been minimized.

Show comment
Hide comment
@ctomc

ctomc Aug 13, 2014

Contributor

this is ok to test

Contributor

ctomc commented Aug 13, 2014

this is ok to test

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Aug 13, 2014

Build 184 is now running using a merge of b60f867

Build 184 is now running using a merge of b60f867

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Aug 13, 2014

Build 184 outcome was SUCCESS using a merge of b60f867
Summary: Tests passed: 2552, ignored: 57 Build time: 0:15:39

Build 184 outcome was SUCCESS using a merge of b60f867
Summary: Tests passed: 2552, ignored: 57 Build time: 0:15:39

@bstansberry

This comment has been minimized.

Show comment
Hide comment
@bstansberry

bstansberry Aug 14, 2014

Contributor

@darranl @aloubyansky please approve

Contributor

bstansberry commented Aug 14, 2014

@darranl @aloubyansky please approve

+ *
+ * @author Claudio Miranda
+ */
+public class ConnectionInfoHandler implements CommandHandler {

This comment has been minimized.

@aloubyansky

aloubyansky Aug 14, 2014

Contributor

It would be better to extend CommandHandlerWithHelp and write a simple txt file with the description of the command or implement --help/-h argument otherwise.

@aloubyansky

aloubyansky Aug 14, 2014

Contributor

It would be better to extend CommandHandlerWithHelp and write a simple txt file with the description of the command or implement --help/-h argument otherwise.

+ */
+ @Override
+ public boolean isBatchMode(CommandContext ctx) {
+ return true;

This comment has been minimized.

@aloubyansky

aloubyansky Aug 14, 2014

Contributor

This one must return false.

@aloubyansky

aloubyansky Aug 14, 2014

Contributor

This one must return false.

+ final StringBuilder buf = new StringBuilder();
+ final ModelControllerClient client = ctx.getModelControllerClient();
+ if(client == null) {
+ buf.append("<connect to the controller and re-run the connection-info command to see the connection information>\n");

This comment has been minimized.

@aloubyansky

aloubyansky Aug 14, 2014

Contributor

Not an important one, perhaps, you have a reason to use '<' and '>' but we don't have any convention for that, they are not used anywhere else for formatting. So, this one might stand out.

@aloubyansky

aloubyansky Aug 14, 2014

Contributor

Not an important one, perhaps, you have a reason to use '<' and '>' but we don't have any convention for that, they are not used anywhere else for formatting. So, this one might stand out.

This comment has been minimized.

@claudio4j

claudio4j Aug 14, 2014

Contributor

I thought to maintain the same message from VersionHandler, as it requires to be connected to the server. If needed I can remove the < and > .

@claudio4j

claudio4j Aug 14, 2014

Contributor

I thought to maintain the same message from VersionHandler, as it requires to be connected to the server. If needed I can remove the < and > .

+ boolean disableLocalAuth = (boolean) ctx.get("disableLocalAuth");
+ String username = "Local connection authenticated as SuperUser";
+ if (disableLocalAuth)
+ username = (String) ctx.get("username");

This comment has been minimized.

@aloubyansky

aloubyansky Aug 14, 2014

Contributor

Do you think it would make sense to create a ConnectionInfo JavaBean (perhaps an immutable one with a protected ctor) which would carry all the related info and store it under a single key instead of a key per property?
A thing to keep in mind is this ctx.get(...)/set(...) is a public API. Theoretically, anybody having access to ctx can manipulate the data. In general, these methods should be avoided whenever possible.

@aloubyansky

aloubyansky Aug 14, 2014

Contributor

Do you think it would make sense to create a ConnectionInfo JavaBean (perhaps an immutable one with a protected ctor) which would carry all the related info and store it under a single key instead of a key per property?
A thing to keep in mind is this ctx.get(...)/set(...) is a public API. Theoretically, anybody having access to ctx can manipulate the data. In general, these methods should be avoided whenever possible.

This comment has been minimized.

@claudio4j

claudio4j Aug 14, 2014

Contributor

I thought about using a javabean, but as there are only four attributes, perhaps was not worth. But as you suggest it, I will do it.

@claudio4j

claudio4j Aug 14, 2014

Contributor

I thought about using a javabean, but as there are only four attributes, perhaps was not worth. But as you suggest it, I will do it.

This comment has been minimized.

@claudio4j

claudio4j Aug 14, 2014

Contributor

Looking at the implementation detail, there is not a single place in CommandContextImpl to call the protected constructor, because the set(key) is called in different places, so I need to add the mutable methods in the javabean. Is that ok ?

@claudio4j

claudio4j Aug 14, 2014

Contributor

Looking at the implementation detail, there is not a single place in CommandContextImpl to call the protected constructor, because the set(key) is called in different places, so I need to add the mutable methods in the javabean. Is that ok ?

+ for (String alg : fingerprints.keySet()) {
+ String algName = String.format("%-13s", alg);
+ buf.append(algName + "- " + fingerprints.get(alg)).append("\n");
+ }

This comment has been minimized.

@aloubyansky

aloubyansky Aug 14, 2014

Contributor

BTW, I have a couple of simple table implementations (org.jboss.as.cli.util.SimpleTable and StrictSizeTable) where you can add rows and columns (with or without headers) and then format them into a StringBuffer (it'll use column width based on the values provided) and optionally ordering the rows. They are primitive API though. Just in case.

@aloubyansky

aloubyansky Aug 14, 2014

Contributor

BTW, I have a couple of simple table implementations (org.jboss.as.cli.util.SimpleTable and StrictSizeTable) where you can add rows and columns (with or without headers) and then format them into a StringBuffer (it'll use column width based on the values provided) and optionally ordering the rows. They are primitive API though. Just in case.

+ public List<CommandArgument> getArguments(CommandContext ctx) {
+ return Collections.emptyList();
+ }
+}

This comment has been minimized.

@aloubyansky

aloubyansky Aug 14, 2014

Contributor

A simple test perhaps? For the ssl one the output will definitely be verbose and formatted, so not that trivial to setup and validate. Do we want a simple one which makes sure the command is available and tests disconnected and non-ssl connection?
Thanks!

@aloubyansky

aloubyansky Aug 14, 2014

Contributor

A simple test perhaps? For the ssl one the output will definitely be verbose and formatted, so not that trivial to setup and validate. Do we want a simple one which makes sure the command is available and tests disconnected and non-ssl connection?
Thanks!

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Aug 15, 2014

Build 201 is now running using a merge of 18e143f

Build 201 is now running using a merge of 18e143f

@claudio4j

This comment has been minimized.

Show comment
Hide comment
@claudio4j

claudio4j Aug 15, 2014

Contributor

Hi, take a look, thanks.

Contributor

claudio4j commented Aug 15, 2014

Hi, take a look, thanks.

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Aug 15, 2014

Build 201 outcome was FAILURE using a merge of 18e143f
Summary: Tests passed: 944, ignored: 6; exit code 1 (new) Build time: 0:01:43

Build problems:

Process exited with code 1

Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.1:compile (default-compile) on project wildfly-cli: Compilation failure
Step Build & test (Maven) failed

Build 201 outcome was FAILURE using a merge of 18e143f
Summary: Tests passed: 944, ignored: 6; exit code 1 (new) Build time: 0:01:43

Build problems:

Process exited with code 1

Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.1:compile (default-compile) on project wildfly-cli: Compilation failure
Step Build & test (Maven) failed
@claudio4j

This comment has been minimized.

Show comment
Hide comment
@claudio4j

claudio4j Aug 15, 2014

Contributor

Ah, had some issues with git merge and commit, sorry.

Contributor

claudio4j commented Aug 15, 2014

Ah, had some issues with git merge and commit, sorry.

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Aug 15, 2014

Build 202 is now running using a merge of 8e7181d

Build 202 is now running using a merge of 8e7181d

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Aug 15, 2014

Build 202 outcome was SUCCESS using a merge of 8e7181d
Summary: Tests passed: 2561, ignored: 57 Build time: 0:15:43

Build 202 outcome was SUCCESS using a merge of 8e7181d
Summary: Tests passed: 2561, ignored: 57 Build time: 0:15:43

+import java.security.cert.X509Certificate;
+import java.util.Date;
+
+public class ConnectionInfoBean {

This comment has been minimized.

@aloubyansky

aloubyansky Aug 15, 2014

Contributor

Looks good. I meant to keep the class public but the constructor and the setters could have the default access modifier, so they are not visible outside the package.

@aloubyansky

aloubyansky Aug 15, 2014

Contributor

Looks good. I meant to keep the class public but the constructor and the setters could have the default access modifier, so they are not visible outside the package.

This comment has been minimized.

@aloubyansky

aloubyansky Aug 15, 2014

Contributor

Actually, if you don't mind, let's introduce a ConnectionInfo interface (which will expose only the get-methods) and simply add a method to CommandContext, e.g. ConnectionInfo getConnectionInfo(). Thanks!

@aloubyansky

aloubyansky Aug 15, 2014

Contributor

Actually, if you don't mind, let's introduce a ConnectionInfo interface (which will expose only the get-methods) and simply add a method to CommandContext, e.g. ConnectionInfo getConnectionInfo(). Thanks!

@claudio4j

This comment has been minimized.

Show comment
Hide comment
@claudio4j

claudio4j Aug 15, 2014

Contributor

Hi, take a look now. Thanks.

Contributor

claudio4j commented Aug 15, 2014

Hi, take a look now. Thanks.

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Aug 15, 2014

Build 204 is now running using a merge of 244b7e6

Build 204 is now running using a merge of 244b7e6

+
+ public ConnectionInfo getConnectionInfo() {
+ return connInfoBean;
+ }

This comment has been minimized.

@aloubyansky

aloubyansky Aug 15, 2014

Contributor

Yes, this looks better now. The final thing though... :) Sorry, I missed it before. There is a method disconnectController() in this class. This method could be called by a user or if the cli receives a connection closed notification. Anyway, to keep the connection info consistent it has to be reset in this method too.
Thanks a lot!

@aloubyansky

aloubyansky Aug 15, 2014

Contributor

Yes, this looks better now. The final thing though... :) Sorry, I missed it before. There is a method disconnectController() in this class. This method could be called by a user or if the cli receives a connection closed notification. Anyway, to keep the connection info consistent it has to be reset in this method too.
Thanks a lot!

This comment has been minimized.

@claudio4j

claudio4j Aug 15, 2014

Contributor

Done!

@claudio4j

claudio4j Aug 15, 2014

Contributor

Done!

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Aug 15, 2014

Build 204 outcome was SUCCESS using a merge of 244b7e6
Summary: Tests passed: 2561, ignored: 57 Build time: 0:15:46

Build 204 outcome was SUCCESS using a merge of 244b7e6
Summary: Tests passed: 2561, ignored: 57 Build time: 0:15:46

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Aug 15, 2014

Build 205 is now running using a merge of abb24a7

Build 205 is now running using a merge of abb24a7

@@ -837,6 +843,8 @@ public void connectController(String controller) throws CommandLineException {
do {
try {
CallbackHandler cbh = new AuthenticationCallbackHandler(username, password);
+ connInfoBean = new ConnectionInfoBean();
+ connInfoBean.setDisableLocalAuth(disableLocalAuth);

This comment has been minimized.

@aloubyansky

aloubyansky Aug 15, 2014

Contributor

What if the connection actually fails? I think in that case you should reset the just initialized connection info in the catches below.

@aloubyansky

aloubyansky Aug 15, 2014

Contributor

What if the connection actually fails? I think in that case you should reset the just initialized connection info in the catches below.

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Aug 15, 2014

Build 205 outcome was SUCCESS using a merge of abb24a7
Summary: Tests passed: 2562, ignored: 57 Build time: 0:15:40

Build 205 outcome was SUCCESS using a merge of abb24a7
Summary: Tests passed: 2562, ignored: 57 Build time: 0:15:40

@@ -0,0 +1,56 @@
+/*
+ * JBoss, Home of Professional Open Source.
+ * Copyright 2011, Red Hat, Inc., and individual contributors

This comment has been minimized.

@darranl

darranl Aug 18, 2014

Contributor

It is now 2014 - all new classes should reflect this in the copyright header.

@darranl

darranl Aug 18, 2014

Contributor

It is now 2014 - all new classes should reflect this in the copyright header.

+
+ ConnectionInfo connInfo = ctx.getConnectionInfo();
+ boolean disableLocalAuth = connInfo.isDisableLocalAuth();
+ String username = "Local connection authenticated as SuperUser";

This comment has been minimized.

@darranl

darranl Aug 18, 2014

Contributor

For this block rather than client side assumptions this information should be obtained by invoking the whoami operation on the server to identify the authenticated user name. SuperUser is a name of a role not a username and local authentication is not automatically granted this role - that is just the default configuration.

@darranl

darranl Aug 18, 2014

Contributor

For this block rather than client side assumptions this information should be obtained by invoking the whoami operation on the server to identify the authenticated user name. SuperUser is a name of a role not a username and local authentication is not automatically granted this role - that is just the default configuration.

This comment has been minimized.

@claudio4j

claudio4j Aug 19, 2014

Contributor

Should it print username = $local ?
I thought about using a description because the "$local" is not intuitive about the current user.

:whoami
{
"outcome" => "success",
"result" => {"identity" => {
"username" => "$local",
"realm" => "ManagementRealm"
}}
}

@claudio4j

claudio4j Aug 19, 2014

Contributor

Should it print username = $local ?
I thought about using a description because the "$local" is not intuitive about the current user.

:whoami
{
"outcome" => "success",
"result" => {"identity" => {
"username" => "$local",
"realm" => "ManagementRealm"
}}
}

This comment has been minimized.

@darranl

darranl Aug 19, 2014

Contributor

I would say yes '$local' should be used, if you call :whoami with verbose=true you can also identify the role(s) they are assigned and output that as well.

The $local user does have a meaning that we can document.

@darranl

darranl Aug 19, 2014

Contributor

I would say yes '$local' should be used, if you call :whoami with verbose=true you can also identify the role(s) they are assigned and output that as well.

The $local user does have a meaning that we can document.

This comment has been minimized.

@claudio4j

claudio4j Aug 20, 2014

Contributor

See the result, with rbac enabled

standalone@localhost:9993 /] connection-info
Username $local, authenticated as ["SuperUser"]

[standalone@localhost:9993 /] connection-info
Username claudio, authenticated as ["Monitor","Deployer"]

@claudio4j

claudio4j Aug 20, 2014

Contributor

See the result, with rbac enabled

standalone@localhost:9993 /] connection-info
Username $local, authenticated as ["SuperUser"]

[standalone@localhost:9993 /] connection-info
Username claudio, authenticated as ["Monitor","Deployer"]

+ st.addLine(new String[]{"Username", username});
+ st.addLine(new String[]{"Logged since", connInfo.getLoggedSince().toString()});
+ X509Certificate[] lastChain = connInfo.getServerCertificates();
+ boolean sslConn = lastChain != null;

This comment has been minimized.

@darranl

darranl Aug 18, 2014

Contributor

For now this is the best way to obtain this, however FYI JBoss Remoting is about to be updated so we can pull the SSLSession from the Connection - that way we can be sure we are displaying the information from the connection rather than a previously cached value.

@darranl

darranl Aug 18, 2014

Contributor

For now this is the best way to obtain this, however FYI JBoss Remoting is about to be updated so we can pull the SSLSession from the Connection - that way we can be sure we are displaying the information from the connection rather than a previously cached value.

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Aug 20, 2014

Build 227 is now running using a merge of e217c59

Build 227 is now running using a merge of e217c59

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Aug 20, 2014

Build 229 is now running using a merge of d529421

Build 229 is now running using a merge of d529421

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Aug 20, 2014

Build 227 outcome was SUCCESS using a merge of d529421
Summary: Tests passed: 2541, ignored: 56 Build time: 0:16:04

Build 227 outcome was SUCCESS using a merge of d529421
Summary: Tests passed: 2541, ignored: 56 Build time: 0:16:04

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Aug 20, 2014

Build 229 outcome was SUCCESS using a merge of d529421
Summary: Tests passed: 2541, ignored: 56 Build time: 0:16:09

Build 229 outcome was SUCCESS using a merge of d529421
Summary: Tests passed: 2541, ignored: 56 Build time: 0:16:09

@darranl

This comment has been minimized.

Show comment
Hide comment
@darranl

darranl Aug 20, 2014

Contributor

Sorry if you wouldn't mind I would suggest instead of authenticated as it may be better to say 'granted role' and 'granted roles'.

Contributor

darranl commented Aug 20, 2014

Sorry if you wouldn't mind I would suggest instead of authenticated as it may be better to say 'granted role' and 'granted roles'.

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Aug 21, 2014

Build 237 is now running using a merge of c8f7959

Build 237 is now running using a merge of c8f7959

@claudio4j

This comment has been minimized.

Show comment
Hide comment
@claudio4j

claudio4j Aug 21, 2014

Contributor

I also added the message when there is no role associated when using rbac

[standalone@localhost:9993 /] connection-info
Username paty, has no role associated.

Contributor

claudio4j commented Aug 21, 2014

I also added the message when there is no role associated when using rbac

[standalone@localhost:9993 /] connection-info
Username paty, has no role associated.

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Aug 21, 2014

Build 237 outcome was SUCCESS using a merge of c8f7959
Summary: Tests passed: 2541, ignored: 56 Build time: 0:15:31

Build 237 outcome was SUCCESS using a merge of c8f7959
Summary: Tests passed: 2541, ignored: 56 Build time: 0:15:31

@claudio4j

This comment has been minimized.

Show comment
Hide comment
@claudio4j

claudio4j Aug 26, 2014

Contributor

Hi @darranl, I think all remaining issues are fixed.

Contributor

claudio4j commented Aug 26, 2014

Hi @darranl, I think all remaining issues are fixed.

@bstansberry

This comment has been minimized.

Show comment
Hide comment
@bstansberry

bstansberry Aug 28, 2014

Contributor

@darranl @aloubyansky since there's been a fair bit of back and forth on this one, please give it a final +1 so I know all issues are addressed and can merge it.

Contributor

bstansberry commented Aug 28, 2014

@darranl @aloubyansky since there's been a fair bit of back and forth on this one, please give it a final +1 so I know all issues are addressed and can merge it.

@bstansberry

This comment has been minimized.

Show comment
Hide comment
@bstansberry

bstansberry Aug 28, 2014

Contributor

Claudio, thanks so much!

Before merging, I need to get an answer to a question:

We require that all contributions be made under the terms of the MIT License, http://www.opensource.org/licenses/mit-license.php. Do you agree to the these terms?

Contributor

bstansberry commented Aug 28, 2014

Claudio, thanks so much!

Before merging, I need to get an answer to a question:

We require that all contributions be made under the terms of the MIT License, http://www.opensource.org/licenses/mit-license.php. Do you agree to the these terms?

@aloubyansky

This comment has been minimized.

Show comment
Hide comment
@aloubyansky

aloubyansky Aug 28, 2014

Contributor

Yes and thanks a lot from me!

Contributor

aloubyansky commented Aug 28, 2014

Yes and thanks a lot from me!

@claudio4j

This comment has been minimized.

Show comment
Hide comment
@claudio4j

claudio4j Aug 28, 2014

Contributor

On Thu, Aug 28, 2014 at 8:53 AM, Brian Stansberry notifications@github.com
wrote:

We require that all contributions be made under the terms of the MIT
License, http://www.opensource.org/licenses/mit-license.php. Do you agree
to the these terms?

Yes.

Claudio Miranda

claudio@claudius.com.br
http://www.claudius.com.br

Contributor

claudio4j commented Aug 28, 2014

On Thu, Aug 28, 2014 at 8:53 AM, Brian Stansberry notifications@github.com
wrote:

We require that all contributions be made under the terms of the MIT
License, http://www.opensource.org/licenses/mit-license.php. Do you agree
to the these terms?

Yes.

Claudio Miranda

claudio@claudius.com.br
http://www.claudius.com.br

bstansberry added a commit that referenced this pull request Sep 2, 2014

Merge pull request #89 from claudio4j/WFLY-2510
implementation of WFLY-2510

@bstansberry bstansberry merged commit 859755d into wildfly:master Sep 2, 2014

1 check passed

default TeamCity Build WildFly Core :: Pull request - Linux finished: Tests passed: 2541, ignored: 56
Details
@bstansberry

This comment has been minimized.

Show comment
Hide comment
@bstansberry

bstansberry Sep 2, 2014

Contributor

Thanks!

Contributor

bstansberry commented Sep 2, 2014

Thanks!

iweiss pushed a commit to iweiss/wildfly-core that referenced this pull request Jun 6, 2016

Merge pull request #89 from bstansberry/JBEAP-2490
[JBEAP-2490][WFCORE-1240] Call the non-deprecated variant
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment