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

Dynamic authentication scopes #2532

Merged
merged 9 commits into from
May 21, 2015
Merged

Dynamic authentication scopes #2532

merged 9 commits into from
May 21, 2015

Conversation

nlf
Copy link
Member

@nlf nlf commented May 12, 2015

I took the simplest approach that I could think of here. Let me know if anything should be changed.

@nlf nlf added the feature New functionality or improvement label May 12, 2015
@@ -269,11 +269,25 @@ internals.Auth.prototype._authenticate = function (request, next) {

// Check scope

var expandScope = function (scope) {

return scope.replace(/{([^{}]*)}/g, function (_, context) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure you don't you want + instead of * ?

@nlf
Copy link
Member Author

nlf commented May 14, 2015

Thinking it may be wise to white list the parameters the dynamic scope is able to access, allowing anything on the request seems like asking for trouble

@nlf
Copy link
Member Author

nlf commented May 14, 2015

Ok, I think I feel ok about this since it only allows the scope to contain things the user is sending in. This should eliminate the possibility of a malicious user exploiting this feature to leak data.

@@ -269,11 +269,29 @@ internals.Auth.prototype._authenticate = function (request, next) {

// Check scope

var expandScope = function (scope) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this needs to be a function? Why not just inline the logic inside the for loop?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was because the linter throws a "don't make functions within a loop" warning, so I moved it out of the loop

@nlf
Copy link
Member Author

nlf commented May 19, 2015

added the tests you wanted, moved the code to make sure the scope is an array to route setup, and tweaked the loop stuff a little. i kept the logic of the replacing in a function to avoid the "don't create functions in a loop" linter warning.

};

for (var i = 0, il = config.scope.length; i < il; ++i) {
config.scope[i] = config.scope[i].replace(/{([^{}]+)}/g, expandScope);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to set a flag when a route contains a scope with valid {} template parameter, then only call this replace logic if that flag is set. Otherwise you are wasting cycles for the 99% not using this feature.

@hueniverse hueniverse self-assigned this May 21, 2015
@hueniverse hueniverse added this to the 8.5.0 milestone May 21, 2015
@hueniverse hueniverse changed the title implement dynamic scopes Dynamic authentication scopes May 21, 2015
hueniverse pushed a commit that referenced this pull request May 21, 2015
Dynamic authentication scopes
@hueniverse hueniverse merged commit ae0634d into master May 21, 2015
@hueniverse hueniverse deleted the dynamic_scope branch May 21, 2015 05:05
hueniverse added a commit that referenced this pull request May 21, 2015
@KyleAMathews KyleAMathews mentioned this pull request Aug 9, 2015
@lock
Copy link

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New functionality or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants