panva / openid-client

OAuth 2 / OpenID Connect Client API for JavaScript Runtimes
MIT License
1.83k stars 392 forks source link

Allow pass through of defined querystring parameters in the passport strategy #136

Closed major-mann closed 5 years ago

major-mann commented 5 years ago

name: Feature addition type: Feature addition about: Allow querystring parameters to be passed through when using passport strategy

This change allows the definition of querystring parameters that should be passed along to the server during authorization. These parameters will be attached to the redirection URI.

codecov[bot] commented 5 years ago

Codecov Report

Merging #136 into master will not change coverage. The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #136   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          18     18           
  Lines         798    809   +11     
=====================================
+ Hits          798    809   +11
panva commented 5 years ago

Hello @major-mann,

thank you, but please see my comments in #135 and #93. I don't see the point of this code given you can explicitly pass parameters on the fly already. Can you provide more details on WHY this is needed in favor of what #93 outlines?

Furthermore this sounds like an easy way to become open redirector if used wrong. In the future please see the CONTRIBUTING.md, specifically

When contributing to this project, please first discuss the change you wish to make via issue, email, or any other method with the owners of this project before proposing a change via a Pull Request.

This is so that you save yourself the effort in cases where the proposed change might be rejected.

panva commented 5 years ago

ad https://github.com/panva/node-openid-client/pull/135#issuecomment-437361103

As pointed in #93 you can already achieve that like so

app.get('/auth', function (req, res) {
  const additionalParams = {}; // assign your passthroughs to additionalParams here.
  passport.authenticate('oidc', additionalParams)(req, res);
});

Can you provide more details on WHY this is needed inside the library then?