Add ability to hide token in cli tools page #871

juanvallejo · 2016-11-14T21:12:24Z

Related Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1388770
Fixes openshift/origin#11975

jwforres · 2016-11-14T21:13:42Z

@spadgett mind taking this one?

spadgett · 2016-11-14T21:21:30Z

app/views/command-line.html

@@ -48,7 +48,7 @@ <h1 id="cli">Command Line Tools</h1>
                  <div ng-show="showSessionToken" class="alert alert-warning">
                    <span class="pficon pficon-warning-triangle-o" aria-hidden="true"></span>
                    <strong>A token is a form of a password.</strong>
-                    Do not share your API token.
+                    Do not share your API token. <a href="#" ng-click="toggleShowSessionToken()" ng-show="showSessionToken">Click to hide the token...</a>


I'd probably just say "Hide token".

You don't need the ng-show here since it's on the parent div.

You only need href="". (Do you mind fixing the reveal href above as well?)

spadgett · 2016-11-14T21:25:31Z

@jwforres Opinion on a copy to clipboard link (without needing to reveal the token)? Right now I have to click show token and then manually select the command.

jwforres · 2016-11-14T21:31:30Z

wouldn't be opposed to the idea, would need to see how it looks in context

On Mon, Nov 14, 2016 at 4:25 PM, Sam Padgett [email protected]
wrote:

@jwforres https://github.com/jwforres Opinion on a copy to clipboard
link (without needing to reveal the token)? Right now I have to click show
token and then manually select the command.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#871 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABZk7Tuz-Jcc1khPePpZw_-dq32STlbbks5q-NHMgaJpZM4Kx0RE
.

juanvallejo · 2016-11-15T15:19:38Z

@jwforres @spadgett

spadgett · 2016-11-15T15:28:14Z

@juanvallejo +1, except I would copy the full command. Something like...?

<pre>oc login https://localhost:8443 --token=&lt;hidden&gt;</pre>
<a href="">Show token</a>
<span class="action-divider">|</span>
<a href="">Copy command to clipboard</a>

If we made that change, we'd probably always show the warning about tokens. And might want to avoid the copy to clipboard link for browsers that don't support it like iOS Safari.

juanvallejo · 2016-11-15T15:35:15Z

@spadgett Sounds good, I'll update this.
Thanks for the feedback, addressed review comments on #871 (review) . I tried updating the reveal href to leave ng-show to its parent, however its sibling span has ng-show set to an opposite value:

<div class="code prettyprint ng-binding" ng-if="sessionToken">
  oc login {{loginBaseURL}} --token=<span ng-show="showSessionToken">{{sessionToken}}</span><a href="#" ng-click="toggleShowSessionToken()" ng-show="!showSessionToken">...click to show token...</a>
</div>

juanvallejo · 2016-11-15T22:29:16Z

@spadgett

Show token is clicked

Copy to clipboard is clicked

EDIT: Result from copying command oc login https://localhost:8443 --token=KXo2gh27B_V5_k_otUvTxKU8H6gJogJtWdZqtU__OGo

spadgett

Thanks @juanvallejo, I think this is better than what we had before. A few comments.

spadgett · 2016-11-16T16:35:28Z

app/scripts/controllers/commandLine.js


    $scope.toggleShowSessionToken = function() {
      $scope.showSessionToken = !$scope.showSessionToken;
    };
+
+    $scope.copySessionToken = function($scope) {
+      var node = $('#' + $scope.copySessionTokenId);


I suggest we update the copy-to-clipboard directive to take in an optional element ID or create a new directive we could reuse (scripts/directives/util.js).

spadgett · 2016-11-16T16:36:20Z

app/views/command-line.html

@@ -48,7 +53,7 @@ <h1 id="cli">Command Line Tools</h1>
                  <div ng-show="showSessionToken" class="alert alert-warning">


Might want to show to show this always since I won't see the warning if I click copy to clipboard.

spadgett · 2016-11-16T16:37:22Z

app/views/command-line.html

+                      <span ng-show="!showSessionToken">
+                        <a href="#" ng-click="toggleShowSessionToken()">Show token</a>
+                        <span class="action-divider">|</span>
+                        <a href="#" id="{{copySessionTokenId}}" ng-click="copySessionToken(this)" title="Copy command to clipboard" data-toggle="tooltip">Copy command to clipboard</a>


Hide this if it's iOS? There's an IS_IOS constant you can use in the controller to set a scope var.

How do the actions look if you move them below the prettyprint div?

spadgett · 2016-11-16T16:38:03Z

app/views/command-line.html

@@ -48,7 +53,7 @@ <h1 id="cli">Command Line Tools</h1>
                  <div ng-show="showSessionToken" class="alert alert-warning">
                    <span class="pficon pficon-warning-triangle-o" aria-hidden="true"></span>
                    <strong>A token is a form of a password.</strong>
-                    Do not share your API token.
+                    Do not share your API token. <a href="#" ng-click="toggleShowSessionToken()">Hide token.</a>


Might want to change the show link to a hide link and just have it toggle.

juanvallejo · 2016-11-16T22:13:15Z

@spadgett Review comments addressed, I'm now re-using the <copy-to-clipboard directive

spadgett · 2016-11-17T17:45:56Z

LGTM, @jwforres OK with this change?

spadgett · 2016-11-17T17:46:19Z

@sspeiche FYI

jwforres · 2016-11-17T17:47:45Z

yep, @spadgett were you going to do the code review on this

spadgett · 2016-11-17T17:55:38Z

were you going to do the code review on this

Yeah, code looks good. Thanks @juanvallejo

spadgett · 2016-11-17T17:56:50Z

hold, spotted one problem

spadgett · 2016-11-17T17:57:57Z

app/views/command-line.html

@@ -38,14 +38,22 @@ <h1 id="cli">Command Line Tools</h1>
                  <p>
                    After downloading and installing it, you can start by logging in using<span ng-if="sessionToken"> this current session token</span>:
                    <div class="code prettyprint ng-binding" ng-if="sessionToken">


@juanvallejo need an outer div with ng-if="sessionToken" surrounding both the prettyprint div and the action links below.

sspeiche · 2016-11-17T18:07:45Z

Why would this "copy to clipboard" not be the same icon for webhooks ?

juanvallejo · 2016-11-17T18:40:37Z

@spadgett Thanks, review comment addressed

@sspeiche

Why would this "copy to clipboard" not be the same icon for webhooks ?

Would that be the

icon? I did not think it would fit with this page, as I have only seen it used when copying contents from an <input element

sspeiche · 2016-11-17T18:42:09Z

It looks like something like this:

spadgett · 2016-11-17T18:52:15Z

@sspeiche are you proposing to use the entire input field or just the icon?

sspeiche · 2016-11-17T19:03:20Z

I'm proposing that everywhere in the web console that we "copy to clipboard" it doesn't do something that looks and feels that different.

spadgett · 2016-11-17T19:16:05Z

I agree consistency is good, but we're being careful not to show the token value. Maybe the copy-to-clipboard input is not wide enough to reveal it. I'm not sure we should count on that, though.

jwforres · 2016-11-18T17:23:35Z

if we are going to do that, we should just switch all of these code blocks on this page to copy to clipboard elements instead

@juanvallejo mind getting a screenshot of what that would look like?

juanvallejo · 2016-11-18T17:51:08Z

@jwforres

if we are going to do that, we should just switch all of these code blocks on this page to copy to clipboard elements instead

Sure, I'll put an example together

juanvallejo · 2016-11-18T19:08:23Z

@jwforres @spadgett @sspeiche

sspeiche · 2016-11-18T20:25:39Z

Fantastic, shouldn't the input fields be wider?

jwforres · 2016-11-18T21:09:05Z

Making the input fields wider would cause the token to be visible, which is
bad. We have them short because they are often obscuring things later in
the input, like with webhooks they are obscuring the secrets in the URL

On Fri, Nov 18, 2016 at 3:25 PM, Steve Speicher [email protected]
wrote:

Fantastic, shouldn't the input fields be wider?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#871 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABZk7QAPLLceF7h2dZ06iFnwQkG9m10Yks5q_gnEgaJpZM4Kx0RE
.

jwforres · 2016-11-18T21:10:21Z

That said, I'm fine with this change. @spadgett ?

On Fri, Nov 18, 2016 at 4:09 PM, Jessica Forrester [email protected]
wrote:

Making the input fields wider would cause the token to be visible, which
is bad. We have them short because they are often obscuring things later
in the input, like with webhooks they are obscuring the secrets in the URL

On Fri, Nov 18, 2016 at 3:25 PM, Steve Speicher [email protected]
wrote:

Fantastic, shouldn't the input fields be wider?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#871 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABZk7QAPLLceF7h2dZ06iFnwQkG9m10Yks5q_gnEgaJpZM4Kx0RE
.

spadgett · 2016-11-18T21:13:19Z

Does it make sense to copy commands with placeholder values like <project-name>? (Maybe?)

spadgett · 2016-11-18T21:15:11Z

Making the input fields wider would cause the token to be visible, which is bad.

We might be able to still show token=<hidden> in the input and copy a different value, although we'd need to add some additional logic to the copy-to-clipboard directive. I'd want to check how 100% width looks in other places we use it since it's a global style.

juanvallejo · 2016-11-18T21:16:54Z

@spadgett I can add a wide option to the directive so that full width does not apply anywhere else. I'll put together an example with this and <hidden> values

juanvallejo · 2016-11-18T21:52:39Z

@spadgett @jwforres @sspeiche

Clipboard contents from copying first input oc login https://localhost:8443 --token=YGvpJBROMXDaY_uO8XZuPKRMh3TKycqGcK9IkBXnxzE

(these changes have not been committed yet)

juanvallejo · 2016-11-21T15:37:11Z

@spadgett added commit with copy-to-clipboard changes, PTAL

spadgett · 2016-11-21T16:02:59Z

app/views/directives/_copy-to-clipboard.html

@@ -1,11 +1,17 @@
-<div class="input-group copy-to-clipboard">
-  <input id="{{id}}" type="text" class="form-control" value="{{clipboardText}}" ng-disabled="isDisabled" ng-readonly="!isDisabled" select-on-focus>
+<div class="input-group copy-to-clipboard" style="{{displayWide ? 'max-width:100%;' : ''}}">


Suggest something like

ng-class="{ 'limit-width': !displayWide }"

then update the CSS to key off that class.

https://github.com/spadgett/origin-web-console/blob/e01508561bb7d0c7f6c53dbedcf7dd42d37e0c52/app/styles/_core.less#L1208-L1210

spadgett · 2016-11-21T16:05:12Z

app/views/directives/_copy-to-clipboard.html

  <span class="input-group-btn" ng-hide="hidden">
-    <a data-clipboard-target="#{{id}}"
+    <a ng-show="!inputText" data-clipboard-target="#{{id}}"


If you use ng-if here and line 10, does it let you remove the get(0) / get(1) special case in the controller?

If I use ng-if, I receive a console error about the element not being found (this happens on this page, as well as every other page that uses this directive)

Ah, OK. We could wrap the controller code in a $timeout to let the DOM render first, but what you have is probably better.

juanvallejo · 2016-11-21T20:42:00Z

@spadgett thanks for the feedback, review comments addressed

spadgett · 2016-11-21T21:27:54Z

thanks @juanvallejo

LGTM, please squash, and I'll merge

juanvallejo · 2016-11-21T21:38:07Z

@spadgett

LGTM, please squash, and I'll merge

Thanks, done!

spadgett

thanks @juanvallejo !

spadgett · 2016-11-21T21:41:28Z

[merge]

openshift-bot · 2016-11-21T21:47:16Z

Evaluated for origin web console merge up to 0b99a35

openshift-bot · 2016-11-21T21:47:17Z

[Test]ing while waiting on the merge queue

openshift-bot · 2016-11-21T21:55:16Z

Evaluated for origin web console test up to 0b99a35

openshift-bot · 2016-11-21T22:11:06Z

Origin Web Console Test Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/748/) (Base Commit: a60cd15)

openshift-bot · 2016-11-21T23:23:15Z

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/751/) (Base Commit: c5349e7)

spadgett self-assigned this Nov 14, 2016

spadgett reviewed Nov 14, 2016

View reviewed changes

juanvallejo force-pushed the jvallejo/add-option-to-hide-token-cli-tools-page branch from 265d154 to 6694fce Compare November 15, 2016 15:29

spadgett reviewed Nov 16, 2016

View reviewed changes

spadgett approved these changes Nov 17, 2016

View reviewed changes

spadgett requested changes Nov 17, 2016

View reviewed changes

spadgett mentioned this pull request Nov 21, 2016

Add "Copy to Clipboard" to the "Command Line Tools" page openshift/origin#11975

Closed

juanvallejo force-pushed the jvallejo/add-option-to-hide-token-cli-tools-page branch from 00dd20a to f2e4fda Compare November 21, 2016 15:44

spadgett reviewed Nov 21, 2016

View reviewed changes

Add ability to copy commands in cli tools page

0b99a35

juanvallejo force-pushed the jvallejo/add-option-to-hide-token-cli-tools-page branch from 44eb014 to 0b99a35 Compare November 21, 2016 21:33

spadgett approved these changes Nov 21, 2016

View reviewed changes

openshift-bot merged commit 7389ce5 into openshift:master Nov 21, 2016

juanvallejo deleted the jvallejo/add-option-to-hide-token-cli-tools-page branch November 22, 2016 14:32

		@@ -48,7 +53,7 @@ <h1 id="cli">Command Line Tools</h1>
		<div ng-show="showSessionToken" class="alert alert-warning">

Add ability to hide token in cli tools page #871

Add ability to hide token in cli tools page #871

Conversation

juanvallejo commented Nov 14, 2016 • edited by spadgett Loading

jwforres commented Nov 14, 2016

Choose a reason for hiding this comment

spadgett commented Nov 14, 2016

jwforres commented Nov 14, 2016

juanvallejo commented Nov 15, 2016

spadgett commented Nov 15, 2016

juanvallejo commented Nov 15, 2016 • edited Loading

juanvallejo commented Nov 15, 2016 • edited Loading

spadgett left a comment

Choose a reason for hiding this comment

spadgett Nov 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanvallejo commented Nov 16, 2016 • edited Loading

spadgett commented Nov 17, 2016

spadgett commented Nov 17, 2016

jwforres commented Nov 17, 2016

spadgett commented Nov 17, 2016

spadgett commented Nov 17, 2016

Choose a reason for hiding this comment

sspeiche commented Nov 17, 2016

juanvallejo commented Nov 17, 2016

sspeiche commented Nov 17, 2016

spadgett commented Nov 17, 2016

sspeiche commented Nov 17, 2016

spadgett commented Nov 17, 2016 • edited Loading

jwforres commented Nov 18, 2016

juanvallejo commented Nov 18, 2016

juanvallejo commented Nov 18, 2016

sspeiche commented Nov 18, 2016

jwforres commented Nov 18, 2016

jwforres commented Nov 18, 2016

spadgett commented Nov 18, 2016

spadgett commented Nov 18, 2016

juanvallejo commented Nov 18, 2016

juanvallejo commented Nov 18, 2016 • edited Loading

juanvallejo commented Nov 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanvallejo commented Nov 21, 2016

spadgett commented Nov 21, 2016

juanvallejo commented Nov 21, 2016

spadgett left a comment

Choose a reason for hiding this comment

spadgett commented Nov 21, 2016

openshift-bot commented Nov 21, 2016

openshift-bot commented Nov 21, 2016

openshift-bot commented Nov 21, 2016

openshift-bot commented Nov 21, 2016

openshift-bot commented Nov 21, 2016 • edited Loading

juanvallejo commented Nov 14, 2016 •

edited by spadgett

Loading

juanvallejo commented Nov 15, 2016 •

edited

Loading

juanvallejo commented Nov 15, 2016 •

edited

Loading

spadgett Nov 16, 2016 •

edited

Loading

juanvallejo commented Nov 16, 2016 •

edited

Loading

spadgett commented Nov 17, 2016 •

edited

Loading

juanvallejo commented Nov 18, 2016 •

edited

Loading

openshift-bot commented Nov 21, 2016 •

edited

Loading