-
-
Notifications
You must be signed in to change notification settings - Fork 45
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
Is implementation of verifyScope
required?
#202
Comments
@shrihari-prakash which version of the package do you refer to and in which file is this function call located? |
Hello @jankapunkt , I am on version 4.3.0. This is the function I am talking about: https://node-oauthoauth2-server.readthedocs.io/en/latest/model/spec.html#verifyscope-accesstoken-scope-callback The model specification strictly wants this function, but when I implement it with a simple console, I never see it called. (See sample here: https://github.com/shrihari-prakash/liquid/blob/352f6f96cc518452489aabd11d9587be0dca3cb3/src/model/oauth.ts#L329) |
This seems to be an outdated documentation. However, let me check the code whether this might still be located somewhere. |
These are the search results when I tried to see where this is called: https://github.com/search?q=repo%3Anode-oauth%2Fnode-oauth2-server%20verifyScope&type=code Not sure if something doesn't appear in git search. |
Oh there were matches I did not expand :). In any case, verifyScope is never called when calling |
Okay I rechecked and they were named differently because they implement different things:
Note that |
Aah ok understood! Perhaps it is a good idea to make |
Hm this is a bit problematic, since it's only optional if you provide your own |
In any case, would you mind to open a PR for the types, targeting |
Let me check the types file and see if such a logic can be incorporated. |
@jankapunkt you are right. It's pretty hard to achieve this conditional thing. However, one realistic option is we make the |
@jankapunkt are you saying even if you do not provide your own |
@shrihari-prakash this logic is already implemented, see https://github.com/node-oauth/node-oauth2-server/blob/development/lib/handlers/authenticate-handler.js#L40 What I'm referring to is to project this logic into the types system:
|
Nice.. Good that the error logic is already there. Then I do not see a problem with making the |
Thanks, would you mind helping out with #210 as well? |
Sure. No problem. |
Merge pull request #209 from shrihari-prakash/verify-scope-fix thanks to @shrihari-prakash
Closing as |
I understand the point of
validateScope
function, however, the library also has averifyScope
definition in model which never get's called at any point. I also tried searching for this function in the repository and couldn;t find any place where it is called but there are references to it. What's the deal with this function? Can I deleteverifyScope
implementations from my model?The text was updated successfully, but these errors were encountered: