feat(auth): [#4] Implement route guards for authentication#18
Merged
feat(auth): [#4] Implement route guards for authentication#18
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 Walkthrough🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/app/auth/components/register/register.spec.ts (1)
14-29:⚠️ Potential issue | 🔴 CriticalMissing
Routerprovider — all tests in this suite will throwNullInjectorError.
Registernow hasprivate router = inject(Router)(added inregister.ts). Angular evaluates class-field injections at component creation time, soTestBed.createComponent(Register)on line 27 will immediately throw aNullInjectorErrorbecause noRouter(orRouterModule/provideRouter) is in scope.🐛 Proposed fix — add Router provision to the TestBed
+import { provideRouter } from '@angular/router'; await TestBed.configureTestingModule({ imports: [Register], providers: [ { provide: Auth, useValue: authServiceSpyObj, }, + provideRouter([]), ], }).compileComponents();Alternatively, use a spy to keep the test fully isolated and assert navigation in the success case:
+import { Router } from '@angular/router'; +const routerSpy = jasmine.createSpyObj<Router>('Router', ['navigate']); providers: [ { provide: Auth, useValue: authServiceSpyObj }, + { provide: Router, useValue: routerSpy }, ],With the spy approach, add an assertion in the success test:
expect(routerSpy.navigate).toHaveBeenCalledWith(['/dashboard']);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/app/auth/components/register/register.spec.ts` around lines 14 - 29, The tests fail with NullInjectorError because Register now injects Router via the private router = inject(Router) field; update the TestBed setup in the beforeEach to provide a Router (or a Router spy) so TestBed.createComponent(Register) can instantiate: add a provider for Router (or provide a jasmine spy object with navigate/joined methods) to the TestBed.configureTestingModule providers array and inject/mock it in the spec (reference Register, router injection, TestBed.createComponent) and, if using a spy, assert navigation (e.g., expect(routerSpy.navigate).toHaveBeenCalledWith([...])) in the success test.
🧹 Nitpick comments (7)
apps/frontend/src/app/auth/components/login/login.ts (1)
47-62: Consider adding subscription teardown viatakeUntilDestroyed.
HttpClientObservables complete after one emission so there is no real leak risk here, but if a slow request outlives the component (e.g., user navigates away manually before the server responds), thenextcallback will still fire and callrouter.navigate(). AdoptingtakeUntilDestroyedis consistent with Angular best-practice guidance for reactive code.♻️ Proposed refactor
+import { takeUntilDestroyed } from '@angular/core/rxjs-interop'; +import { DestroyRef } from '@angular/core'; export class Login { private formBuilder = inject(FormBuilder); private authService = inject(Auth); private router = inject(Router); + private destroyRef = inject(DestroyRef); ... this.authService.login(userData).subscribe({ + // automatically unsubscribes when the component is destroyed next: () => {Then chain
.pipe(takeUntilDestroyed(this.destroyRef))before.subscribe(...):- this.authService.login(userData).subscribe({ + this.authService.login(userData).pipe(takeUntilDestroyed(this.destroyRef)).subscribe({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/app/auth/components/login/login.ts` around lines 47 - 62, Wrap the authService.login(...) observable with takeUntilDestroyed(this.destroyRef) before subscribing to ensure the subscription is torn down if the component is destroyed; specifically, update the call site where authService.login(userData).subscribe({ ... }) is invoked to chain .pipe(takeUntilDestroyed(this.destroyRef)) so that the next/error handlers (which set this.errorMessage, reset this.loginForm and call this.router.navigate(['/dashboard'])) won't run after component teardown.apps/backend/src/auth/auth.module.ts (1)
7-8: LGTM —JwtStrategywired correctly.Registering
JwtStrategyas a provider is the correct pattern for@nestjs/passport; Passport discovers and initializes it automatically through NestJS DI.As a minor clarity improvement, consider specifying
defaultStrategyso the module's intent is self-documenting:♻️ Optional: explicit default strategy
- PassportModule, + PassportModule.register({ defaultStrategy: 'jwt' }),Also applies to: 13-13, 27-27
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/auth/auth.module.ts` around lines 7 - 8, Add an explicit default strategy to the PassportModule registration to make the module intent self-documenting: update the PassportModule import/registration in AuthModule to call PassportModule.register with { defaultStrategy: 'jwt' } so JwtStrategy is the explicit default; apply the same change where PassportModule is registered elsewhere in this module (references: PassportModule and JwtStrategy) to keep behavior consistent and clear.apps/frontend/src/app/auth/interceptors/token-interceptor.spec.ts (1)
14-16: Scaffold test exercises no behavior — consider adding meaningful assertions.
expect(interceptor).toBeTruthy()only verifies the function reference is defined, which is always true. The actual behaviors worth covering are: (1)Authorization: Bearer <token>is set whengetToken()returns a value, and (2) the request is forwarded unchanged when no token exists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/app/auth/interceptors/token-interceptor.spec.ts` around lines 14 - 16, Add real assertions that verify TokenInterceptor.intercept behavior: write one test where AuthService.getToken() is mocked to return a token and assert the outgoing HttpRequest contains an Authorization: Bearer <token> header and that handle() is called with the modified request; and write another test where getToken() returns null/undefined and assert handle() is called with the original request (no Authorization header). Use the existing interceptor variable, mock AuthService.getToken, spy on HttpHandler.handle to capture the request passed through, and call interceptor.intercept(request, handler) to perform the checks.apps/frontend/src/app/auth/guards/auth-guard.ts (1)
5-14: Return aUrlTreeinstead of navigating imperatively and returningfalse.Calling
router.navigate()and returningfalsetriggers two separate navigation cycles — the current one is cancelled and a new one is queued. Returning aUrlTreeintegrates the redirect cleanly into the active navigation cycle, behaves correctly when multiple guards are composed, and is the idiomatic Angular pattern since v7.2.Additionally, capturing the current URL as a
returnUrlquery param lets the login page redirect the user back after a successful sign-in.♻️ Proposed refactor
-export const authGuard: CanActivateFn = () => { +export const authGuard: CanActivateFn = (_route, state) => { const authService = inject(Auth); const router = inject(Router); if (authService.isAuthenticated()) { return true; } - router.navigate(['/auth/login']); - return false; + return router.createUrlTree(['/auth/login'], { + queryParams: { returnUrl: state.url }, + }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/app/auth/guards/auth-guard.ts` around lines 5 - 14, The guard currently imperatively calls router.navigate() and returns false; change authGuard to return a UrlTree instead so the redirect is handled in the same navigation cycle: update the CanActivateFn signature to accept (route, state) so you can access state.url, and replace router.navigate(['/auth/login']) + return false with return router.createUrlTree(['/auth/login'], { queryParams: { returnUrl: state.url } }); keep the isAuthenticated() check (Auth.isAuthenticated()) and ensure the return type remains boolean | UrlTree (or Observable/Promise thereof) for compatibility with Angular routing.apps/frontend/src/app/auth/guards/auth-guard.spec.ts (1)
10-16: Scaffold test covers no guard logic — consider adding behavioral specs.Like the interceptor spec,
expect(executeGuard).toBeTruthy()only checks that the wrapper function is defined. The two meaningful cases to cover are: (1) returnstruewhenisAuthenticated()is truthy, and (2) navigates to/auth/loginand returnsfalse/UrlTreewhen unauthenticated. Both require mockingAuthandRouterin the providers array.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/app/auth/guards/auth-guard.spec.ts` around lines 10 - 16, The current spec only asserts executeGuard is defined — add behavioral tests that mock the Auth service and Router in TestBed.configureTestingModule providers: create one test where mocked Auth.isAuthenticated() returns true and assert executeGuard returns true, and another where isAuthenticated() returns false and the mocked Router.navigate or createUrlTree is called for '/auth/login' and executeGuard returns false or an UrlTree; reference the executeGuard wrapper and the Auth and Router mocks (and the guard under test, e.g., AuthGuard) when adding these specs.apps/frontend/src/app/pages/dashboard/dashboard.ts (2)
13-14: TypeprofileDatainstead of usinganyThe
/auth/profileresponse shape is known fromJwtStrategy.validate()—{ userId, username }. Replace theanywith a concrete interface (or reuse/extend a shared DTO) to get type-safety and avoid masking future shape changes.♻️ Proposed fix
+interface ProfileData { + userId: number | string; + username: string; +} export class Dashboard implements OnInit { private httpClient = inject(HttpClient); - profileData: any = null; + profileData: ProfileData | null = null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/app/pages/dashboard/dashboard.ts` around lines 13 - 14, profileData is currently typed as any; replace it with a concrete interface that matches the /auth/profile response from JwtStrategy.validate() (e.g., { userId: string; username: string }) or import/reuse the shared DTO used by JwtStrategy.validate(); update the declaration `profileData: any = null` to use that interface (and initialize to null or undefined with the proper union type), and adjust any local usages to the new typed fields so you get compile-time checks (refer to the profileData property in dashboard.ts and the DTO/interface in JwtStrategy.validate()).
4-4: Implement theOnInitinterface
ngOnInitis defined butOnInitis neither imported nor declared on the class. This loses compile-time enforcement of the lifecycle contract.♻️ Proposed fix
-import { Component, inject } from '@angular/core'; +import { Component, inject, OnInit } from '@angular/core'; ... -export class Dashboard { +export class Dashboard implements OnInit {Also applies to: 16-16
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/app/pages/dashboard/dashboard.ts` at line 4, The class declares ngOnInit but does not implement the OnInit interface, so add OnInit to the imports from '@angular/core' and declare the component class as implementing OnInit (e.g., import { Component, inject, OnInit } and update the class signature to "export class DashboardComponent implements OnInit") so the lifecycle method is compile-time checked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/backend/src/auth/auth.controller.ts`:
- Around line 34-36: The getProfile(`@Request`() req) parameter is implicitly any,
causing no-unsafe-return/member-access errors; update the signature of
getProfile in auth.controller.ts to accept a properly typed Request payload that
matches JwtStrategy.validate()'s return shape (e.g., an object containing id and
email), then return req.user typed accordingly; ensure you use the same
interface/type used by JwtStrategy.validate() (or define and reuse a
UserJwtPayload/interface) and use the field email (not username) per the JWT
payload contract.
In `@apps/backend/src/auth/auth.service.ts`:
- Around line 48-52: The JWT payload in register and login (auth.service.ts:
payload with sub and email) does not match JwtStrategy.validate(), which expects
payload.username; update JwtStrategy.validate() to return { userId: payload.sub,
username: payload.email } (i.e., use payload.email) so req.user.username is
populated, and update any controller parameter typings that assume a username
field (e.g., the auth controller's Request user type) to reflect username is an
email string rather than a missing property.
- Around line 54-59: The tests fail because the mock UserResponseDto objects are
missing the new required access_token property; update the mocks in auth.spec.ts
and auth.controller.spec.ts to include an access_token field (a string, e.g.,
'test-token' or the existing accessToken variable) so the objects conform to
UserResponseDto; locate the mock objects referenced in those specs (the objects
that set id, name, email) and add access_token: '<appropriate string>' to each.
In `@apps/backend/src/auth/guards/jwt-auth.guard.ts`:
- Around line 1-5: Replace the insecure fallback in jwt.strategy.ts so the app
fails fast when JWT_SECRET is missing: locate the JwtStrategy constructor where
secretOrKey is set (currently using configService.get<string>('JWT_SECRET') ||
'secretKey') and change it to use ConfigService.getOrThrow<string>('JWT_SECRET')
(imported from `@nestjs/config`) so a descriptive error is thrown at startup
instead of silently using 'secretKey'; ensure the ConfigService type is
available in the JwtStrategy class and update imports accordingly.
In `@apps/backend/src/auth/jwt.strategy.ts`:
- Around line 8-13: The constructor for the JWT strategy currently uses a
hardcoded fallback for secretOrKey; instead, read the JWT_SECRET via
configService.get<string>('JWT_SECRET'), validate it is a non-empty string, and
if missing throw an error (e.g., new Error('Missing JWT_SECRET')) so the app
fails fast at startup; update the super call in constructor to pass the
validated secret value and remove the 'secretKey' fallback, referencing the
constructor and ConfigService used in jwt.strategy.ts.
- Around line 16-18: The validate method is marked async and uses an untyped
payload; remove the unnecessary async from validate and add a proper payload
interface (e.g., interface JwtPayload { sub: string; username?: string; } or
similar) and use it in the method signature (validate(payload: JwtPayload)) so
.sub and .username are typed; keep the same return shape ({ userId: payload.sub,
username: payload.username }) but ensure the method is synchronous and payload
is strongly typed (reference symbols: validate and JwtPayload).
In `@apps/frontend/src/app/auth/components/login/login.ts`:
- Around line 48-52: The success handler currently calls
this.router.navigate(['/dashboard']) and ignores its Promise, so navigation
failures are silent after this.loginForm.reset() and clearing this.errorMessage;
update the next handler (the arrow function in the subscribe success block) to
handle the Promise returned by this.router.navigate(['/dashboard']) — add
.then(...) to confirm navigation and optionally clear/reset state or .catch(...)
(or a second .then boolean check) to restore the form and set this.errorMessage
with a user-friendly message when navigation returns false or throws; reference
the next handler, this.router.navigate, this.loginForm.reset, and
this.errorMessage when implementing the change.
In `@apps/frontend/src/app/auth/interceptors/token-interceptor.ts`:
- Around line 9-16: The interceptor currently attaches the Authorization header
to every outgoing request; update the logic in token-interceptor.ts (inside the
intercept method where `token`, `req.clone`, and `next(clonedRequest)` are used)
to only add the header when the request targets your API origin — e.g. check
`token && req.url.startsWith(API_BASE_URL)` or verify same-origin (`new
URL(req.url, location.origin).origin === API_BASE_URL`) before calling
`req.clone({ setHeaders: { Authorization: ... } })` and `next(clonedRequest)`;
otherwise call `next(req)` unmodified.
In `@apps/frontend/src/app/auth/services/auth.ts`:
- Around line 42-46: The setSession() function currently silently skips when
authResult.access_token is missing and writes JWTs to localStorage; update
setSession(authResult) to throw a clear error (e.g., throw new Error("Missing
access_token")) when access_token is absent so callers (register() / login())
can handle and avoid unconditional navigation, and remove storing the access
token in localStorage (TOKEN_KEY); instead keep the short-lived access token in
an in-memory field on the Auth service (e.g., this.accessToken) and rely on a
refresh token delivered/set as an HttpOnly cookie from the server for long-lived
auth, and ensure register()/login() check for thrown errors from setSession()
before navigating to /dashboard.
- Around line 34-36: isAuthenticated currently only checks presence of a token
(TOKEN_KEY) and will return true for expired JWTs; update isAuthenticated() to
decode the stored JWT, verify the exp claim against current time, remove the
token from localStorage and return false if expired, and only return true if a
valid, unexpired token exists; reference the isAuthenticated() method and
TOKEN_KEY, ensure behavior aligns with authGuard so expired tokens cause a false
result (and trigger a redirect to /login).
In `@apps/frontend/src/app/pages/dashboard/dashboard.html`:
- Around line 2-4: Remove the debug/dev message and raw JSON dump in the
dashboard template: delete the paragraph ("If you see your profile data below,
the Interceptor works! 🎉") and the <pre>{{ profileData | json }}</pre> debug
output, and replace them with a production-ready UI that renders profileData
properly (for example a ProfileCard component or formatted fields bound to the
profileData object). Update the component/template that provides profileData so
it passes the necessary properties to the new UI elements and ensure any
test/debug-only markers are removed from the Dashboard view.
In `@apps/frontend/src/app/pages/dashboard/dashboard.spec.ts`:
- Around line 9-17: The test is missing an HttpClient provider so
fixture.detectChanges() triggers NullInjectorError due to Dashboard.ngOnInit()
making HTTP calls; update the TestBed configuration to add
provideHttpClientTesting() to imports/providers and inject HttpTestingController
in the spec, then call fixture.detectChanges() only after setting up
HttpTestingController expectations and flushing the mocked HTTP request(s) for
the Dashboard component's requests. Locate references to
TestBed.configureTestingModule, Dashboard, fixture.detectChanges(), and use
HttpTestingController to expect and flush the HTTP call(s) made by
Dashboard.ngOnInit().
In `@apps/frontend/src/app/pages/dashboard/dashboard.ts`:
- Around line 17-25: Remove the debug console.log in the HTTP success handler
and handle auth errors by injecting Angular's Router and redirecting to '/login'
on 401 responses: in the method that calls
this.httpClient.get(`${environment.apiUrl}/auth/profile`) (the next/error
handlers that set this.profileData), delete the console.log('Profile Data
received:', data) and replace the error handler so it checks err.status === 401
and calls this.router.navigate(['/login']) (otherwise log or surface other
errors), and ensure the component's constructor receives a private router:
Router so the redirect works.
In `@libs/shared-dtos/src/user/user-response.dto.ts`:
- Line 9: Remove the auth field from the generic DTO: delete the readonly
access_token! declaration from UserResponseDto and create a new AuthResponseDto
class (e.g., in libs/shared-dtos/src/auth/auth-response.dto.ts) that extends
UserResponseDto and adds readonly access_token!: string;. Update the auth flow
to use the new DTO: change any auth service return types (e.g., AuthService
methods in auth.service.ts) to return AuthResponseDto instead of
UserResponseDto, and update the frontend AuthService typings for login/register
responses to use AuthResponseDto so only authentication endpoints expose the
token.
---
Outside diff comments:
In `@apps/frontend/src/app/auth/components/register/register.spec.ts`:
- Around line 14-29: The tests fail with NullInjectorError because Register now
injects Router via the private router = inject(Router) field; update the TestBed
setup in the beforeEach to provide a Router (or a Router spy) so
TestBed.createComponent(Register) can instantiate: add a provider for Router (or
provide a jasmine spy object with navigate/joined methods) to the
TestBed.configureTestingModule providers array and inject/mock it in the spec
(reference Register, router injection, TestBed.createComponent) and, if using a
spy, assert navigation (e.g.,
expect(routerSpy.navigate).toHaveBeenCalledWith([...])) in the success test.
---
Nitpick comments:
In `@apps/backend/src/auth/auth.module.ts`:
- Around line 7-8: Add an explicit default strategy to the PassportModule
registration to make the module intent self-documenting: update the
PassportModule import/registration in AuthModule to call PassportModule.register
with { defaultStrategy: 'jwt' } so JwtStrategy is the explicit default; apply
the same change where PassportModule is registered elsewhere in this module
(references: PassportModule and JwtStrategy) to keep behavior consistent and
clear.
In `@apps/frontend/src/app/auth/components/login/login.ts`:
- Around line 47-62: Wrap the authService.login(...) observable with
takeUntilDestroyed(this.destroyRef) before subscribing to ensure the
subscription is torn down if the component is destroyed; specifically, update
the call site where authService.login(userData).subscribe({ ... }) is invoked to
chain .pipe(takeUntilDestroyed(this.destroyRef)) so that the next/error handlers
(which set this.errorMessage, reset this.loginForm and call
this.router.navigate(['/dashboard'])) won't run after component teardown.
In `@apps/frontend/src/app/auth/guards/auth-guard.spec.ts`:
- Around line 10-16: The current spec only asserts executeGuard is defined — add
behavioral tests that mock the Auth service and Router in
TestBed.configureTestingModule providers: create one test where mocked
Auth.isAuthenticated() returns true and assert executeGuard returns true, and
another where isAuthenticated() returns false and the mocked Router.navigate or
createUrlTree is called for '/auth/login' and executeGuard returns false or an
UrlTree; reference the executeGuard wrapper and the Auth and Router mocks (and
the guard under test, e.g., AuthGuard) when adding these specs.
In `@apps/frontend/src/app/auth/guards/auth-guard.ts`:
- Around line 5-14: The guard currently imperatively calls router.navigate() and
returns false; change authGuard to return a UrlTree instead so the redirect is
handled in the same navigation cycle: update the CanActivateFn signature to
accept (route, state) so you can access state.url, and replace
router.navigate(['/auth/login']) + return false with return
router.createUrlTree(['/auth/login'], { queryParams: { returnUrl: state.url }
}); keep the isAuthenticated() check (Auth.isAuthenticated()) and ensure the
return type remains boolean | UrlTree (or Observable/Promise thereof) for
compatibility with Angular routing.
In `@apps/frontend/src/app/auth/interceptors/token-interceptor.spec.ts`:
- Around line 14-16: Add real assertions that verify TokenInterceptor.intercept
behavior: write one test where AuthService.getToken() is mocked to return a
token and assert the outgoing HttpRequest contains an Authorization: Bearer
<token> header and that handle() is called with the modified request; and write
another test where getToken() returns null/undefined and assert handle() is
called with the original request (no Authorization header). Use the existing
interceptor variable, mock AuthService.getToken, spy on HttpHandler.handle to
capture the request passed through, and call interceptor.intercept(request,
handler) to perform the checks.
In `@apps/frontend/src/app/pages/dashboard/dashboard.ts`:
- Around line 13-14: profileData is currently typed as any; replace it with a
concrete interface that matches the /auth/profile response from
JwtStrategy.validate() (e.g., { userId: string; username: string }) or
import/reuse the shared DTO used by JwtStrategy.validate(); update the
declaration `profileData: any = null` to use that interface (and initialize to
null or undefined with the proper union type), and adjust any local usages to
the new typed fields so you get compile-time checks (refer to the profileData
property in dashboard.ts and the DTO/interface in JwtStrategy.validate()).
- Line 4: The class declares ngOnInit but does not implement the OnInit
interface, so add OnInit to the imports from '@angular/core' and declare the
component class as implementing OnInit (e.g., import { Component, inject, OnInit
} and update the class signature to "export class DashboardComponent implements
OnInit") so the lifecycle method is compile-time checked.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes Issue #4
This PR introduces comprehensive route protection and session management across both the frontend (Angular) and backend (NestJS) applications, ensuring that only authenticated users can access protected resources.
Backend Changes (NestJS):
Frontend Changes (Angular):
Summary by CodeRabbit