-
-
Notifications
You must be signed in to change notification settings - Fork 926
#250 Changed revokeAtuhorizationCode to expect a boolean rather than ... #274
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
#250 Changed revokeAtuhorizationCode to expect a boolean rather than ... #274
Conversation
...r than an authorization code
...lean rather than an authorization code
Hi @mjsalinger, what was the reasoning for removing the revocation checks? I felt they were useful (from the point of view of this module) to validate that the code was actually revoked.
Can you share your use-case for this?
IMO the check was counter-intuitive. The fact that there was at least one issue reporting this as a "bug" supports this.
Even if someone implemented the revocation function correctly, the only case where the check was actually useful was when the actual revocation was implemented like this:
- update the database record with an expiration date in the past
- return the updated database record
If revocation is implemented in any other way (let's say, deletion of the database record), the check is effectively testing if the user wrote one additional line of code: Pretending to have expired the token.
Agreed, a revoked token isn't necessarily an expired token. In our use case, we also deleted the token from the database rather than add an expiration. It seemed counterintuitive to and was causing issues with our backend as well as others (#290, #250). The consensus seemed to be to make it a boolean and leave the details of how the token was revoked to the model.
Fixes #250
Changes the expectation of
model.revokeAuthorizationCodefrom an auth code to a boolean.