Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

discuss: improve the UE of config Authentication plugins #529

Closed
1 of 5 tasks
liuxiran opened this issue Sep 30, 2020 · 18 comments
Closed
1 of 5 tasks

discuss: improve the UE of config Authentication plugins #529

liuxiran opened this issue Sep 30, 2020 · 18 comments
Labels
discuss enhancement New feature or request wait for update 1. Solution 2. Due date 3. Assignees if needed
Milestone

Comments

@liuxiran
Copy link
Contributor

liuxiran commented Sep 30, 2020

Please answer these questions before submitting your issue.

  • Why do you submit this issue?
  • Question or discussion
  • Bug
  • Requirements
  • Feature or performance improvement
  • Other

Why

The Authentication plugins, e.g: basic-auth openid-connect and other auth plugins supported by Apisix should work with consumer together. Only when one auth plugin is enabled and configed correctly can a consumer be created or modified, and can access a route which is also enabled the same auth plugin. At the same time, a route or a service can enabled the auth plugin without any configs(even if configured here, the auth plugin configuration of consumer will matched by default).

RIght now, the auth plugins can be configed both in consumer and route, but the auth config in route doesn't really work, and it will Induce user that only the configed consumer can access the route. So it would be better to improve the UE of config auth plugins.

Proposal

  1. the authentication plugins should be enabled with configuration in consumer module.
    2020-09-30 10-44-11屏幕截图

  2. the authentication plugins should be only shown with a switch to enabled/disabled in route or service module.
    2020-09-30 10-44-11屏幕截图

  3. right now if a route enabled two auth plugins, it should be matched with this two auth token, In fact, it is not possible for users to access the same route using two or more different authentication methods. so it would be better to only enabled one auth plugin in route.

@liuxiran liuxiran changed the title [Disscussion] improve the UE of config Authentication plugins discuss: improve the UE of config Authentication plugins Sep 30, 2020
@liuxiran
Copy link
Contributor Author

@juzhiyuan @LiteSun @bzp2010

@juzhiyuan
Copy link
Member

Got your point, you mean for Auth plugins, we should only config it in Consumer instead of Route, and we COULD choose to enable or disable it in Route instead of cobfig, right?

I agree to this proposal, how about others suggestions?

cc @membphis @nic-chen @moonming ?

@membphis
Copy link
Member

agree with this proposal too.

apache/apisix#2308 , if we support this feature, the dashboard will easier.

@nic-chen
Copy link
Member

agree +1

@liuxiran
Copy link
Contributor Author

Got your point, you mean for Auth plugins, we should only config it in Consumer instead of Route, and we COULD choose to enable or disable it in Route instead of cobfig, right?

I agree to this proposal, how about others suggestions?

cc @membphis @nic-chen @moonming ?

yes you got my point @juzhiyuan 🤝, and furthermore, if the authentication plugin can be turned into the consumer’s default support plugin, just like proxy-rewrite in route, that would be better, but the cost of plugin modification may be higher

@juzhiyuan
Copy link
Member

yep, let's implement your proposal first :D

@juzhiyuan juzhiyuan added this to the 1.7 milestone Sep 30, 2020
@juzhiyuan
Copy link
Member

I just put this feature in M1.7.

@juzhiyuan juzhiyuan added the enhancement New feature or request label Sep 30, 2020
@juzhiyuan juzhiyuan self-assigned this Nov 25, 2020
@juzhiyuan
Copy link
Member

I checked this issue once more, and it seems that the backend should support this feature too 🤔 @liuxiran @membphis right?

@liuxiran
Copy link
Contributor Author

liuxiran commented Nov 30, 2020

I checked this issue once more, and it seems that the backend should support this feature too @liuxiran @membphis right?

thanks for @juzhiyuan 's reminder, I also rethinked this issue, and IMHO, it is better to get support from BE:

  • get whether the plugin needs to be config together with category of all plugins instead of only get plugin names from GET /apisix/admin/plugins

whether the plugin needs to be config could also not be a specific flag, FE can also judge from whether the [plugin].schema.properties is empty, in this case , api should return the schema at the same time.

This would also apply to other plugins that do not require configuration :)

@juzhiyuan
Copy link
Member

This solution works for me, but it seems that there still has also a lot of work to do, could you please take a look at this issue? cc @membphis @tokers.

@membphis
Copy link
Member

We can create a new API to return all the information of the plug-in, such as schema, type, priority, etc.

For example the new API: /apisix/admin/plugins/meta_attributes, we need to discuss it on the mailing list first.

@liuxiran would like to handle this job?

@liuxiran
Copy link
Contributor Author

We can create a new API to return all the information of the plug-in, such as schema, type, priority, etc.

For example the new API: /apisix/admin/plugins/meta_attributes, we need to discuss it on the mailing list first.

@liuxiran would like to handle this job?

sure, I have already sent a mail to dev, just waiting for others' comments and suggestions :)

@juzhiyuan juzhiyuan added the wait for update 1. Solution 2. Due date 3. Assignees if needed label Dec 3, 2020
@juzhiyuan
Copy link
Member

Hi folks, final conclusion here?

@membphis
Copy link
Member

membphis commented Dec 8, 2020

@liuxiran I think we can close this issue and create a new one with the final conclusion from the emailing list.

@liuxiran
Copy link
Contributor Author

liuxiran commented Dec 8, 2020

@liuxiran I think we can close this issue and create a new one with the final conclusion from the emailing list.

got it , and as we talked yesterday., this issue would be move to M2.3, right? @membphis

@membphis
Copy link
Member

membphis commented Dec 8, 2020

yes, confirm this @liuxiran

@juzhiyuan juzhiyuan modified the milestones: 2.2, 2.3 Dec 8, 2020
@juzhiyuan juzhiyuan added good first issue Good for newcomers and removed wait for update 1. Solution 2. Due date 3. Assignees if needed good first issue Good for newcomers labels Dec 12, 2020
@juzhiyuan juzhiyuan removed their assignment Dec 12, 2020
@nic-chen nic-chen added the wait for update 1. Solution 2. Due date 3. Assignees if needed label Dec 14, 2020
@juzhiyuan
Copy link
Member

any update?

@liuxiran
Copy link
Contributor Author

after the new api GET /plugins?all=ture finished, I will open a new issue to trace the process. close it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss enhancement New feature or request wait for update 1. Solution 2. Due date 3. Assignees if needed
Projects
None yet
Development

No branches or pull requests

4 participants