oc login: Show tokenURL message if only IDP is basic and user has not provided username #553

sallyom · 2020-09-03T06:56:49Z

If a user provides oc login -u anything, try for basic auth prompt.
If only password IDP available, and no username was provided, show the tokenURL message instead of basic auth prompt.

sallyom · 2020-09-03T07:00:25Z

this is a guess as to what should happen

soltysh · 2020-09-03T09:57:37Z

I don't think this is a good approach, b/c you're basically forcing username to be passed to pick basic auth. I was rather imagining that when you get information from the server about supported auth methods, and if only basic you'd go with that always. But when more than basic is present always prefer the other one. @stlaz might help you with that and I think that's what @smarterclayton had in mind

soltysh · 2020-09-03T09:58:05Z

The actual response you're faking here is coming from oauth-server, from here: https://github.com/openshift/oauth-server/blob/dcbeb48c9cedcf0827629556f1e6cdb8538d4ebe/pkg/authenticator/challenger/placeholderchallenger/placeholder_challenger.go#L24

sallyom · 2020-09-03T13:39:45Z

The actual response you're faking here is coming from oauth-server, from here: https://github.com/openshift/oauth-server/blob/dcbeb48c9cedcf0827629556f1e6cdb8538d4ebe/pkg/authenticator/challenger/placeholderchallenger/placeholder_challenger.go#L2

I saw that, ok - i'll rework this.

sallyom · 2020-09-08T14:38:54Z

I don't think this is a good approach, b/c you're basically forcing username to be passed to pick basic auth. I was rather imagining that when you get information from the server about supported auth methods, and if only basic you'd go with that always. But when more than basic is present always prefer the other one. @stlaz might help you with that and I think that's what @smarterclayton had in mind

I've reworked this:
Change in openshift/oauth-server to mark the bootstrap basic auth with realm "bootstrap" (as @stlaz and I discussed via slack). Here in oc if bootstrap basic challenge and no other password IDP & no username passed, then show the tokenURL.

sallyom · 2020-09-08T16:56:29Z

/test e2e-cmd

stlaz

you're also going to have to improve

oc/pkg/helpers/tokencmd/basicauth.go

Line 115 in ea0d540

func basicRealm(headers http.Header) (bool, string) {

so that it prefers non-kubeadmin realms

pkg/helpers/tokencmd/request_token.go

sallyom · 2020-09-10T16:04:51Z

@stlaz pt-another-l, thanks

stlaz

is there any specific code that will handle the warning received from the oauth-server? I couldn't find where the message about the token URL is actually printed in the case of the placeholderchallenger on the oauth-server side

pkg/helpers/tokencmd/request_token.go

pkg/helpers/tokencmd/basicauth.go

pkg/helpers/tokencmd/request_token.go

pkg/helpers/tokencmd/basicauth.go

sallyom · 2020-09-15T03:11:03Z

@stlaz, updated this PR, I fixed all the issues and added unit tests. I have a question about the basic no realm see above. thanks again for your review.

sallyom · 2020-10-08T14:15:01Z

@stlaz ptal at the unit test changes, head is spinning w/ those, thank you : )

stlaz · 2020-10-08T14:20:44Z

pkg/helpers/tokencmd/basicauth.go

+	if _, ok := err.(*basicNoUsername); ok {
+		return true
+	}
+	return false


_, ok := err.(*basicNoUsername) return ok

or export the error an inline it?

done, exported/inline, thanks!

pkg/helpers/tokencmd/basicauth.go

pkg/helpers/tokencmd/basicauth_test.go

soltysh

/approve
there are some nits from Standa left, so I'm leaving him final tag

stlaz

Needs further improvements and refactoring due to the changes of basic challenge handler.

The test case changes seem wrong -> you can add more but keep the behavior of the old tests. Unless I missed anything, the old tests weren't prompting for password only so just do that instead of turning them into negative tests.

pkg/helpers/tokencmd/basicauth.go

pkg/helpers/tokencmd/request_token_test.go

stlaz · 2020-10-09T13:02:51Z

/hold
to prevent random lgtms :)

soltysh · 2020-10-12T16:38:43Z

pkg/helpers/tokencmd/basicauth_test.go

@@ -98,34 +98,6 @@ Password: `,
 			},
 		},

-		"interactive challenge": {


@sallyom can this be changed to half-interactive when you pass username it should still ask you for a password, or we have one covering that case?

this can be omitted, as the one above covers that, it's "interactive challenge with default user"

sallyom · 2020-10-12T16:42:52Z

@stlaz I've updated all test cases, i think you can remove the hold now, and thanks

stlaz

nits only \o/

The PR looks good to me now

pkg/cli/login/login.go

stlaz · 2020-10-13T07:55:40Z

pkg/helpers/tokencmd/basicauth.go

@@ -14,6 +14,16 @@ import (
 	"github.com/openshift/oc/pkg/helpers/term"
 )

+type BasicNoUsername struct{}


godoc please :)

// BasicAuthNoUsernameError is an error that means that basic authentication challenge handling was attempted but the required username was not provided from the command line options

rename to BasicAuthNoUsernameError

updated, much better, thanks

stlaz · 2020-10-13T12:02:35Z

/hold cancel

stlaz

last nits, I promise

stlaz · 2020-10-14T08:15:17Z

pkg/helpers/tokencmd/basicauth.go

@@ -14,6 +14,16 @@ import (
 	"github.com/openshift/oc/pkg/helpers/term"
 )

+type BasicNoUsername struct{}


// BasicAuthNoUsernameError is an error that means that basic authentication challenge handling was attempted but the required username was not provided from the command line options

rename to BasicAuthNoUsernameError

stlaz · 2020-10-14T08:15:30Z

pkg/helpers/tokencmd/basicauth.go

+}
+
+// BasicNoUsernameError returns an error for a basic challenge without a username
+func BasicNoUsernameError() error {


NewBasicAuthNoUsernameError()

yes, much better, updated : )

Also, in request_token.go, updated BasicNoUsernameMessage to BasicAuthNoUsernameMessage

… provided username

stlaz · 2020-10-14T13:50:35Z

/lgtm

openshift-ci-robot · 2020-10-14T13:50:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sallyom, soltysh, stlaz

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [soltysh]

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

openshift-ci-robot requested review from smarterclayton and soltysh September 3, 2020 06:57

openshift-ci-robot assigned soltysh Sep 3, 2020

openshift-ci-robot requested a review from stlaz September 3, 2020 07:00

sallyom mentioned this pull request Sep 4, 2020

Introduce a bootstrap realm for basic-challenge so it's unique. openshift/oauth-server#55

Closed

sallyom force-pushed the login-token-url branch from f927e22 to a0a593e Compare September 4, 2020 16:27

sallyom changed the title ~~oc login: Only offer basic auth if/when a user provides -u|--username, otherwise provide the token request URL message.~~ oc login: Show tokenURL if only IDP is bootstrap IDP and user has not provided "kubeadm" as user Sep 4, 2020

sallyom changed the title ~~oc login: Show tokenURL if only IDP is bootstrap IDP and user has not provided "kubeadm" as user~~ oc login: Show tokenURL if only IDP is bootstrap and user has not provided "kubeadm" as user Sep 4, 2020

sallyom changed the title ~~oc login: Show tokenURL if only IDP is bootstrap and user has not provided "kubeadm" as user~~ WIP: oc login: Show tokenURL if only IDP is bootstrap and user has not provided "kubeadm" as user Sep 5, 2020

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 5, 2020

sallyom changed the title ~~WIP: oc login: Show tokenURL if only IDP is bootstrap and user has not provided "kubeadm" as user~~ oc login: Show tokenURL if only IDP is bootstrap and user has not provided "kubeadm" as user Sep 8, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 8, 2020

sallyom force-pushed the login-token-url branch from a0a593e to c5d9bcd Compare September 8, 2020 14:39

stlaz suggested changes Sep 10, 2020

View reviewed changes

pkg/helpers/tokencmd/request_token.go Outdated Show resolved Hide resolved

sallyom force-pushed the login-token-url branch from c5d9bcd to 5a7fdd3 Compare September 10, 2020 15:58

stlaz suggested changes Sep 11, 2020

View reviewed changes

sallyom force-pushed the login-token-url branch from 5a7fdd3 to 0ea5a8b Compare September 14, 2020 06:49

stlaz suggested changes Sep 14, 2020

View reviewed changes

pkg/helpers/tokencmd/basicauth.go Outdated Show resolved Hide resolved

pkg/helpers/tokencmd/request_token.go Outdated Show resolved Hide resolved

pkg/helpers/tokencmd/request_token.go Outdated Show resolved Hide resolved

pkg/helpers/tokencmd/basicauth.go Outdated Show resolved Hide resolved

sallyom force-pushed the login-token-url branch 3 times, most recently from a6f2422 to 0fe41a9 Compare September 15, 2020 03:03

sallyom commented Sep 15, 2020

View reviewed changes

pkg/helpers/tokencmd/basicauth.go Outdated Show resolved Hide resolved

sallyom force-pushed the login-token-url branch from 9c777a5 to bcf0351 Compare October 8, 2020 14:13

stlaz reviewed Oct 8, 2020

View reviewed changes

soltysh approved these changes Oct 9, 2020

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 9, 2020

stlaz suggested changes Oct 9, 2020

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 9, 2020

sallyom force-pushed the login-token-url branch 5 times, most recently from 709a09a to 97e9b30 Compare October 12, 2020 16:37

soltysh reviewed Oct 12, 2020

View reviewed changes

stlaz reviewed Oct 13, 2020

View reviewed changes

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 13, 2020

sallyom force-pushed the login-token-url branch from 97e9b30 to 1a68b3e Compare October 13, 2020 17:52

stlaz reviewed Oct 14, 2020

View reviewed changes

sallyom force-pushed the login-token-url branch from 1a68b3e to 4f3f784 Compare October 14, 2020 13:38

oc login: Show tokenURL message if only IDP is basic and user has not…

57684f9

… provided username

sallyom force-pushed the login-token-url branch from 4f3f784 to 57684f9 Compare October 14, 2020 13:48

openshift-ci-robot assigned stlaz Oct 14, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2020

openshift-merge-robot merged commit ef8cad4 into openshift:master Oct 14, 2020

guillaumerose mentioned this pull request Mar 1, 2021

[BUG] oc login doesn't work without username oc login https://api.crc.testing:6443 crc-org/crc#2032

Closed

andreaskaris mentioned this pull request May 3, 2021

cannot log into ocp cluster with oc 4.7 client without the username (oc login vs oc login --username) #817

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

oc login: Show tokenURL message if only IDP is basic and user has not provided username #553

oc login: Show tokenURL message if only IDP is basic and user has not provided username #553

sallyom commented Sep 3, 2020 •

edited

sallyom commented Sep 3, 2020

soltysh commented Sep 3, 2020

soltysh commented Sep 3, 2020

sallyom commented Sep 3, 2020

sallyom commented Sep 8, 2020

sallyom commented Sep 8, 2020

stlaz left a comment

sallyom commented Sep 10, 2020

stlaz left a comment

sallyom commented Sep 15, 2020

sallyom commented Oct 8, 2020

stlaz Oct 8, 2020

sallyom Oct 12, 2020

soltysh left a comment

stlaz left a comment

stlaz commented Oct 9, 2020

soltysh Oct 12, 2020

sallyom Oct 12, 2020

soltysh Oct 12, 2020

sallyom commented Oct 12, 2020

stlaz left a comment

stlaz Oct 13, 2020

sallyom Oct 13, 2020

stlaz Oct 14, 2020 •

edited

sallyom Oct 14, 2020

stlaz commented Oct 13, 2020

stlaz left a comment

stlaz Oct 14, 2020 •

edited

stlaz Oct 14, 2020

sallyom Oct 14, 2020

stlaz commented Oct 14, 2020

openshift-ci-robot commented Oct 14, 2020

oc login: Show tokenURL message if only IDP is basic and user has not provided username #553

oc login: Show tokenURL message if only IDP is basic and user has not provided username #553

Conversation

sallyom commented Sep 3, 2020 • edited

sallyom commented Sep 3, 2020

soltysh commented Sep 3, 2020

soltysh commented Sep 3, 2020

sallyom commented Sep 3, 2020

sallyom commented Sep 8, 2020

sallyom commented Sep 8, 2020

stlaz left a comment

Choose a reason for hiding this comment

sallyom commented Sep 10, 2020

stlaz left a comment

Choose a reason for hiding this comment

sallyom commented Sep 15, 2020

sallyom commented Oct 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh left a comment

Choose a reason for hiding this comment

stlaz left a comment

Choose a reason for hiding this comment

stlaz commented Oct 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sallyom commented Oct 12, 2020

stlaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stlaz Oct 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stlaz commented Oct 13, 2020

stlaz left a comment

Choose a reason for hiding this comment

stlaz Oct 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stlaz commented Oct 14, 2020

openshift-ci-robot commented Oct 14, 2020

sallyom commented Sep 3, 2020 •

edited

stlaz Oct 14, 2020 •

edited

stlaz Oct 14, 2020 •

edited