-
Notifications
You must be signed in to change notification settings - Fork 5
#23 Add: Feature: Track User Account Creation and Last Login Timestamps #33
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: main
Are you sure you want to change the base?
Conversation
…n db. Login test is fixed.
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.
Pull request overview
This PR implements tracking of user account creation and last login timestamps by adding createdAt and lastLoginAt fields to the User model. The changes include database schema updates via Flyway migration V8, model and DTO modifications, and updates to the authentication service to record login timestamps.
Key Changes:
- Added timestamp tracking fields (
createdAt,lastLoginAt) to the User entity with appropriate JPA annotations - Created Flyway migration V8 to alter the users table schema
- Updated login and OAuth authentication flows to record last login timestamps
- Enhanced UserDto to expose timestamp information to API consumers
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
V8__Alter_users_table_to_add_register_login_times.sql |
Adds created_at and last_login_at columns to users table |
User.java |
Adds createdAt field with @CreationTimestamp and lastLoginAt field to the User model |
LoginRequest.java |
Adds lastLoginAt field to the login request DTO |
AuthService.java |
Updates login and OAuth methods to set lastLoginAt timestamps; includes minor formatting improvements |
UserDto.java |
Adds createdAt and lastLoginAt fields to expose timestamps via API |
AuthServiceTest.java |
Updates test to use findByUsernameIgnoreCase; removes unused imports |
AuthController.java |
Code formatting improvements (spacing in conditionals and method parameters) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| @@ -0,0 +1,2 @@ | |||
| ALTER TABLE users ADD COLUMN created_at TIMESTAMP WITH TIME ZONE NOT NULL; | |||
Copilot
AI
Dec 1, 2025
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 created_at column is marked as NOT NULL, but this migration will fail if there are any existing users in the database because they won't have a value for this column. Consider either:
- Adding a
DEFAULT NOW()orDEFAULT CURRENT_TIMESTAMPclause, or - Making the column nullable initially, then backfilling data and adding the NOT NULL constraint in a subsequent step, or
- Setting a specific timestamp as the default value for existing records
Example: ALTER TABLE users ADD COLUMN created_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT NOW();
| ALTER TABLE users ADD COLUMN created_at TIMESTAMP WITH TIME ZONE NOT NULL; | |
| ALTER TABLE users ADD COLUMN created_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT NOW(); |
| @Column(name = "created_at", nullable = false, updatable = false) | ||
| private Instant createdAt; | ||
|
|
||
| @Column(name = "last_login_at", nullable = false) |
Copilot
AI
Dec 1, 2025
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 lastLoginAt field is marked as nullable = false, but this is incorrect because:
- Users won't have a last login timestamp at the moment of registration (only after their first login)
- The database migration on line 2 of V8 correctly makes this column nullable
- Existing users from OAuth registration don't have this field set until they login
Change this to nullable = true to match the database schema and the actual behavior:
@Column(name = "last_login_at", nullable = true)| @Column(name = "last_login_at", nullable = false) | |
| @Column(name = "last_login_at", nullable = true) |
| public class LoginRequest { | ||
| private String username; | ||
| private String password; | ||
| private final Instant lastLoginAt = Instant.now(); |
Copilot
AI
Dec 1, 2025
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 lastLoginAt field should not be included in the LoginRequest DTO. This is a security and design issue because:
- Security: Clients should not be able to control server-side timestamps - this could allow timestamp manipulation
- Design: Login timestamps should be set by the server at the time of successful authentication, not by the client
- Inconsistency: The
handleGoogleOAuthmethod correctly setslastLoginAtserver-side usingInstant.now()(line 207 of AuthService)
Remove this field entirely from LoginRequest. The server should set the timestamp in the login() method using Instant.now().
| private final Instant lastLoginAt = Instant.now(); |
| authResponse.setUser(user); | ||
| authResponse.setToken(token); | ||
|
|
||
| user.setLastLoginAt(loginRequest.getLastLoginAt()); |
Copilot
AI
Dec 1, 2025
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 line uses the timestamp from the client's LoginRequest, which is a security issue and incorrect design. The server should set the timestamp at the time of successful login, not trust the client's timestamp.
Change this to:
user.setLastLoginAt(Instant.now());This matches the correct implementation in handleGoogleOAuth() at line 207.
| user.setLastLoginAt(loginRequest.getLastLoginAt()); | |
| user.setLastLoginAt(Instant.now()); |
| user.setLastLoginAt(loginRequest.getLastLoginAt()); | ||
| userRepository.save(user); |
Copilot
AI
Dec 1, 2025
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 new lastLoginAt timestamp functionality lacks test coverage. Consider adding test cases to verify:
- That
lastLoginAtis correctly set when a user logs in successfully - That
lastLoginAtis persisted to the database (verifyuserRepository.save()is called after setting the timestamp)
The existing test testLogin_success() should be updated to verify this behavior.
| user.setLastLoginAt(Instant.now()); | ||
| userRepository.save(user); |
Copilot
AI
Dec 1, 2025
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 lastLoginAt timestamp functionality in the OAuth flow lacks test coverage. Consider adding test cases to verify that lastLoginAt is correctly set and persisted during OAuth login.
Registration and login verified via Postman, dates are correctly saved and next login the lastLoginAt updates.