Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: explicitly support array of strings for AuthenticateHandler.options #309

Merged
merged 1 commit into from
Jul 30, 2024

Conversation

dhensby
Copy link
Contributor

@dhensby dhensby commented Jul 24, 2024

Summary

In #267 some changes were made for compliance reasons where scopes should be treated as strings for our OAuth interfaces but as arrays of strings for internal purposes. As part of that change, we accidentally made the scope prop for the authenticate method only support strings.

This change remediates any potential backwards compatibility changes from v5.0 to v5.1

Linked issue(s)

#307 & #267

Involved parts of the project

authenticate handler

Added tests?

@jankapunkt
Copy link
Member

We should make clear that array scopes need to be trimmed etc. We do that with strings in parseScope but we do not validate if it's an array. Could there be any issues involved with array scopes?

@dhensby
Copy link
Contributor Author

dhensby commented Jul 24, 2024

We can introduce a "validate scopes" function that validates an array of scopes - I did think about this, but decided that would lean this towards a "feature" rather than a "fix" (and I wanted to go for a fix here).

Because this array of scopes if provided by the developer (or should be!) then I don't think there's such a risk to have malformed arrays of scopes. Do we validated the arrays of scopes that are provided in any other part of the library?

@dhensby
Copy link
Contributor Author

dhensby commented Jul 24, 2024

Just to touch on the typings too - we could just put it as scope?: string[] if we feel use of a string is to be deprecated, because there's no need for the typings to include deprecated values and clearly the typings hadn't been causing problems for people up to now 😅

jankapunkt
jankapunkt previously approved these changes Jul 25, 2024
@dhensby
Copy link
Contributor Author

dhensby commented Jul 25, 2024

I've made the small change which essentially reverts the change from #305 so that the typings reflect our ideal implementation.

I think that keeps things straightforward for implementors in TS and also means it's less likely users will need to perform refactors when we do remove the string support if they've been following the types.

@Lordfirespeed
Copy link
Contributor

Lordfirespeed commented Jul 28, 2024

Any blockers here/on a release? At the moment I'm relying on

"foo" as unknown as string[]

Which is a bit of a code-smell, to say the least 😅

@jankapunkt
Copy link
Member

@Lordfirespeed I'm back at the office tomorrow and will then release an RC that reflects on all recent PRs.

@Lordfirespeed
Copy link
Contributor

@Lordfirespeed I'm back at the office tomorrow

Awesome! Thanks very much ♥️ hope your holidays have been restful & enjoyable

In node-oauth#267 some changes were made for compliance reasons where scopes should be treated
as strings for our OAuth interfaces but as arrays of strings for internal purposes.
As part of that change, we accidentally made the `scope` prop for the `authenticate`
method only support strings.

This change remediates any potential backwards compatibility changes from v5.0 to
v5.1
@dhensby dhensby merged commit 463b517 into node-oauth:master Jul 30, 2024
5 checks passed
@dhensby dhensby deleted the pulls/scope-fix branch July 30, 2024 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants