refactor(api-service): API auth exclusively in API service; cleanup WIP #7640
Conversation
✅ Deploy Preview for dev-web-novu ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for dashboard-v2-novu-staging ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| import { SwitchEnvironmentCommand } from './usecases/switch-environment/switch-environment.command'; | ||
| import { SwitchEnvironment } from './usecases/switch-environment/switch-environment.usecase'; | ||
| import { SwitchOrganizationCommand } from './usecases/switch-organization/switch-organization.command'; | ||
| import { SwitchOrganization } from './usecases/switch-organization/switch-organization.usecase'; | ||
| import { AuthService } from './services/auth.service'; |
There was a problem hiding this comment.
These services are used only in API, no reason to have them in app-generic
|
|
||
| @Injectable() | ||
| export class RootEnvironmentGuard implements CanActivate { | ||
| constructor(@Inject(forwardRef(() => AuthService)) private authService: AuthService) {} |
There was a problem hiding this comment.
No need to use this magic if it lives in the same package
| } from '@novu/shared'; | ||
| import { IAuthService } from './auth.service.interface'; | ||
| import { AuthProviderEnum, AuthenticateContext, ISubscriberJwt, UserSessionData } from '@novu/shared'; | ||
| import { IAuthService } from '@novu/application-generic'; |
There was a problem hiding this comment.
This one interface needs to stay in app-generic because its reused in EE auth.
We need to have common auth interface for private and public authentication so it makes sense.
| @@ -90,7 +89,6 @@ export class WorkflowController { | |||
| } | |||
|
|
|||
| @Put(':workflowId/sync') | |||
| @UseGuards(UserAuthGuard) | |||
There was a problem hiding this comment.
If there is a @UserAuthentication() on top of controller, doing @UseGuards(UserAuthGuard) on each method is redundant, because @UserAuthentication() already contains that guard inside it
| @@ -16,6 +16,7 @@ | |||
| "@handlebars/parser": "^2.1.0", | |||
| "@novu/application-generic": "workspace:*", | |||
| "@novu/ee-dal": "workspace:*", | |||
| "@novu/ee-auth": "workspace:*", | |||
There was a problem hiding this comment.
@novu/ee-translation should use authentication mechanisms directly from @novu/ee-auth package same as @novu/ee-billing
@novu/ee-translation will always be imported only in enterprise mode, why using UserAuthGuard, which insides decides conditionally between EEUserAuthGuard and CommunityUserAuthGuard, when you always know it will be EEUserAuthGuard
|
|
||
| @Controller('/bridge') | ||
| @UseInterceptors(ClassSerializerInterceptor) | ||
| @UserAuthentication() |
There was a problem hiding this comment.
This is equal when used on top of controller
@UserAuthGuard() === @UserAuthentication()
which is equal to using @UseGuards(UserAuthGuard) on each controller method
I used @UserAuthentication() because its used already on 80% of controllers, whereas @UserAuthGuard() was used on the rest 20%
ChmaraX
changed the title
What changed? Why was the change needed?
Screenshots
Expand for optional sections
Related enterprise PR
Special notes for your reviewer