search

node-oauth / node-oauth2-server

🚀 The successor to oauthjs/oauth2-server. 🔒 Complete, compliant, maintained and well tested OAuth2 Server for node.js. Includes native async await and PKCE.
https://www.npmjs.com/package/@node-oauth/oauth2-server
MIT License
286 stars 39 forks source link

fix(types): type declaration for `AuthenticateOptions.scope` #305

Closed Lordfirespeed closed 1 month ago

Lordfirespeed commented 1 month ago

Resolves #306

dhensby commented 1 month ago

This change means the types reflect the real usage, so definitely should merge - but this touches on our discussion some time ago where I thought the conclusion was that we should be using scopes as arrays of strings for internal purposes but space delimited strings for the external interfaces. I'd have expected this to accept an array and not a string.

I'm not suggesting we change the implementation right now, but something to think about and potentially change for the next (major) release.

jankapunkt commented 1 month ago

I've been on vacation so I did not completely read the discussion, however I agree doing a breaking change with a major release.

Lordfirespeed commented 1 month ago

Guys, the breaking change actually snuck into v5.1: https://github.com/node-oauth/node-oauth2-server/commit/fcb567be659eb78c2548b28cde25a69567a9757b changed the accepted type from string[] (as in v5) to string. So this should probably be reverted.

Lordfirespeed commented 1 month ago

@dhensby @jankapunkt

jankapunkt commented 1 month ago

I remember there was a discussion on standard compliance which led to this change @dhensby correct?

dhensby commented 1 month ago

The changes made in #267 we to bring the library back in-line with OAuth specification and thus compliance. The change was a bug fix because it's a stated aim of the library to be standard compliant.

Whether that meant that some internal parts of the system that should have remained as arrays of strings were changed to just strings at the same time, may be the case, but really it's completely academic - 5.0.0 was a bad release based on not being standards compliant and 5.1.0 has been out for over 4 months with no complaints, to revert it to accept an array and not a string now would just be a dogmatic fixation on SemVer for no gain and just a lot of disruption.

We could look at accepting both space delimited strings and array of strings so we can technically adhere to semver, but I wouldn't be making a fuss about that myself.