Rename variable from args to query params for authz endpoint

[shawnhankim]

Issue:

• Enhance custom query params for OIDC authZ endpoint #77

Background:

• As a Product Manager,
  I want to more flexibly configure query parameters for the OIDC authZ endpoint. So customers can customize the OIDC endpoints to pass vendor specific query parameters to complete their flow. For example, Azure AD B2B expects to send a special query param called resource-id to be passed to its authorization endpoint.

In addition to that, I want to synchronize the variable name between NGINX Plus and NGINX Management Suite.

• NGINX Plus OIDC: $oidc_authz_extra_args is merged (Dec/8/2022)
• NGINX Management Suite: $oidc_authz_query_params is released (Jul/20/2022)

Description:

• Refactored and enhanced the existing reference implementation and the latest PR to support following options:
  • option 1. Use built-in params
  • option 2. Extend extra params after the built-in params
  • option 3. Replace built-in params with custom params
• Revised the name from $oidc_authz_extra_args to $oidc_authz_query_params.
• Added key/values (e.g., $pkce_code_challenge, $nonce_hash) that can be configured as query params by customers for the OIDC authZ endpoint.

[shawnhankim]

Hi @alanwilkie-finocomp ,

• Thanks for your PR and great finding.
• The variable of $oidc_authz_extra_args is used as a different name ($oidc_authz_query_params) in the product of NGINX Management Suite.
• I was wondering if we could change the name from extra_args to query_params which is used in the many areas such as Open API, and if we could synchronize the variable name between TWO products.

Hi @route443 ,

• I have additionally added an option for customers to choose as the description. So the endpoint will be more flexibly configurable, and this PR can also handle the question of Issue #47.

Hi @alanwilkie-finocomp and @route443,

I would appreciate it if you could feel free to review and let me know if you have other opinion. Happy New Year!

[shawnhankim]

FYI: You can test this with this repo: https://github.com/nginx-openid-connect/nginx-oidc-examples/tree/main/001-oidc-local-test

[route443]

Hi @shawnhankim, Thank you for your contribution to the NGINX OIDC reference implementation!
I have some doubts about the appropriateness of this PR:

1. The current variable $oidc_authz_extra_args already emphasizes that this OIDC var implies some set of additional optional parameters/arguments on top of the authZArgs njs variable. That is, the current var name, in my opinion, fully reflects its main purpose.
2. The fact that you are already using a different name for this variable does not impose any additional liability on us. That is, if you have made a fork of the project, from now on you can change it as you like in accordance with your own goals and objectives, and this does not impose any responsibility on the parent.
3. We don't need the added flexibility of completely replacing built-in authZArgs args with user-defined args. This adds complexity to the configuration process with no visible benefits.

Thanks for understanding!

[shawnhankim]

Hi @route443 ,

Thanks for your review. I understand what you mean.

Per our discussion in the other PR, I have just renamed the variable name from args to query params as it is commonly used. It would be also helpful to synchronize naming convention between logout query params and authz query params instead of args.

For example, OpenAPI Spec uses several parameter types as follows:

• path parameters, such as /users/{id}
• query parameters, such as /users?role=admin
• header parameters, such as X-MyHeader: Value
• cookie parameters, which are passed in the Cookie header

We might be able to add more custom path/query/header/cookie parameters here in the future. By the way, NMS-ACM already has some of additional params. So, I wanted to use the variable name as parameters rather than args. I was wondering if it would make sense to you.

Thanks!