feat(profile): Add oauth-authenticated /account/profile endpoint. #1070
Conversation
| } | ||
| var scopes = auth.credentials.scopes | ||
| for (var i = 0; i < scopes.length; i++) { | ||
| if (scopes[i] === 'profile') { |
seanmonstar
Oct 1, 2015
Member
How about:
var allowed = set('profile', 'profile:write', 'profile:' + item, 'profile:' + item + ':write');
for (var i = 0, len = scopes.length; i < len; i++) {
if (allowed[scopes[i]]) {
return true;
}
}
Assuming set is some sort of function to easily make an object:
function set() {
var obj = {};
for (var i = 0, len = arguments.length; i < len; i++) {
obj[arguments[i]] = 1;
}
return obj;
}
Bonus points if these sets are consts created once, instead of every request.
How about:
var allowed = set('profile', 'profile:write', 'profile:' + item, 'profile:' + item + ':write');
for (var i = 0, len = scopes.length; i < len; i++) {
if (allowed[scopes[i]]) {
return true;
}
}Assuming set is some sort of function to easily make an object:
function set() {
var obj = {};
for (var i = 0, len = arguments.length; i < len; i++) {
obj[arguments[i]] = 1;
}
return obj;
}Bonus points if these sets are consts created once, instead of every request.
rfk
Oct 28, 2015
Author
Member
We discussed this a little IRL and it sounds like the right answer is to provide an abstraction for scope-checking in a shared utility lib of some kind. But we don't have to do that in this PR.
We discussed this a little IRL and it sounds like the right answer is to provide an abstraction for scope-checking in a shared utility lib of some kind. But we don't have to do that in this PR.
| default: false, | ||
| env: 'OAUTH_INSECURE' | ||
| } | ||
| }, |
seanmonstar
Oct 1, 2015
Member
This assumes the oauth server is a process on the same machine?
I know in our dev servers, we seem to like to put several services behind paths as well.
This assumes the oauth server is a process on the same machine?
I know in our dev servers, we seem to like to put several services behind paths as well.
rfk
Oct 28, 2015
Author
Member
Are you asked whether this should just be a URL rather than individual host/port/protocol components? If so then yes, I think that would make sense, but we'll have to figure out how that plays with the hapi oauth integration lib.
Are you asked whether this should just be a URL rather than individual host/port/protocol components? If so then yes, I think that would make sense, but we'll have to figure out how that plays with the hapi oauth integration lib.
|
@seanmonstar do you feel like picking up dev work on this, so we can ship it alongside the service-tokens stuff in (hopefully!) train-48? I can help but I'm trying to keep myself off the critical path where possible. |
|
okie doke! |
e4af078
|
@rfk trying to understand how far along this was. After a rebase, it seems that it works? You mentioned docs are needed, was there something else? |
|
@seanmonstar so I think it's largely ready to go then. Let's switch back to author=rfk and r=seanmonstar to keep things consistent. I'll take a look at cleaning up the oauth config into maybe using a URL rather than individual components, if that seems sensible to you. One concrete thing you could take a look at in the meantime, is diving into the hapi-fxa-oauth plugin [1] for a general-purpose r? on whether it's doing all the right things. The codes's quite small and I trust @dannycoates to get it right, but having another set of eyes on there before we put it in front of our core account data is probably a good idea :-) |
|
OK @seanmonstar, I think this is ready for r?. Unfortunately it will (should!) fail travis because it expects the yet-to-be-released changes from mozilla/hapi-fxa-oauth#1, so we'll have to update deps and shrinkwrap before final merge. |
|
Oh FFS race condition on concurrent_tests.js, you gotta be kidding me... |
6749e94
to
b1c6144
|
OK, upstream version bumped, race condition in tests fixed, this should be good to merge. |
feat(profile): Add oauth-authenticated /account/profile endpoint.
A preliminary suggestion for how /account/profile might work, posting for feedback. Needs docs before final merge. @seanmonstar care to take a look?
Fixes #1053.