Set up ory kratos, add identity admin api #1460

shuofan · 2021-08-16T09:44:38Z

What type of this PR

feature

What this PR does / why we need it:

the first step in replacing uc component

Specified Reviewers:

/assign @Effet

codecov · 2021-08-16T09:59:58Z

Codecov Report

Merging #1460 (810578d) into master (c35c350) will increase coverage by 0.11%.
The diff coverage is 17.47%.

@@            Coverage Diff             @@
##           master    #1460      +/-   ##
==========================================
+ Coverage   15.49%   15.61%   +0.11%     
==========================================
  Files        1143     1148       +5     
  Lines      109672   109930     +258     
==========================================
+ Hits        16995    17166     +171     
- Misses      91020    91051      +31     
- Partials     1657     1713      +56

Impacted Files	Coverage Δ
apistructs/user.go	`0.00% <ø> (ø)`
modules/core-services/initialize.go	`0.00% <0.00%> (ø)`
modules/dop/initialize.go	`5.98% <0.00%> (-0.02%)`	⬇️
modules/openapi/api/apis/uc/uc_user_create.go	`0.00% <0.00%> (ø)`
modules/openapi/api/apis/uc/uc_user_freeze.go	`0.00% <0.00%> (ø)`
...s/openapi/api/apis/uc/uc_user_list_login_method.go	`4.68% <0.00%> (-0.32%)`	⬇️
modules/openapi/api/apis/uc/uc_user_unfreeze.go	`0.00% <0.00%> (ø)`
...les/openapi/api/apis/uc/uc_user_update_userinfo.go	`0.00% <0.00%> (ø)`
modules/openapi/hooks/posthandle/injectuserinfo.go	`24.39% <0.00%> (-1.59%)`	⬇️
pkg/ucauth/identity.go	`0.00% <0.00%> (ø)`
... and 23 more

Effet · 2021-09-10T05:50:36Z

modules/openapi/api/apis/uc/uc_user_freeze.go

@@ -86,6 +86,10 @@ func freezeUser(w http.ResponseWriter, r *http.Request) {
 }

 func handleFreezeUser(userID, operatorID string, token ucauth.OAuthToken) error {
+	if token.TokenType == ucauth.OryCompatibleClientId {
+		return ucauth.ChangeUserState(token.AccessToken, userID, "inactive")


magic value is not good: "inactive"

Effet · 2021-09-10T06:01:37Z

modules/openapi/api/apis/uc/uc_user_list_login_method.go

+	// TODO: password oidc
+	if token.TokenType == ucauth.OryCompatibleClientId {
+		return &listLoginTypeResult{
+			RegistryType: []string{"email"},


magic value

Effet · 2021-09-10T06:01:50Z

modules/openapi/api/apis/uc/uc_user_unfreeze.go

@@ -87,6 +87,10 @@ func unfreezeUser(w http.ResponseWriter, r *http.Request) {
 }

 func handleUnfreezeUser(userID, operatorID string, token ucauth.OAuthToken) error {
+	if token.TokenType == ucauth.OryCompatibleClientId {
+		return ucauth.ChangeUserState(token.AccessToken, userID, "active")


magic value

Effet · 2021-09-10T06:13:50Z

pkg/ucauth/identity_paging.go

+				cnt++
+			}
+		}
+		if cnt >= 10 {


why only 10 matched users?

Effet · 2021-09-10T06:15:50Z

pkg/ucauth/identity_paging.go

+	if start > len(i) {
+		return nil
+	}
+	end := start + pageSize


it's better to ensure pageSize > 0

Effet · 2021-09-10T06:20:26Z

pkg/ucauth/kratos/identity.schema.json

@@ -0,0 +1,52 @@
+{
+  "$id": "https://schemas.ory.sh/presets/kratos/quickstart/email-password/identity.schema.json",


looks like it's an example value.

Effet · 2021-09-10T06:21:02Z

pkg/ucauth/kratos/kratos.yml

@@ -0,0 +1,80 @@
+version: v0.4.6-alpha.1


version is too low?

Effet · 2021-09-10T06:21:39Z

pkg/ucauth/kratos/kratos.yml

+    base_url: http://kratos:4434/
+
+selfservice:
+  default_browser_return_url: http://one.erda.local


should be replace to related path, not static domain

Config source validation has been improved in later ory/kratos version. we are not using it. it can be replaced by env SELFSERVICE_DEFAULT_BROWSER_RETURN_URL .

Effet · 2021-09-10T06:22:35Z

pkg/ucauth/model.go

+		Mobile:   u.Phone,
+		Email:    u.Email,
+		Enabled:  true,
+		Locked:   u.State == "inactive",


magic value

Effet · 2021-09-10T06:23:10Z

quick-start/kratos/kratos.yml

@@ -11,7 +11,7 @@ serve:
    base_url: http://kratos:4434/

 selfservice:
-  default_browser_return_url: /
+  default_browser_return_url: http://one.erda.local


do not user hard-coded domain

sfwn · 2021-09-18T07:27:09Z

.erda/migrations/kratos/20210914-kratos-uc-id.sql

@@ -0,0 +1,7 @@
+CREATE TABLE `kratos_uc_userid_mapping` (
+  `id` varchar(50) NOT NULL COMMENT 'uc userid',


change id to uc_user_id.

add index for both uc_user_id and user_id separately.

add user_id index

sfwn · 2021-09-18T07:53:49Z

modules/core-services/dao/migration.go

+func (client *DBClient) InsertMapping(userID, uuid, hash string) error {
+	return client.Transaction(func(tx *gorm.DB) error {
+		sql := fmt.Sprintf("UPDATE identity_credentials SET config = JSON_SET(config, '$.hashed_password', ?) WHERE identity_id = ?")
+		if err := client.Exec(sql, hash, uuid).Error; err != nil {


client.Exec should be tx.Exec ?

sfwn · 2021-09-18T08:06:17Z

modules/core-services/endpoints/endpoints.go

@@ -244,6 +246,12 @@ func WithFileSvc(svc *filesvc.FileService) Option {
 	}
 }

+func WithMigrationSvc(svc *migration.Migration) Option {


Bad pkg. Just put it under uc service.

sfwn · 2021-09-18T08:06:56Z

modules/core-services/endpoints/endpoints.go

@@ -433,5 +449,6 @@ func (e *Endpoints) Routes() []httpserver.Endpoint {
 		{Path: "/api/users", Method: http.MethodGet, Handler: e.ListUser},
 		{Path: "/api/users/current", Method: http.MethodGet, Handler: e.GetCurrentUser},
 		{Path: "/api/users/actions/search", Method: http.MethodGet, Handler: e.SearchUser},
+		{Path: "/api/users/userID", Method: http.MethodGet, Handler: e.GetUcUserID},


Bad path. No uppercase.

sfwn · 2021-09-18T08:11:47Z

modules/core-services/initialize.go

+		return
+	}
+	for i := 0; i < 3; i++ {
+		if ep.UcSvc().MigrationReady() {


The condition maybe inverse?

sfwn · 2021-09-18T08:12:35Z

modules/core-services/initialize.go

+	if !conf.OryEnabled() {
+		return
+	}
+	for i := 0; i < 3; i++ {


sfwn · 2021-09-18T08:13:04Z

modules/core-services/model/migration.go

@@ -0,0 +1,20 @@
+package model


filename should be uc_migration

sfwn · 2021-09-18T08:42:56Z

modules/openapi/conf/conf.go

 }

+// TODO change return url
 func OryLogoutURL() string {
 	return "/.ory/kratos/public/self-service/browser/flows/logout"


Is this url correct?

will remove deprecated configs

sfwn · 2021-09-22T11:10:34Z

modules/core-services/endpoints/endpoints.go

@@ -254,6 +262,14 @@ func (e *Endpoints) GetLocale(request *http.Request) *i18n.LocaleResource {
 	return e.bdl.GetLocaleByRequest(request)
 }

+func (e *Endpoints) MigrationSvc() *migration.Migration {


split true user service.

sfwn · 2021-09-22T11:24:04Z

quick-start/kratos/kratos.yml

      lifespan: 10m

    registration:
      lifespan: 10m
-      ui_url: /uc/auth/registration
+      ui_url: http://one.erda.local/uc/auth/registration 


add comment in code. Not just on conversation.

adjusted

sfwn · 2021-09-22T14:19:48Z

/approve

shuofan · 2021-09-23T02:01:27Z

/cherry-pick release/1.3

* Set up ory kratos, add identity admin api * Code refactor, comment in kratos config

* Set up ory kratos, add identity admin api * Code refactor, comment in kratos config Co-authored-by: shuofan <fanshuo2015@gmail.com>

erda-bot requested a review from Effet August 16, 2021 09:44

shuofan added dop devops plaoform feature labels Aug 16, 2021

shuofan force-pushed the feature/ory-kratos-setup branch from db847bd to ea72ec5 Compare September 7, 2021 13:55

shuofan marked this pull request as ready for review September 7, 2021 13:56

shuofan force-pushed the feature/ory-kratos-setup branch from ea72ec5 to 6dadb3b Compare September 9, 2021 12:13

Effet previously requested changes Sep 10, 2021

View reviewed changes

Effet marked this pull request as draft September 11, 2021 14:42

shuofan force-pushed the feature/ory-kratos-setup branch from 6dadb3b to 1f5f9cb Compare September 18, 2021 07:13

sfwn requested changes Sep 18, 2021

View reviewed changes

shuofan force-pushed the feature/ory-kratos-setup branch 2 times, most recently from 20f8a5e to 79bc038 Compare September 22, 2021 09:43

Set up ory kratos, add identity admin api

d3a43be

shuofan force-pushed the feature/ory-kratos-setup branch from 79bc038 to d3a43be Compare September 22, 2021 10:11

shuofan marked this pull request as ready for review September 22, 2021 10:44

sfwn requested changes Sep 22, 2021

View reviewed changes

Code refactor, comment in kratos config

810578d

sfwn approved these changes Sep 22, 2021

View reviewed changes

erda-bot added the approved label Sep 22, 2021

erda-bot approved these changes Sep 22, 2021

View reviewed changes

erda-bot merged commit af0f09f into erda-project:master Sep 22, 2021

erda-bot pushed a commit to erda-bot/erda that referenced this pull request Sep 23, 2021

Set up ory kratos, add identity admin api (erda-project#1460)

5175136

* Set up ory kratos, add identity admin api * Code refactor, comment in kratos config

erda-bot mentioned this pull request Sep 23, 2021

Automated cherry pick of #1460: Set up ory kratos, add identity admin api #2045

Merged

erda-bot added a commit that referenced this pull request Sep 23, 2021

Set up ory kratos, add identity admin api (#1460) (#2045)

3263160

* Set up ory kratos, add identity admin api * Code refactor, comment in kratos config Co-authored-by: shuofan <fanshuo2015@gmail.com>

shuofan deleted the feature/ory-kratos-setup branch September 23, 2021 06:17

		@@ -0,0 +1,52 @@
		{
		"$id": "https://schemas.ory.sh/presets/kratos/quickstart/email-password/identity.schema.json",

		@@ -0,0 +1,7 @@
		CREATE TABLE `kratos_uc_userid_mapping` (
		`id` varchar(50) NOT NULL COMMENT 'uc userid',

Set up ory kratos, add identity admin api #1460

Set up ory kratos, add identity admin api #1460

Conversation

shuofan commented Aug 16, 2021

What type of this PR

What this PR does / why we need it:

Specified Reviewers:

codecov bot commented Aug 16, 2021 • edited Loading

Codecov Report

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfwn commented Sep 22, 2021

shuofan commented Sep 23, 2021

codecov bot commented Aug 16, 2021 •

edited

Loading