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

[Documentation] revokeAuthorizationCode argument should be named code.authorizationCode, not code.code #216

Closed
menewman opened this issue Aug 14, 2023 · 4 comments · Fixed by #222
Labels
documentation 📑 Improvements or additions to documentation

Comments

@menewman
Copy link
Contributor

Specify your setup

N/A -- purely a documentation issue

Describe the bug

View the documentation here:
https://node-oauthoauth2-server.readthedocs.io/en/latest/model/spec.html#revokeauthorizationcode-code

Snapshot of how it looks now:
Screenshot 2023-08-14 at 4 01 00 PM

There is one thing that is confusing about this, and one thing that I believe might be incorrect.

Confusing:

  • The code argument is described as The return value (I think this means it's identical to the value returned by getAuthorizationCode, but it is a bit confusing in the context of describing a function parameter)

Possibly incorrect:

  • The code.code argument does not match the return value of getAuthorizationCode, which would be code.authorizationCode instead.

Below, when a sample implementation snippet is provided, it actually uses authorizationCode and not code:
Screenshot 2023-08-14 at 4 07 51 PM

Also, when I look at an example of a consumer of this library (@leaonline/oauth2-server), I see that in fact the field used does appear to be named authorizationCode, not code:
https://github.com/leaonline/oauth2-server/blob/master/lib/model/meteor-model.js#L118

So I believe it is just a typo in the docs.

Expected behavior

Ideally, the docs should be updated for clarity.

The description for the code argument overall could be updated to something like "The code to be revoked," mirroring the description for saveAuthorizationCode.

And the code.code field could be renamed in the docs to code.authorizationCode.

Additional context

I'd be happy to submit a small documentation fix PR if maintainers agree that what I'm suggesting makes sense. :)

Just sending an issue first to comply with the "No PR without issue" rule in the Contributing guidelines:
https://github.com/node-oauth/node-oauth2-server/blob/074e3921edb41763999df4ad1c95e9c029675318/CONTRIBUTING.md#no-pr-without-issue

@menewman
Copy link
Contributor Author

menewman commented Aug 14, 2023

Actually, as I read the docs more, I'm a little unsure that it's true that it should be authorizationCode, as the documentation for getAuthorizationCode does in fact use code.code, even though saveAuthorizationCode uses code.authorizationCode. Maybe the intention was always to use code.

Either way, the docs for revokeAuthorizationCode are internally inconsistent, though, since the field documentation says code.code but the example snippet shows code.authorizationCode.

It is interesting to me that the consumer I looked at saves and returns authorizationCode instead of code:
https://github.com/leaonline/oauth2-server/blob/master/lib/model/meteor-model.js#L98

...but, maybe it doesn't actually matter what it's called, as long as it is consistent between getAuthorizationCode and revokeAuthorizationCode? (i.e., we always use code or always use authorizationCode?)

@jankapunkt
Copy link
Member

Hi @menewman thanks a lot for pointing this out! Since we forked the project and inherited the docs there may be details we simply missed during reviews. Generally, we are very thankful for every PR that fixes issues with code and docs! I

will take a look at things and come back to you.

@jankapunkt
Copy link
Member

You are right:

The code.code argument does not match the return value of getAuthorizationCode, which would be code.authorizationCode instead

This is wrong in the current documentation of revokeAuthorizationCode

Actually, as I read the docs more, I'm a little unsure that it's true that it should be authorizationCode, as the documentation for getAuthorizationCode does in fact use code.code, even though saveAuthorizationCode uses code.authorizationCode. Maybe the intentional was always to use code.

This seems to be a typo, too. We did a rewrite and renamed code.code to code.authorizationCode to avoid confusing namespaces but this seems to be slipped when reviewing the docs.

The return type of getAuthorizationCode is simply what the Model returns and if your Model saved the code with the same structure as proposed in saveAuthorizationCode it is likely to be code.authorizationCode rather than code.code.

I also searched the tests and none of them used code.code or { code: ... } in any responses anymore.

Summarized, yes the docs are wrong and there should nowhere be a code.code parameter anymore.

@jankapunkt jankapunkt added the documentation 📑 Improvements or additions to documentation label Aug 15, 2023
@menewman
Copy link
Contributor Author

Thank you for reviewing, @jankapunkt!

In that case, I'd be happy to submit a PR to clean up the docs so they consistently say code.authorizationCode 👍

[off-topic] I found all of your work on this OAuth library (and on the Meteor package that consumes it) to be very, very helpful in understanding and working with OAuth within a Meteor application that I'm working on. So even though we haven't met, I wanted to say that you rock and I really appreciate your work and knowledge-sharing. I'm a fan! 🙏 🙌

jankapunkt added a commit that referenced this issue Aug 16, 2023
Merge pull request #222 from menewman/docs-fix-code-parameters thanks to @menewman
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation 📑 Improvements or additions to documentation
Projects
None yet
2 participants