AUTH-355: Add OAuth2 Authorization Code Grant Flow for login #1402

liouk · 2023-04-11T14:33:02Z

This PR reopens #1031 to add OAuth2 Authorization Code Grant Flow for oc login, adding minor improvements to the original PR. This can be invoked with oc login --web.

Requires

openshift-ci-robot · 2023-04-11T14:33:06Z

@liouk: This pull request references AUTH-355 which is a valid jira issue.

In response to this:

This PR reopens #1031 to add OAuth2 Authorization Code Grant Flow for oc login, adding minor improvements to the original PR. This can be invoked with oc login --web.

Requires

AUTH-356: Add openshift-cli-client OAuth Client cluster-authentication-operator#606

AUTH-357: update osin to latest version oauth-server#121

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/test-infra repository.

liouk · 2023-04-11T14:47:14Z

@stlaz: with oc login --web the user logs in the openshift console on the browser and thus authorizes the code grant; this also means that as long as the user is logged in the openshift console, there won't be a prompt for login, so subsequent invocations of oc login --web will result in automatic grants. If one needs to login with a different user, they would first need to logout from the openshift console -- is this desired/acceptable?

liouk · 2023-04-11T14:47:31Z

@stlaz: would we need a timeout while waiting for the authentication/authorization to happen via the browser?

liouk · 2023-04-11T14:47:46Z

@stlaz: would it be a good idea to print the browser URL on the console when oc login --web is invoked, in case the user needs to manually open it? (e.g. automatically opening it didn't work, user accidentally closed tab)

stlaz · 2023-04-24T10:52:27Z

oc login --web the user logs in the openshift console on the browser and thus authorizes the code grant; this also means that as long as the user is logged in the openshift console, there won't be a prompt for login, so subsequent invocations of oc login --web will result in automatic grants. If one needs to login with a different user, they would first need to logout from the openshift console -- is this desired/acceptable?

I believe this should be fine - two users would not generally share the same cookie cache.

would we need a timeout while waiting for the authentication/authorization to happen via the browser?

The wait should be on the client side so I think we can just keep the localhost server running as long as necessary.

would it be a good idea to print the browser URL on the console when oc login --web is invoked, in case the user needs to manually open it? (e.g. automatically opening it didn't work, user accidentally closed tab)

definitely

liouk · 2023-04-24T17:25:17Z

/retest-required

liouk · 2023-05-04T14:25:48Z

/hold

PR commits need squashing before merging

liouk · 2023-05-10T08:46:49Z

Changed to WIP in order to incorporate AUTH-376.

openshift-ci-robot · 2023-05-10T09:16:35Z

@liouk: This pull request references AUTH-355 which is a valid jira issue.

In response to this:

This PR reopens #1031 to add OAuth2 Authorization Code Grant Flow for oc login, adding minor improvements to the original PR. This can be invoked with oc login --web.

Requires

AUTH-356: Add openshift-cli-client OAuth Client cluster-authentication-operator#606

AUTH-357: update osin to latest version oauth-server#121

OCPBUGS-5233: update osin to latest version oauth-server#128

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/test-infra repository.

liouk · 2023-05-17T10:31:08Z

Rebase requires

AUTH-355: add support for oauth authorization code flow library-go#1531

liouk · 2023-05-30T09:02:52Z

Marked as WIP due to #1402 (comment)

pkg/cli/admin/catalog/mirror.go

pkg/cli/login/loginoptions.go

liouk · 2023-06-21T15:52:13Z

/hold
should merge only after the following dependencies (as discussed with workloads team):

liouk · 2023-06-22T07:30:35Z

/retest-required
infrastructure issues

stlaz · 2023-07-03T06:50:56Z

/lgtm

ardaguclu · 2023-07-04T16:21:54Z

pkg/cli/login/login.go

@@ -83,6 +87,8 @@ func NewCmdLogin(f kcmdutil.Factory, streams genericclioptions.IOStreams) *cobra
 	cmds.Flags().StringVarP(&o.Username, "username", "u", o.Username, "Username for server")
 	cmds.Flags().StringVarP(&o.Password, "password", "p", o.Password, "Password for server")

+	cmds.Flags().BoolVarP(&o.WebLogin, "web", "w", o.WebLogin, "Login with web browser")


As being asked by @atiratree, what is the expected behavior of this flag against 4.13 server?

@ardaguclu @atiratree this flag needs the server to have a specific default OAuth client (openshift-cli-client) and also the oauth server to have the latest OSIN library; i.e., it needs this code, which will only exist on 4.14 onwards:

AUTH-356: Add openshift-cli-client OAuth Client cluster-authentication-operator#606

AUTH-357: update osin to latest version oauth-server#121

OCPBUGS-5233: update osin to latest version oauth-server#128

Any attempt to invoke oc login --web against a pre 4.14 server will result on an error on the browser, as the server won't have the necessary oauth client with which this flow proceeds:

{"error":"invalid_request","error_description":"The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed."}

I would not block the PR merge on this, but I think it would be better from UX perspective if the user got a preemptive error in the CLI that the web authentication is not possible. It could use the same mechanism as we use in

https://github.com/openshift/oc/blob/master/pkg/cli/login/loginoptions.go#L292

atiratree · 2023-07-04T23:07:38Z

@liouk opened a rebase of the dependent PR to pull in the library-go changes: #1495

ardaguclu · 2023-07-05T07:00:24Z

/hold cancel

This change adds a new flag called `--web` which launches the system browser with the authorization endpoint as the target. The user can then authenticate and is redirected back to a server which is listening on the loopback address on a random port. The callback receives the authorization code which it can exchange for a token and configure the client.

ardaguclu · 2023-07-05T09:18:44Z

Nice feature!
/lgtm

openshift-ci · 2023-07-05T09:19:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ardaguclu, liouk, 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~~ [ardaguclu]

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 added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Apr 11, 2023

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 11, 2023

openshift-ci bot requested review from deads2k and soltysh April 11, 2023 14:37

liouk changed the title ~~[WIP]: AUTH-355: Add OAuth2 Authorization Code Grant Flow for login~~ AUTH-355: Add OAuth2 Authorization Code Grant Flow for login Apr 25, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 25, 2023

liouk changed the title ~~AUTH-355: Add OAuth2 Authorization Code Grant Flow for login~~ [WIP]: AUTH-355: Add OAuth2 Authorization Code Grant Flow for login Apr 25, 2023

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 25, 2023

liouk changed the title ~~[WIP]: AUTH-355: Add OAuth2 Authorization Code Grant Flow for login~~ AUTH-355: Add OAuth2 Authorization Code Grant Flow for login May 4, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 4, 2023

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

liouk changed the title ~~AUTH-355: Add OAuth2 Authorization Code Grant Flow for login~~ [WIP] AUTH-355: Add OAuth2 Authorization Code Grant Flow for login May 10, 2023

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 10, 2023

liouk changed the title ~~[WIP] AUTH-355: Add OAuth2 Authorization Code Grant Flow for login~~ AUTH-355: Add OAuth2 Authorization Code Grant Flow for login May 10, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 10, 2023

liouk mentioned this pull request May 17, 2023

AUTH-355: add support for oauth authorization code flow openshift/library-go#1531

Merged

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 17, 2023

liouk changed the title ~~AUTH-355: Add OAuth2 Authorization Code Grant Flow for login~~ WIP: AUTH-355: Add OAuth2 Authorization Code Grant Flow for login May 30, 2023

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 30, 2023

liouk force-pushed the web-login branch from 7de53e7 to abf4927 Compare June 20, 2023 11:11

stlaz reviewed Jun 21, 2023

View reviewed changes

pkg/cli/admin/catalog/mirror.go Outdated Show resolved Hide resolved

liouk force-pushed the web-login branch from abf4927 to ce35a6f Compare June 21, 2023 08:36

liouk changed the title ~~WIP: AUTH-355: Add OAuth2 Authorization Code Grant Flow for login~~ AUTH-355: Add OAuth2 Authorization Code Grant Flow for login Jun 21, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 21, 2023

stlaz reviewed Jun 21, 2023

View reviewed changes

pkg/cli/login/loginoptions.go Outdated Show resolved Hide resolved

liouk force-pushed the web-login branch from 01dc4ce to e421b76 Compare June 21, 2023 12:20

openshift-ci bot assigned stlaz Jul 3, 2023

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

ardaguclu reviewed Jul 4, 2023

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 4, 2023

ardaguclu mentioned this pull request Jul 5, 2023

upgrade distribution No 2 #1495

Merged

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 5, 2023

liouk added 2 commits July 5, 2023 10:59

add github.com/pkg/browser to go.mod and vendor

b6d3b0d

liouk force-pushed the web-login branch from e421b76 to 23ffd2d Compare July 5, 2023 08:39

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 5, 2023

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 5, 2023

openshift-ci bot assigned ardaguclu Jul 5, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 5, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 5, 2023

openshift-merge-robot merged commit 9b8d0f8 into openshift:master Jul 5, 2023
11 of 12 checks passed

liouk mentioned this pull request Nov 14, 2023

[release-4.13] AUTH-443: Add OAuth2 Authorization Code Grant Flow for login #1599

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AUTH-355: Add OAuth2 Authorization Code Grant Flow for login #1402

AUTH-355: Add OAuth2 Authorization Code Grant Flow for login #1402

liouk commented Apr 11, 2023 •

edited

openshift-ci-robot commented Apr 11, 2023 •

edited by openshift-ci bot

liouk commented Apr 11, 2023

liouk commented Apr 11, 2023

liouk commented Apr 11, 2023

stlaz commented Apr 24, 2023

liouk commented Apr 24, 2023

liouk commented May 4, 2023

liouk commented May 10, 2023 •

edited

openshift-ci-robot commented May 10, 2023 •

edited by openshift-ci bot

liouk commented May 17, 2023 •

edited

liouk commented May 30, 2023

liouk commented Jun 21, 2023 •

edited

liouk commented Jun 22, 2023

stlaz commented Jul 3, 2023

ardaguclu Jul 4, 2023

liouk Jul 4, 2023

atiratree Jul 4, 2023 •

edited

atiratree commented Jul 4, 2023

ardaguclu commented Jul 5, 2023

ardaguclu commented Jul 5, 2023

openshift-ci bot commented Jul 5, 2023

AUTH-355: Add OAuth2 Authorization Code Grant Flow for login #1402

AUTH-355: Add OAuth2 Authorization Code Grant Flow for login #1402

Conversation

liouk commented Apr 11, 2023 • edited

openshift-ci-robot commented Apr 11, 2023 • edited by openshift-ci bot

liouk commented Apr 11, 2023

liouk commented Apr 11, 2023

liouk commented Apr 11, 2023

stlaz commented Apr 24, 2023

liouk commented Apr 24, 2023

liouk commented May 4, 2023

liouk commented May 10, 2023 • edited

openshift-ci-robot commented May 10, 2023 • edited by openshift-ci bot

liouk commented May 17, 2023 • edited

liouk commented May 30, 2023

liouk commented Jun 21, 2023 • edited

liouk commented Jun 22, 2023

stlaz commented Jul 3, 2023

ardaguclu Jul 4, 2023

Choose a reason for hiding this comment

liouk Jul 4, 2023

Choose a reason for hiding this comment

atiratree Jul 4, 2023 • edited

Choose a reason for hiding this comment

atiratree commented Jul 4, 2023

ardaguclu commented Jul 5, 2023

ardaguclu commented Jul 5, 2023

openshift-ci bot commented Jul 5, 2023

liouk commented Apr 11, 2023 •

edited

openshift-ci-robot commented Apr 11, 2023 •

edited by openshift-ci bot

liouk commented May 10, 2023 •

edited

openshift-ci-robot commented May 10, 2023 •

edited by openshift-ci bot

liouk commented May 17, 2023 •

edited

liouk commented Jun 21, 2023 •

edited

atiratree Jul 4, 2023 •

edited