[meta] list of original project pr

[FStefanni]

Hi,

this is a list of original project pr still open, to be analyzed and possibly integrated in this code base.

• [x] 1 2.x support: oauth.token promise, returns token object instead of sending response - 424
  • This is referring to a very old version, no more relevant
• [x] 2 PKCE - 452
  • Addressed in issue #76, so we do not need to track it here
• [x] 3 Don't fail on vschar check on state if it is empty and allowEmptyState=1 - 473
  • see https://github.com/node-oauth/node-oauth2-server/pull/93
• [x] 4 Implement validateRedirectUri model function - 482
  • This is a feature-adding pr. Since it seems fine, I just added it in pr #97. The code is slightly changed w.r.t. original pr (just a little cleaner imho).
• [x] 5 Update extension-grants.rst with example - 530
  • Fixed by pr #92
• [x] 6 Allow "allowed" parameter to be fetched from body - 532
  • see https://github.com/node-oauth/node-oauth2-server/pull/94
• [x] 7 Pass scope to model on refresh_token request - 540
  • The original pr is fine w.r.t. the RFC6749 Section 6. I believe the missing part is to ensure the passed scope does not contains non-validated scopes, and this missing check could lead to potential security risks. So I suggest to implement a check to ensure the passed request scope object is a "subset" of validated scopes (or, as alternative, a method which removes invalid scopes).
• [x] 8 Update README.md - 556
  • Fixed by pr #91
• [x] 9 rokenToken is not required if alwaysIssueNewRefreshToken = false - 558
  • The pr is right, w.r.t. the rest of the code base. So it is is just a little improvement, to make the model.revokeToken() method implementation optional. So the pr could be accepted, even if it is not strictly useful (it is possible to implement the method with an empty body, or maybe with a default implementation inside a "model" base class). Therefore I am not proposing a pr for this.
• [x] 10 Add return statements for more clarity - 561
  • Pr proposed: #98. This is a simple "doc" improvement. The original pr states that is for clarity, but it is not: the doc examples are actually bugged, so this pr is required.
• [x] 11 RFC 6749#4.1.2.1 - fix order of error handling - 565
  • Pr proposed: #112. The original pr fixes the order of some checks, according with the standard, which seems fine to me.
• [x] 12 The object returned by the response.getHeaders() method does not prot… - 567
  • This pr proposes a fix already present in the code. So it is useless.
• [x] 13 Update authorize-handler.js - 576
  • This pr is an alternative implementation of pr https://github.com/oauthjs/node-oauth2-server/pull/649. See comments below (point number 20).
• [x] 14 v5: add codecoverage and upgrade packages - 610
  • This targets branch v5-dev (typescript). Probably no more useful.
• [x] 15 fix: Bearer regular expression matching in authenticate handler - 614
  • The original pr is fine. Proposed the pr #105
• [x] 16 Update refresh-token-grant-type.js - 615
  • This pr is wrong. The change introduce a bug.
• [x] 17 Passing validated scope into generateAccessToken. - 620
  • The pr targets branch v5-dev, and claims it is already fixed in other branches. So it is useless.
• [x] 18 Set WWW-Authenticate header for invalid requests - 646
  • The original pr is right, so I just re-proposed it here: #96
• [x] 19 Remove scope validation - 647
  • Proposed pr #103
  • The implementation is slightly different from the original pr. Details given in the pr description
• [x] 20 Fix user denial and missing state parameter on error redirect - 649
  • This is a different implementation of point 13
  • Propose pr #99
• [x] 21 Update refresh-token-grant-type.js - 657
  • The pr does not looks correct. Checking also the v5-dev branch, the client object seems to have a id member, and not a clientId member. Written into the original pr to have feedback.
• [x] 22 Add PKCE support - 658
  • Addressed in issue #76, so we do not need to track it here
• [x] 23 Add PKCE support (for v5-dev Typescript) - 659
  • Addressed in issue #76, so we do not need to track it here
• [ ] 24 Fix model spec to use code.authorizationCode instead of code.code - 660
  • The original pr just upgrades the doc, according with the original dev branch. Even if the pr seems not strictly useful right now, it shows that the original dev branch could contain some interesting improvements, worth to merge in this code base. To be further analyzed.
• [x] 25 dependencies updated #662
  • This targets branch v5-dev (typescript). Probably no more useful.
• [x] 26 Bump statuses from 1.5.0 to 2.0.1 - 670
  • No more relevant: package has been removed
• [x] 27 Update token-util.js - 678
  • Already fixed
• [x] 28 Bump mocha from 5.2.0 to 8.4.0 - 689
  • Fixed by the following n.32
• [x] 29 Bump sinon from 7.5.0 to 11.1.1 - 694
  • Fixed by the following n.33
• [x] 30 Bump jshint from 2.13.0 to 2.13.1 - 708
  • No more relevant: jshint has been replaced with eslint
• [ ] 31 Only require 'redirect_uri' in token/authorization_code when one was provided in authorize/code - 709
  • The original pr question is correct. But I am wondering whether the proposed code is fine. In fact I am not 100% sure about the getRedirectUri() method implementation. Probably, just removing || client.redirectUris[0] will fix all the issues. But I am not completely sure about this: I do not know the standard and the code so well...
• [x] 32 Bump mocha from 5.2.0 to 9.1.3 - 716
  • Fixed by pr #90
• [x] 33 Bump sinon from 7.5.0 to 12.0.1 - 718
  • Already fixed

The list will be upgraded to track the ongoing process of integrating the pr's.

Regards.

[HappyZombies]

I don't think we need to be tracking the dependabot PRs, those will be made for us automatically, plus reduce the clutter

[FStefanni]

Hi,

I agree. This was just to have a complete log of currently open pr's. Basically, this is just a one time stuff, just to be sure to not miss anything. But if you prefer, I can just delete the lines.

Regards.

[FStefanni]

Hi,

there are some pr referring to the v5-dev branch (typescript). I believe they are no more useful, since probably the new Typescript implementation will start from the current master branch, which is more up-to-date (the old v5-dev could be just of inspiration on how to translate). If you agree, I mark them as "fixed".

involved points so far:

• 14
• 25

Regards.

[jankapunkt]

Thanks a lot for the list @FStefanni :+1:

Please note to read our contributions guidelines before opening a PR and use the PR Template, so we all have an easier reviewing and PR process.

[jankapunkt]

By the way is there a way to write tests to validate the rst definitions?

[jankapunkt]

@FStefanni do you mind adding your review to #93 and #94 ?

[FStefanni]

Hi,

I commented directly in the two pr.

Regards.

[jorenvandeweyer]

I checked of number 7 since that pull request makes no sense and even makes it worse. We already have our own issue that addresses the old issue. #104

[jankapunkt]

Can we update this list to see what exactly is still missing? Also we should discuss, whether these should be part of v5 or if they should already be published sooner, for example in a 4.3.0 release or something,