-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add context access rule support + tests #55
Conversation
[#179181363]
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.
Looks good overall.
My requested changes are to make sure the tests cover the common cases, and one edge case.
functions/src/index.test.ts
Outdated
it("fails if user is only a class/context member", async () => { | ||
await createS3Settings(); | ||
const resource = await createS3Resource({ | ||
accessRules: [{ type: "context", contextId: user.contextId, platformId: user.platformId } as ContextAccessRule] |
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 there a test where the accessRules include both a user rule and a context rule?
This would be a common case where a resource has both, and a student matches both.
And we'd want to make sure the student can still get credentials and update the resource.
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've added this line in my last commit:
token-service/functions/src/index.test.ts
Line 84 in 2d300ec
{type: "context", platformId: user.platformId, contextId: user.contextId}, |
That way most of the tests that check owner rule will also deal with the member rule being in the same array.
Also, I've updated other tests that focus on the context rule to add a dummy user rule with userId
pointing to another user. So, more real-life scenarios are being tested there.
@@ -48,6 +48,21 @@ const invalidMemberAccessRules: AccessRule[] = [ | |||
} | |||
]; | |||
|
|||
const validContextAccessRules: AccessRule[] = [ | |||
{ |
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.
probably should add a user rule to this array since that will be the most common setup of AccessRule.
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.
Done in the last commit.
[#179181363]
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.
Looks good.
@pjanik can you merge and deploy to token-service-staging. And figure out what the next version of token service is, then add that as a label on the story. Thanks. |
I've marked the stories with token-service-2.1.0. I've also deployed v2.1.0-pre.1 tag and updated token-service-staging. |
[#179181363]
The context access rule is not restricted to teachers. It gives access only to AWS credentials, similarly to the user access rule with
role: member
.New resources can now be created with multiple access rules specified. A single string is still supported not to break backward compatibility. Also, I've implemented simple logic to automatically add the "user" access rule if the client app specifies only "context". It's opinionated, but I can imagine a situation where someone forgets to specify the "user" rule too. And I don't see any use case where we don't want the owner to have full access to his resource.