#1066 add openid parameters

[samuelcostae]

Description

Allow admins to configure additional_properties to be sent on to OpenID providers.

Category

Enhancement

Why these changes are required?

In certain cases OpenID provider expects additional request parameter with a particular value.

What is the old behavior before changes and new behavior after changes?

Issues Resolved

#1066

Testing

Manuel testing so far

Check List

• New functionality includes testing
• New functionality has been documented
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8f1334e) 67.06% compared to head (46e9d40) 67.06%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1637   +/-   ##
=======================================
  Coverage   67.06%   67.06%           
=======================================
  Files          94       94           
  Lines        2402     2402           
  Branches      318      318           
=======================================
  Hits         1611     1611           
  Misses        713      713           
  Partials       78       78           

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

[peternied]

Thanks for the contribution Sam - could you add or update tests that confirm these parameters are included?

[samuelcostae]

Thanks for the contribution Sam - could you add or update tests that confirm these parameters are included?

Hi! in your opinion, should I extract the query creator logic to a method and test that, or test the whole route? I would assume the whole route, but decide to ask cause I don't see other instances of the routes set up by the authclasses being tested directly.

Thanks

[stephen-crawford]

Hi Sam, thanks for putting this together. This is looking good, but I wanted to make sure you saw Craig's comment above (https://github.com/opensearch-project/security-dashboards-plugin/pull/1637/files#r1388235430). Basically, it looks like we cannot allow:

grant_type
         REQUIRED.  Value MUST be set to "authorization_code".

   code
         REQUIRED.  The authorization code received from the
         authorization server.

   redirect_uri
         REQUIRED, if the "redirect_uri" parameter was included in the
         authorization request as described in [Section 4.1.1](https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.1), and their
         values MUST be identical.

   client_id
         REQUIRED, if the client is not authenticating with the
         authorization server as described in [Section 3.2.1](https://datatracker.ietf.org/doc/html/rfc6749#section-3.2.1).

To be overwritten.

I got these fields from the RFC Craig linked. There may be others, but I saw these at first. Could you make sure we block overwriting these?

Thanks again

[samuelcostae]

Hi Sam, thanks for putting this together. This is looking good, but I wanted to make sure you saw Craig's comment above (https://github.com/opensearch-project/security-dashboards-plugin/pull/1637/files#r1388235430). Basically, it looks like we cannot allow:

grant_type
         REQUIRED.  Value MUST be set to "authorization_code".

   code
         REQUIRED.  The authorization code received from the
         authorization server.

   redirect_uri
         REQUIRED, if the "redirect_uri" parameter was included in the
         authorization request as described in [Section 4.1.1](https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.1), and their
         values MUST be identical.

   client_id
         REQUIRED, if the client is not authenticating with the
         authorization server as described in [Section 3.2.1](https://datatracker.ietf.org/doc/html/rfc6749#section-3.2.1).

To be overwritten.

I got these fields from the RFC Craig linked. There may be others, but I saw these at first. Could you make sure we block overwriting these?

Thanks again

In that case, since the overwriting of values with "additional parameters" was more of an 'accidental' feature, would it make sense to just block it altogether? To keep the 'rules' more straight forward and with less exceptions?

I've pushed a commit with this change. Let me know if that is ok.

[fabianschwamborn]

Hello, i am interested in this topic too.

@samuelcostae Thank you for the patch!

I was just about to make something similar but then i saw your pull request. I need to inject "prompt=select_account" because the Microsoft Active Directory OIDC module has no other way to display the login mask, if another account is already authenticated in browser. ( https://learn.microsoft.com/en-us/entra/identity-platform/v2-protocols-oidc)

I'll re-review this once the linter changes are addressed

[stephen-crawford]

Overall looks good, one suggestion for fixing your lint issue and then a question about expanding testing scope. Thanks Sam :)

[samuelcostae]

@cwperks Can you review/approve again please?

[DarshitChanpura]

Looks good!