Support OIDC for the SSO #5008

kumo-rn5s · 2024-06-29T11:10:27Z

What this PR does / why we need it:

This PR introduces a new OIDC login method to enhance user authentication options by supporting the basic OIDC Authorization Code Flow. It also includes support for common OIDC providers like Keycloak, Okta, and Cognito.

Which issue(s) this PR fixes:

Fixes #4906

Does this PR introduce a user-facing change?:

How are users affected by this change: Users will have the option to log in using OIDC, providing a more secure and versatile authentication method.
Is this breaking change: No
How to migrate (if breaking change): No need

TODO:

t-kikuc · 2024-07-18T07:16:23Z

Thank you for your great contribution!
I'll run your code locally, and then we'll review the code!

t-kikuc

Thank you for your great contributions.

Your code worked well! Pleaes let me share some comments.

t-kikuc · 2024-07-26T05:23:16Z

pkg/oauth/oidc/oidc.go

+var avatarUrlClaimKeys = []string{"picture", "avatar_url"}
+var roleClaimKeys = []string{"groups", "roles", "cognito:groups", "custom:roles", "custom:groups"}
+
+// OAuthClient is a oauth client for OIDC.


[nits]

Suggested change

// OAuthClient is a oauth client for OIDC.

// OAuthClient is an oauth client for OIDC.

t-kikuc · 2024-07-26T13:15:16Z

pkg/model/project.go

@@ -348,6 +355,34 @@ func (p *ProjectSSOConfig_GitHub) GenerateAuthCodeURL(project, callbackURL, stat
 	return authURL, nil
 }

+// GenerateAuthCodeURL generates an auth URL for the specified configuration.
+func (p *ProjectSSOConfig_Oidc) GenerateAuthCodeURL(project, state string) (string, error) {


How about adding tests in project_test.go like this?
(although (p *ProjectSSOConfig_GitHub) GenerateAuthCodeURL does not have tests...)

func TestGenerateAuthCodeURL(t *testing.T) { testcases := []struct { title string project string state string issuer string scopes []string expectedURL string }{ { title: "scopes are configured", project: "test-project", state: "test-state", issuer: "https://accounts.google.com", // TODO: Is this proper? or use mock?? scopes: []string{"openid", "profile", "email"}, expectedURL: "https://accounts.google.com/o/oauth2/v2/auth?access_type=online&client_id=&prompt=consent&response_type=code&scope=openid+profile+email&state=test-state%3Atest-project", }, { title: "empty scopes", project: "test-project", state: "test-state", issuer: "https://accounts.google.com", scopes: []string{}, expectedURL: "https://accounts.google.com/o/oauth2/v2/auth?access_type=online&client_id=&prompt=consent&response_type=code&scope=openid&state=test-state%3Atest-project", }, } for _, tc := range testcases { t.Run(tc.title, func(t *testing.T) { p := &ProjectSSOConfig_Oidc{ Issuer: tc.issuer, Scopes: tc.scopes, } url, err := p.GenerateAuthCodeURL(tc.project, tc.state) assert.NoError(t, err) assert.Equal(t, tc.expectedURL, url) }) } }

t-kikuc · 2024-07-26T13:31:45Z

pkg/model/project.proto

+        string user_info_endpoint = 7;
+        // The address of the proxy used while communicating with the OpenID Connect service.
+        string proxy_url = 8;
+        // scopes to request from the OpenID Connect service.


[nits]

Suggested change

// scopes to request from the OpenID Connect service.

// Scopes to request from the OpenID Connect service.

t-kikuc · 2024-07-30T01:52:06Z

web/src/components/login-page/login-form/index.tsx

@@ -41,6 +41,9 @@ const useStyles = makeStyles((theme) => ({
  githubLoginButton: {
    background: "#24292E",
  },
+  oidcLoginButton: {
+    background: "#4A90E2",
+  },


[nits] To make a space between GitHub and OIDC.

Suggested change

},

marginTop: theme.spacing(1),

},

t-kikuc · 2024-07-31T01:47:02Z

docs/content/en/docs-dev/user-guide/managing-controlplane/auth.md

+    sharedSSOConfigs:
+      - name: oidc
+        provider: OIDC
+        oidc:


t-kikuc · 2024-08-02T05:39:28Z

pkg/oauth/oidc/oidc.go

+
+	idTokenRAW, ok := c.Token.Extra("id_token").(string)
+	if !ok {
+		return nil, fmt.Errorf("no access_token in oauth2 token")


[nits]

Suggested change

return nil, fmt.Errorf("no access_token in oauth2 token")

return nil, fmt.Errorf("no id_token in oauth2 token")

t-kikuc · 2024-08-02T05:42:12Z

pkg/app/server/httpapi/callback.go

 	default:
 		return nil, fmt.Errorf("not implemented")
 	}
 }
+
+func parseProjectAndState(r *http.Request) (string, string, error) {


Would you add unit tests of parseProjectAndState() in callback_test.go? (Sorry, the file is empty now...)

t-kikuc · 2024-08-09T10:05:03Z

[TODO] I have 2 nits of UI we'll improve in other PRs.

1. Show username+role on the header.

Currently, users can't see which account they logged in.
So, for instance, I want to show the username+role when hovering over the user icon.

2. Hide the SSO button(s) in the login form when they are not configured in the project.

Currently, both login with GitHub and login with OIDC are shown.
However, it's confusing because the GitHub button will appear even if OIDC is configured.(and then the GitHub button goes to OIDC's auth page)

kumo-rn5s · 2024-08-16T05:00:32Z

@t-kikuc I have addressed all the comments mentioned above.
Please review the changes from 9087fce to ef53065

Signed-off-by: kumo-rn5s <firosstuart@gmail.com>

t-kikuc

Thank you for your great amazing contribution.

I commented some nits, but we'll fix in other PRs.

t-kikuc · 2024-08-16T05:22:11Z

docs/content/en/docs-dev/user-guide/managing-controlplane/configuration-reference.md

+
+| Field | Type | Description | Required |
+|-|-|-|-|
+| ClientId | string | The client id string of OpenID Connect oauth app. | Yes |


We'll fix in later PRs.

Field names are actually lowerCamelCase.

solved by #5173

t-kikuc · 2024-08-16T05:24:17Z

pkg/app/server/httpapi/callback_test.go

+)
+
+func TestParseProjectAndState(t *testing.T) {
+	tests := []struct {


We'll add t.Parallel() in later PRs.

solved by #5173

t-kikuc · 2024-08-16T05:24:45Z

pkg/model/project_test.go

@@ -853,3 +853,66 @@ func TestProject_DeleteRBACRole(t *testing.T) {
 		})
 	}
 }
+
+func TestGenerateAuthCodeURL(t *testing.T) {
+	tests := []struct {


We'll add 't.Parallel()' later.

ffjlabo

Thank you for the great contribution! I left a comment 🙏

pkg/app/server/httpapi/callback.go

Co-authored-by: Yoshiki Fujikane <40124947+ffjlabo@users.noreply.github.com> Signed-off-by: Kumo <35224826+kumo-rn5s@users.noreply.github.com>

kumo-rn5s · 2024-08-26T15:34:44Z

@ffjlabo Sorry for the late response. I have signed off that suggestion to commit.

t-kikuc · 2024-08-27T14:56:40Z

@kumo-rn5s Sorry, would you resolve the conflict of go.mod? 🙏

(The faiied test / go (pull_request) does not matter because it's due to a flaky test)

Signed-off-by: Kumo <35224826+kumo-rn5s@users.noreply.github.com>

Signed-off-by: kumo-rn5s <firosstuart@gmail.com>

t-kikuc · 2024-08-28T09:05:57Z

Reopen due to the flaky test...

codecov · 2024-08-28T09:26:26Z

Codecov Report

Attention: Patch coverage is 51.04167% with 94 lines in your changes missing coverage. Please review.

Project coverage is 22.96%. Comparing base (3d4bd4f) to head (35ad7e9).
Report is 1 commits behind head on master.

Files	Patch %	Lines
pkg/oauth/oidc/oidc.go	46.03%	68 Missing ⚠️
pkg/app/server/httpapi/callback.go	44.73%	21 Missing ⚠️
pkg/model/project.go	82.14%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5008      +/-   ##
==========================================
+ Coverage   22.82%   22.96%   +0.14%     
==========================================
  Files         412      413       +1     
  Lines       43838    44020     +182     
==========================================
+ Hits        10005    10111     +106     
- Misses      33042    33120      +78     
+ Partials      791      789       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

t-kikuc

Greatest Amazing Big Impact Contribution, Thank you!!! 🚀

ffjlabo

Thank you for the great commit🚀

kumo-rn5s requested review from nghialv, khanhtc1202, kentakozuka, ffjlabo, t-kikuc and Warashi as code owners June 29, 2024 11:10

github-actions bot added area/web area/go labels Jun 29, 2024

kumo-rn5s marked this pull request as draft June 29, 2024 11:11

kumo-rn5s force-pushed the feature-oidc-support branch 2 times, most recently from 3655160 to 1862336 Compare June 29, 2024 11:19

t-kikuc added the do-not-merge/wip label Jul 1, 2024

kumo-rn5s force-pushed the feature-oidc-support branch 2 times, most recently from 126d268 to 0c85d07 Compare July 6, 2024 07:39

kumo-rn5s marked this pull request as ready for review July 6, 2024 07:40

kumo-rn5s force-pushed the feature-oidc-support branch 2 times, most recently from bc6e64a to 44f2ee7 Compare July 6, 2024 10:54

t-kikuc removed the do-not-merge/wip label Jul 8, 2024

kumo-rn5s force-pushed the feature-oidc-support branch 4 times, most recently from 1fe9366 to 50640ad Compare July 11, 2024 09:38

t-kikuc added the area/server label Jul 12, 2024

t-kikuc reviewed Aug 2, 2024

View reviewed changes

kumo-rn5s force-pushed the feature-oidc-support branch 2 times, most recently from 9e30f02 to 4ce7f7b Compare August 16, 2024 04:57

feat: add OIDC login button to login form page

02a32bd

Signed-off-by: kumo-rn5s <firosstuart@gmail.com>

kumo-rn5s force-pushed the feature-oidc-support branch from 07fc480 to 2cce09b Compare August 16, 2024 05:04

github-actions bot added the area/manifests label Aug 16, 2024

fix: unit test function name

ef53065

Signed-off-by: kumo-rn5s <firosstuart@gmail.com>

kumo-rn5s force-pushed the feature-oidc-support branch from 2cce09b to ef53065 Compare August 16, 2024 05:05

Merge branch 'master' into feature-oidc-support

a609f64

github-actions bot removed the area/manifests label Aug 16, 2024

kumo-rn5s requested a review from t-kikuc August 16, 2024 05:06

t-kikuc previously approved these changes Aug 19, 2024

View reviewed changes

ffjlabo reviewed Aug 23, 2024

View reviewed changes

pkg/app/server/httpapi/callback.go Outdated Show resolved Hide resolved

Update pkg/app/server/httpapi/callback.go

5bc68ea

Co-authored-by: Yoshiki Fujikane <40124947+ffjlabo@users.noreply.github.com> Signed-off-by: Kumo <35224826+kumo-rn5s@users.noreply.github.com>

kumo-rn5s dismissed t-kikuc’s stale review via 5bc68ea August 26, 2024 15:32

Merge branch 'master' into feature-oidc-support

f4aef79

kumo-rn5s requested a review from ffjlabo August 26, 2024 15:35

kumo-rn5s and others added 2 commits August 28, 2024 12:38

Merge branch 'master' into feature-oidc-support

ab46146

Signed-off-by: Kumo <35224826+kumo-rn5s@users.noreply.github.com>

run: go mod tidy

35ad7e9

Signed-off-by: kumo-rn5s <firosstuart@gmail.com>

kumo-rn5s force-pushed the feature-oidc-support branch from dd05789 to 35ad7e9 Compare August 28, 2024 08:54

t-kikuc closed this Aug 28, 2024

t-kikuc reopened this Aug 28, 2024

t-kikuc approved these changes Aug 28, 2024

View reviewed changes

ffjlabo approved these changes Aug 28, 2024

View reviewed changes

t-kikuc merged commit f7c0b13 into pipe-cd:master Aug 28, 2024
24 of 27 checks passed

This was referenced Sep 2, 2024

Show username on the header #5172

Merged

[nits] fix docs to lowerCamelCase and add t.Parallel() #5173

Merged

This was referenced Sep 3, 2024

Update release to v0.48.8 #5176

Merged

Release v0.48.9 #5205

Merged

github-actions bot mentioned this pull request Sep 24, 2024

Update RELEASE to v0.49.0 #5221

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support OIDC for the SSO #5008

Support OIDC for the SSO #5008

kumo-rn5s commented Jun 29, 2024 •

edited

Loading

t-kikuc commented Jul 18, 2024

t-kikuc left a comment

t-kikuc Jul 26, 2024

t-kikuc Jul 26, 2024

t-kikuc Jul 26, 2024

t-kikuc Jul 30, 2024

t-kikuc Jul 31, 2024

t-kikuc Aug 2, 2024

t-kikuc Aug 2, 2024

t-kikuc commented Aug 9, 2024

kumo-rn5s commented Aug 16, 2024 •

edited

Loading

t-kikuc left a comment

t-kikuc Aug 16, 2024

t-kikuc Sep 2, 2024

t-kikuc Aug 16, 2024

t-kikuc Sep 2, 2024

t-kikuc Aug 16, 2024

ffjlabo left a comment

kumo-rn5s commented Aug 26, 2024

t-kikuc commented Aug 27, 2024

t-kikuc commented Aug 28, 2024

codecov bot commented Aug 28, 2024 •

edited

Loading

t-kikuc left a comment

ffjlabo left a comment

	// OAuthClient is a oauth client for OIDC.
	// OAuthClient is an oauth client for OIDC.

	// scopes to request from the OpenID Connect service.
	// Scopes to request from the OpenID Connect service.

	return nil, fmt.Errorf("no access_token in oauth2 token")
	return nil, fmt.Errorf("no id_token in oauth2 token")

Support OIDC for the SSO #5008

Support OIDC for the SSO #5008

Conversation

kumo-rn5s commented Jun 29, 2024 • edited Loading

t-kikuc commented Jul 18, 2024

t-kikuc left a 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

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

t-kikuc commented Aug 9, 2024

1. Show username+role on the header.

2. Hide the SSO button(s) in the login form when they are not configured in the project.

kumo-rn5s commented Aug 16, 2024 • edited Loading

t-kikuc left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffjlabo left a comment

Choose a reason for hiding this comment

kumo-rn5s commented Aug 26, 2024

t-kikuc commented Aug 27, 2024

t-kikuc commented Aug 28, 2024

codecov bot commented Aug 28, 2024 • edited Loading

Codecov Report

t-kikuc left a comment

Choose a reason for hiding this comment

ffjlabo left a comment

Choose a reason for hiding this comment

kumo-rn5s commented Jun 29, 2024 •

edited

Loading

kumo-rn5s commented Aug 16, 2024 •

edited

Loading

codecov bot commented Aug 28, 2024 •

edited

Loading