-
Notifications
You must be signed in to change notification settings - Fork 19
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
Ft/utapi auth #153
Ft/utapi auth #153
Conversation
9b61383
to
3f2856e
Compare
👍 |
1 similar comment
👍 |
ba24687
to
3f2856e
Compare
LGTM 👍 |
👍 |
fd1c732
to
6b96388
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a few concerns here and there, as well as some questions.
@@ -35,27 +35,44 @@ function _findAction(service, method) { | |||
if (service === 's3') { | |||
return _actionMap[method]; | |||
} | |||
if (service === 'utapi') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else if
would make more sense. Or a switch
statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be expanding at some point so I want to be clear which service is being used. When we get beyond 3 items, we can organize with an object or map structure.
} | ||
return 'arn:aws:s3:::'; | ||
} | ||
if (service === 'utapi') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here regarding else
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
// arn:scality:utapi:::resourcetype/resource | ||
// (possible resource types are buckets, accounts or users) | ||
if (specificResource) { | ||
return `arn:scality:utapi:::${generalResource}/${specificResource}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is plain duplication if we look a few lines above. Can we get rid of that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to keep the different services separate at least for now.
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "arsenal", | |||
"version": "1.1.0", | |||
"version": "1.1.0-utapi", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really wanted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's in a drop me commit to make sure integration was picking up the right version.
@@ -1112,6 +1112,24 @@ describe('policyEvaluator', () => { | |||
}); | |||
}); | |||
|
|||
describe('policyEvaluator for utapi', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only two tests for utapi?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the wildcards, variables, conditions and operators are not different for utapi so no need to replicate the prior tests.
bf28f35
to
eafbdcd
Compare
These changes are needed to enable authorization checks for utapi requests.