-
Notifications
You must be signed in to change notification settings - Fork 0
EDFIAL-352-EDFIAL-353-Auth setup #7
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
base: development
Are you sure you want to change the base?
Conversation
edandylytics
left a comment
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.
Tests look good! There's a couple small issues and some cleanup.
| @Type(() => GetTenantDto) | ||
| tenant: Tenant; | ||
| @Expose() | ||
| roles: AppRoles[]; |
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.
When we put this change in, existing sessions won't have a roles property. Let's have the type reflect that. It'll help us avoid runtime errors related to assuming roles exists when it might not.
| roles: AppRoles[]; | ||
| get privileges() { | ||
| return new Set<PrivilegeKey>( | ||
| ...this.roles.flatMap((role) => (role in rolePrivileges ? rolePrivileges[role] : [])) |
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 misled you in an earlier conversation; this doesn't work quite like what we want.
We want to create a set of privileges that is the union of the privileges associated with each role the user has. new Set(...) takes an iterable and adds each value from the iterable to the set. So we want initialization to look like new Set([privilegeA, privilegeB, ...etc]).
But the spread operator on the flatMap instead results in this new Set( Set(<privileges from role 1>), Set(<privileges from role 2>), ...) Values from the first argument are added to the resulting set, which is why it appears to work. But if we had multiple roles, the privileges form other roles would be ignored.
I think if we remove the spread operator from flatMap and convert rolePrivileges[role] to an array (it starts out as a set) it'll work fine.
| }); | ||
| it('should reject requests from user without the PartnerAdmin role', async () => { | ||
| const resA = await request(app.getHttpServer()) | ||
| .post('/partners/assessments/test') |
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.
Tests look good, but now that we have the new controller, lets' move them to a separate file
edandylytics
left a comment
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.
Overall, looks good. One small change to test cleanup and we should be good. Nice job slimming down the auth logic -- much more straightforward now.
|
|
||
| afterEach(async () => { | ||
| await sessionStore.destroy(sessA.sid); | ||
| await jest.restoreAllMocks(); |
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.
We should avoid restoreAllMocks. There are some mocks used when we bootstrap the app that are needed in order to prevent the app from relying on external services. restoreAllMocks will remove those and that could lead to flaky tests.
Instead, we can do this:
// when you create the mock, save a reference to a variable initialized outside the beforeEach
mockGetBundles = jest.spyOn(EarthbeamBundlesService.prototype, 'getBundles').mockResolvedValue(allBundles);
// then, in your afterEach, use that reference to restore just the one mock this file touches
mockGetBundles.mockRestore()FWIW, we use restoreAllMocks in other test files. I did a commit yesterday to move these to the pattern above.
https://edanalytics.atlassian.net/browse/EDFIAL-352 and https://edanalytics.atlassian.net/browse/EDFIAL-353
original PR with comments https://github.com/edanalytics/runway_old/pull/107/files