Add copy to clipboard buttons to display token page #200

vojtechszocs · 2025-10-29T19:38:47Z

This PR modifies the token display page for quicker access to token data.

HTTP GET now renders the tokenTemplate directly, i.e. without HTTP POST "Display Token" form submit.
Each snippet now includes a "Show" and "Copy" button.

Click "Show" to reveal the snippet. Click "Copy" to copy the snippet to clipboard.

JavaScript code is using standard Clipboard API which should be supported by all modern browsers.

pkg/server/tokenrequest/tokenrequest.go

spadgett

/lgtm

Thanks @vojtechszocs 👍

/hold
for @everettraven to review

vojtechszocs · 2025-11-04T15:34:57Z

/retest

everettraven

This seems fine to me. I don't think I have approval permissions. Will likely need @ibihim for approval.

/lgtm

ibihim · 2025-11-12T21:26:54Z

pkg/server/tokenrequest/tokenrequest.go

  <code>{{.AccessToken}}</code>

+  <script>
+    var commands = {


Maybe this is my old school JS, but isn't it better to have something like this namespaced approach:

var openshift = openshift || {}; openshift.commands = openshift.commands || {}; openshift.commands.login = '...'; openshift.commands.apiCall = '...';

or a IIFE that is completely isolated?

Before this PR, the token request page had no JavaScript so I'm not sure if using the openshift variable to namespace the commands is really needed.

I wanted to keep the changes at minimum so I went with a single commands variable to hold the strings to copy.

var commands = { foo: 'x' };

is functionally the same as

var commands = {}; commands.foo = 'x';

As for IIFE, these are typically used to avoid pollution of the global object namespace but I don't think we need them in our case.

The idea is to define some global object to store the strings to be referenced by the copy button's onclick handler.

I still think that it is always worth to introduce best practices, but nevertheless, this isn't a thing that I would keep blocking the PR.

ibihim · 2025-11-12T21:28:05Z

pkg/server/tokenrequest/tokenrequest.go

+    oc login
+    <span class="nowrap">--token={{.AccessToken}}</span>
+    <span class="nowrap">--server={{.PublicMasterURL}}</span>
+    <button onclick="navigator.clipboard.writeText(commands.login)">Copy</button>


Shouldn't we add some feedback for the user, like changing the button text to "Copied!" or so?

This would add more complexity, which is something I wanted to avoid.

The page template is missing standard HTML structure, including <!doctype html>, <head>, <body> etc. which cause browsers to render this page it in "quirks" mode.

I could try to add some custom-styled "Copied!" feedback text upon clicking the button, but do we really need it?

Implemented.

ibihim · 2025-11-12T21:28:54Z

pkg/server/tokenrequest/tokenrequest.go

+    oc login
+    <span class="nowrap">--token={{.AccessToken}}</span>
+    <span class="nowrap">--server={{.PublicMasterURL}}</span>
+    <button onclick="navigator.clipboard.writeText(commands.login)">Copy</button>


https://developer.mozilla.org/en-US/docs/Web/API/Clipboard/writeText

Isn't it best practice to handle the error somehow?

We just call the clipboard.writeText function without handling the resulting Promise.

This function is async and resolves when the clipboard's contents have been updated. If we had a "Copied!" feedback text implemented, we'd be handing the Promise, but currently we don't need that.

I'm not sure we need an explicit error handler here, unless we want to implement the feedback text, could be something like "Failed to copy the snippet to clipboard, see browser logs for details" or similar.

In practice, clipboard.writeText function could fail if the page was forbidden to copy text to clipboard via setting page permissions, e.g. for Chromium based browsers [1]:

Writing requires either the clipboard-write permission or transient activation.

[1] https://developer.mozilla.org/en-US/docs/Web/API/Clipboard_API#security_considerations

@vojtechszocs Maybe just throw up an alert on an error? Something like...? "Copy to clipboard failed. Select the content to copy it manually."

In practice, I'd expect clipboard write to almost never fail.

Implemented

openshift-ci · 2025-12-10T19:58:58Z

New changes are detected. LGTM label has been removed.

openshift-ci · 2025-12-11T00:25:14Z

@vojtechszocs: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

ibihim · 2025-12-19T12:27:50Z

I wish there would have been a more detailed explanation why it is fine to get rid of the intermediate CSRF step, but after bouncing my head for an hour on this topic, the worst thing I could imagine is that a disgruntled Alice could sneak in her tokens on your page.

ibihim · 2025-12-19T12:28:09Z

I will approve once @everettraven lgtm'ed it... or @liouk

everettraven

I'm concerned about us making a breaking change to the acceptable requests here - because it is out there, and has been out there for a while, I would not be surprised if there is some kind of end user workflow that depends on sending a properly formed POST request to display the token.

Additionally, do we have any kind of testing in place for UX/UI related changes we can update / put in place here?

If there is not existing testing infrastructure here it isn't the end of the world, but if it exists we should make sure the UX changes are appropriately tested.

everettraven · 2025-12-19T13:58:43Z

pkg/server/tokenrequest/tokenrequest.go

 func (t *tokenRequest) displayToken(osinOAuthClient *osincli.Client, w http.ResponseWriter, req *http.Request) {
-	switch req.Method {
-	case http.MethodGet:
-		t.displayTokenGet(osinOAuthClient, w, req)
-	case http.MethodPost:
-		t.displayTokenPost(osinOAuthClient, w, req)
-	default:
+	if req.Method != http.MethodGet {
 		http.Error(w, "Method not allowed", http.StatusMethodNotAllowed)


This seems like a breaking change - is there a particular reason we are making this change?

spadgett · 2025-12-19T16:02:11Z

pkg/server/tokenrequest/tokenrequest.go

-    <input type="hidden" name="code" value="{{.Code}}">
-    <input type="hidden" name="csrf" value="{{.CSRF}}">
-    <button type="submit">
-      Display Token


We can't remove this. This was there to prevent cross-site request forgery.

/hold

openshift-ci · 2025-12-19T16:03:20Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: everettraven, vojtechszocs
Once this PR has been reviewed and has the lgtm label, please assign deads2k for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot requested review from ibihim and liouk October 29, 2025 19:39

spadgett reviewed Oct 29, 2025

View reviewed changes

pkg/server/tokenrequest/tokenrequest.go Outdated Show resolved Hide resolved

pkg/server/tokenrequest/tokenrequest.go Outdated Show resolved Hide resolved

spadgett reviewed Oct 30, 2025

View reviewed changes

pkg/server/tokenrequest/tokenrequest.go Outdated Show resolved Hide resolved

vojtechszocs force-pushed the token-page-copy-to-clipboard branch from 0f2e803 to f02b3b4 Compare October 30, 2025 16:47

spadgett reviewed Oct 30, 2025

View reviewed changes

pkg/server/tokenrequest/tokenrequest.go Outdated Show resolved Hide resolved

spadgett reviewed Nov 3, 2025

View reviewed changes

pkg/server/tokenrequest/tokenrequest.go Outdated Show resolved Hide resolved

vojtechszocs force-pushed the token-page-copy-to-clipboard branch 2 times, most recently from 3632827 to 3cd1e22 Compare November 3, 2025 18:39

spadgett reviewed Nov 3, 2025

View reviewed changes

pkg/server/tokenrequest/tokenrequest.go Outdated Show resolved Hide resolved

vojtechszocs force-pushed the token-page-copy-to-clipboard branch from 3cd1e22 to c9a96d7 Compare November 3, 2025 18:54

spadgett approved these changes Nov 3, 2025

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 3, 2025

openshift-ci bot assigned spadgett Nov 3, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 3, 2025

everettraven approved these changes Nov 6, 2025

View reviewed changes

openshift-ci bot assigned everettraven Nov 6, 2025

ibihim reviewed Nov 12, 2025

View reviewed changes

vojtechszocs added 3 commits December 10, 2025 20:57

Add copy to clipboard buttons to display token page

7909825

Add Copy button feedback & add Show button to reveal data

34ca6e5

displayToken HTTP GET will render the display token page

412eb21

vojtechszocs force-pushed the token-page-copy-to-clipboard branch from c9a96d7 to 412eb21 Compare December 10, 2025 19:58

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 10, 2025

everettraven reviewed Dec 19, 2025

View reviewed changes

spadgett requested changes Dec 19, 2025

View reviewed changes

Add copy to clipboard buttons to display token page #200

Are you sure you want to change the base?

Add copy to clipboard buttons to display token page #200

Uh oh!

Conversation

vojtechszocs commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

spadgett left a comment

Choose a reason for hiding this comment

Uh oh!

vojtechszocs commented Nov 4, 2025

Uh oh!

everettraven left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Dec 10, 2025

Uh oh!

openshift-ci bot commented Dec 11, 2025

Uh oh!

ibihim commented Dec 19, 2025

Uh oh!

ibihim commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

everettraven left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Dec 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vojtechszocs commented Oct 29, 2025 •

edited

Loading

ibihim commented Dec 19, 2025 •

edited

Loading