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
allow non expiring authentication tokens by setting ttl to -1. #1797
Conversation
I guess this was the initial intent since the -1 value is allowed already
Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test." |
really useful patch. |
Hi @jesucarr, thank you for the pull request. I am fine with allowing non-expiring auth tokens, even though I think it may be a security vulnerability. However, I am not rather uncomfortable to enable this "feature" in all existing applications. Could you please introduce a new setting to enable this feature? var isValid = elapsedSeconds < secondsToLive ||
(this.constructor.settings.allowEternalTTL && secondsToLive === -1); Note that you can already achieve the same effect by setting TTL to @raymondfeng @ritch what's your opinion on this? Am I too paranoid? |
@slnode ok to test |
+1 for config to turn this feature on/off |
ok just pushed the new setting as proposed. I think it's a good idea. However I changed the name to |
@jesucarr thank you for the update. Could you please add a unit-test to check the correctness of your implementation and prevent regressions in the future? |
+1 good community contribution, will try it out when its merged |
It feels incongruent to have this |
is there any good plugin for loopback api security? |
If anyone is looking at workarounds and wants an expiration date longer than 1 year -- see #353 |
Any news on this feature getting into production? |
Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test." |
+1 |
Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test." |
+1 |
2 similar comments
+1 |
+1 |
LGTM, @bajtos @raymondfeng @ritch Thoughts? We should get this merged if there are no objections. |
@jesucarr How we can use the new option after merging this change? |
@ahmed-abdulmoniem you just need to set the ttl to -1 in the options |
Can someone add unit tests to verify these changes? |
@jesucarr I mean in which options, options of the model or what? |
@ahmed-abdulmoniem yes AccessToken options, or login options https://docs.strongloop.com/display/public/LB/Logging+in+users |
This is a good point. Perhaps we should implement this change at the place where the AccessToken is created and we have access to Regardless of that, we need a unit-test before we can land this change. Is there any volunteer to pick up that task? |
Hi guys, do you know if this is going to be merged any time soon? |
++++1 |
A couple things before this can be merged:
I want this code to be overly-clear so that if any of us come accross it again, its obvious what it is doing. Something like: var created = this.created.getTime();
var elapsedSeconds = (now - created) / 1000;
var secondsToLive = this.ttl;
var allowEternalTTL = this.constructor.settings.allowEternalTTL;
var isEternalToken = secondsToLive === -1;
var isValidEternalToken = isEternalToken && allowEternalTTL;
var isValid = isValidEternalToken || (elapsedSeconds < secondsToLive); |
Can one of the admins verify this patch? |
@bajtos agree 100% |
Closing in favour of #2841. |
I guess this was the initial intent since the -1 value is allowed already