feat: Add config struct of OpenID-Connect Login #2597

ljzsmllx · 2022-08-19T06:40:21Z

Please answer these questions before submitting a pull request, or your PR will get closed.

Why submit this pull request?

Bugfix
New feature provided
Improve performance
Backport patches

What changes will this PR take into?

Please update this section with detailed description.

feat: The series PR will add a new feature to the Dashboard to make it support login through OpenID-Connect.
This PR only adds some new config information to the config file.

fix: Fix some errors occurred in go-lint test

Related issues

Checklist:

Did you explain what problem does this PR solve? Or what new features have been added?
Have you added corresponding test cases?
Have you modified the corresponding document?
Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

starsz · 2022-08-21T14:53:35Z

api/conf/conf.yaml

-  #   host: 127.0.0.1     # the address on which the `Manager API` should listen for HTTPS.
-                          # The default value is 0.0.0.0, if want to specify, please enable it.
+      # ssl:
+      #   host: 127.0.0.1     # the address on which the `Manager API` should listen for HTTPS.


Why do you change this?

Sorry, hand shaking...

starsz · 2022-08-21T14:53:58Z

api/conf/conf.yaml

  expire_time: 3600     # jwt token expire time, in second
  users:                # yamllint enable rule:comments-indentation
    - username: admin   # username and password for login `manager api`
      password: admin
    - username: user
      password: user

+oidcApp:


oidc would be better.

starsz · 2022-08-21T14:54:23Z

api/conf/conf.yaml

+  apps:
+    - appName: authing
+      clientId: 62f3bafc15fe957a20a2ab1a
+      clientSecret: 35e4d9c9a21d2176922a4b0395ec1373


Better not put your config here.

starsz · 2022-08-21T14:55:04Z

api/internal/conf/conf.go

+	ClientSecret string
+	Scope        string
+	State        string
+	RedirectUri  string


Should add yaml flag for each field.

starsz · 2022-08-24T06:42:26Z

api/conf/conf.yaml

-                         # log example: 2020-12-09T16:38:09.039+0800	INFO	filter/logging.go:46	/apisix/admin/routes/r1	{"status": 401, "host": "127.0.0.1:9000", "query": "asdfsafd=adf&a=a", "requestId": "3d50ecb8-758c-46d1-af5b-cd9d1c820156", "latency": 0, "remoteIP": "127.0.0.1", "method": "PUT", "errs": []}
+        # such as: logs/access.log, /tmp/logs/access.log, /dev/stdout, /dev/stderr
+        # such as absolute path on Windows: winfile:///C:\access.log
+      # log example: 2020-12-09T16:38:09.039+0800	INFO	filter/logging.go:46	/apisix/admin/routes/r1	{"status": 401, "host": "127.0.0.1:9000", "query": "asdfsafd=adf&a=a", "requestId": "3d50ecb8-758c-46d1-af5b-cd9d1c820156", "latency": 0, "remoteIP": "127.0.0.1", "method": "PUT", "errs": []}


Better not modify this.

starsz · 2022-08-24T06:42:37Z

api/conf/conf.yaml

-                        # NOTE: Highly recommended to modify this value to protect `manager api`.
-                        # if it's default value, when `manager api` start, it will generate a random string to replace it.
+    # NOTE: Highly recommended to modify this value to protect `manager api`.
+  # if it's default value, when `manager api` start, it will generate a random string to replace it.


Better not modify this.

starsz · 2022-08-24T06:42:58Z

api/conf/conf.yaml

+      client_id:
+      client_secret:
+      scope: oidc
+      state: 123456


What's the state field?

starsz · 2022-08-24T06:43:37Z

api/internal/conf/conf.go

+	AppName      string `mapstructure:"app_name"`
+	ClientId     string `mapstructure:"client_id"`
+	ClientSecret string `mapstructure:"client_secret"`
+	Scope        string


Do we need to add mapstructure of this field?

starsz · 2022-08-24T08:26:02Z

api/conf/conf.yaml

+    secret
+  expire_time: 3600
+  apps:
+    - app_name: authing


I think we can support one app first.
So just remove apps.

starsz · 2022-08-24T08:27:13Z

api/conf/conf.yaml

+      client_id:
+      client_secret:
+      scope: oidc
+      redirect_uri: /authing/callback


redirect_uri shouldn't contain the authing config.
Maybe oidc/callback.

api/internal/handler/data_loader/route_import.go

api/internal/conf/conf.go

codecov-commenter · 2022-08-27T02:20:39Z

Codecov Report

Merging #2597 (caaf7c6) into master (d166405) will increase coverage by 1.61%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2597      +/-   ##
==========================================
+ Coverage   68.77%   70.39%   +1.61%     
==========================================
  Files         133      195      +62     
  Lines        3523     7528    +4005     
  Branches      864      867       +3     
==========================================
+ Hits         2423     5299    +2876     
- Misses       1100     1936     +836     
- Partials        0      293     +293

Flag	Coverage Δ
backend-e2e-test-ginkgo	`65.00% <100.00%> (?)`
backend-unit-test	`50.61% <100.00%> (?)`
frontend-e2e-test	`68.50% <ø> (-0.28%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
api/internal/conf/conf.go	`64.58% <100.00%> (ø)`
api/internal/filter/invalid_request.go	`100.00% <100.00%> (ø)`
...rnal/handler/data_loader/loader/openapi3/import.go	`79.31% <100.00%> (ø)`
web/src/pages/Route/components/Step1/MetaView.tsx	`85.88% <0.00%> (-14.12%)`	⬇️
web/src/helpers.tsx	`70.49% <0.00%> (-3.28%)`	⬇️
web/src/pages/Route/Create.tsx	`81.74% <0.00%> (-1.59%)`	⬇️
api/internal/handler/schema/plugin.go	`100.00% <0.00%> (ø)`
api/internal/handler/handler.go	`79.31% <0.00%> (ø)`
api/internal/utils/json_patch.go	`62.50% <0.00%> (ø)`
... and 62 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

nic-chen · 2022-08-27T03:54:05Z

Maybe we should merge to the next branch now?
Looks like in later versions we will use next branch instead of master?
@bzp2010 @starsz

starsz · 2022-08-29T02:24:33Z

Maybe we should merge to the next branch now? Looks like in later versions we will use next branch instead of master? @bzp2010 @starsz

Since we will merge the next branch to master.
So both are OK.

* upstream/master: (23 commits) feat: Add config struct of OpenID-Connect Login (apache#2597) feat: set serverUrlMap with env, update cypress, update stylelint (apache#2583) chore: fix function name typo (apache#2599) fix: page refresh causes deletion exception (apache#2593) feat: support show all enable plugin list tab (apache#2585) fix: drawer components delete plugin not working (apache#2573) feat: add batch delete function for route (apache#2502) test: reduce fe ci time (apache#2557) doc(csp): add correct csp rule (apache#2548) doc: add a notice about the compatibility of Ingress and Dashboard (apache#2552) fix: add judgement for last_report_time (apache#2551) fix: cli test invalid etcd (apache#2544) feat: fix actions version to root version (apache#2521) fix: duplicate ID (apache#2501) fix: block arbitrary file index (apache#2497) docs: update deploy-with-docker.md (apache#2472) feat: translating Turkish for new features (apache#2487) docs: add new import and export docs to sidebar (apache#2485) docs: add data loader and new OpenAPI 3 loader (apache#2484) feat: support data loader in frontend (apache#2480) ... # Conflicts: # api/internal/route.go # web/config/defaultSettings.ts # web/yarn.lock

update config

4d72b71

ljzsmllx changed the title ~~OpenID-Connect~~ feat: OpenID-Connect Aug 20, 2022

ljzsmllx changed the title ~~feat: OpenID-Connect~~ feat: OpenID-Connect login Aug 20, 2022

fix errors of go-lint

776a0eb

ljzsmllx changed the title ~~feat: OpenID-Connect login~~ feat: Login by the authorization of Authing in OpenID-Connect protocol Aug 20, 2022

ljzsmllx changed the title ~~feat: Login by the authorization of Authing in OpenID-Connect protocol~~ feat: Add config struct of OpenID-Connect Login Aug 20, 2022

starsz reviewed Aug 21, 2022

View reviewed changes

chunriyeqiongsaigao and others added 5 commits August 22, 2022 17:02

polished

768d4b4

polished

051b2fb

polished

51a6a51

polished

1ba6024

polished

17908c2

starsz reviewed Aug 24, 2022

View reviewed changes

starsz approved these changes Aug 25, 2022

View reviewed changes

ljzsmllx marked this pull request as ready for review August 25, 2022 04:06

nic-chen reviewed Aug 26, 2022

View reviewed changes

api/internal/handler/data_loader/route_import.go Outdated Show resolved Hide resolved

nic-chen reviewed Aug 26, 2022

View reviewed changes

api/internal/conf/conf.go Outdated Show resolved Hide resolved

polished

caaf7c6

nic-chen approved these changes Aug 26, 2022

View reviewed changes

starsz merged commit 95566d5 into apache:master Aug 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add config struct of OpenID-Connect Login #2597

feat: Add config struct of OpenID-Connect Login #2597

ljzsmllx commented Aug 19, 2022 •

edited

Loading

starsz Aug 21, 2022

ljzsmllx Aug 21, 2022

starsz Aug 21, 2022

ljzsmllx Aug 21, 2022

starsz Aug 21, 2022

starsz Aug 21, 2022

starsz Aug 24, 2022

starsz Aug 24, 2022

starsz Aug 24, 2022

starsz Aug 24, 2022

starsz Aug 24, 2022

ljzsmllx Aug 24, 2022

starsz Aug 24, 2022

ljzsmllx Aug 24, 2022

codecov-commenter commented Aug 27, 2022 •

edited

Loading

nic-chen commented Aug 27, 2022

starsz commented Aug 29, 2022

feat: Add config struct of OpenID-Connect Login #2597

feat: Add config struct of OpenID-Connect Login #2597

Conversation

ljzsmllx commented Aug 19, 2022 • edited Loading

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

codecov-commenter commented Aug 27, 2022 • edited Loading

Codecov Report

nic-chen commented Aug 27, 2022

starsz commented Aug 29, 2022

ljzsmllx commented Aug 19, 2022 •

edited

Loading

codecov-commenter commented Aug 27, 2022 •

edited

Loading