Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Fix authentication #17

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

Merged
fulleni merged 5 commits into main from fix_authentication
Jul 13, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 32 additions & 20 deletions lib/src/services/auth_service.dart
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,25 +75,23 @@ class AuthService {
// For dashboard login, first validate the user exists and has permissions.
if (isDashboardLogin) {
final user = await _findUserByEmail(email);

// For dashboard login, the user must exist AND have permission.
// If either condition fails, throw the appropriate exception.
if (user == null) {
_log.warning('Dashboard login failed: User $email not found.');
throw const UnauthorizedException(
'This email address is not registered for dashboard access.',
);
}

// Use the PermissionService to check for the specific dashboard login permission.
if (!_permissionService.hasPermission(
user,
Permissions.dashboardLogin,
)) {
} else if (!_permissionService.hasPermission(user, Permissions.dashboardLogin)) {
_log.warning(
'Dashboard login failed: User ${user.id} lacks required permission (${Permissions.dashboardLogin}).',
);
throw const ForbiddenException(
'Your account does not have the required permissions to sign in.',
);
}

_log.info('Dashboard user ${user.id} verified successfully.');
}

Expand All @@ -105,11 +103,13 @@ class AuthService {
await _emailRepository.sendOtpEmail(recipientEmail: email, otpCode: code);
_log.info('Initiated email sign-in for $email, code sent.');
} on HtHttpException {
// Propagate known exceptions from dependencies
// Propagate known exceptions from dependencies or from this method's logic.
// This ensures that specific errors like ForbiddenException are not
// masked as a generic server error.
rethrow;
} catch (e) {
// Catch unexpected errors during orchestration
_log.severe('Error during initiateEmailSignIn for $email: $e');
} catch (e, s) {
// Catch unexpected errors during orchestration.
_log.severe('Error during initiateEmailSignIn for $email: $e', e, s);
throw const OperationFailedException(
'Failed to initiate email sign-in process.',
);
Expand Down Expand Up @@ -166,13 +166,25 @@ class AuthService {
// This closes the loophole where a non-admin user could request a code
// via the app flow and then use it to log into the dashboard.
if (isDashboardLogin) {
if (user.email != email) {
Copy link

@gemini-code-assist gemini-code-assist bot Jul 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This is a great security check. However, email addresses are typically treated as case-insensitive. This comparison is case-sensitive. If the user lookup in _findUserByEmail is or becomes case-insensitive, a user logging in with User@example.com might be found in the database as user@example.com, causing this check to fail incorrectly. To make this more robust, consider performing a case-insensitive comparison.

 if (user.email.toLowerCase() != email.toLowerCase()) {

// This is a critical security check. If the user found by email
// somehow has a different email than the one provided, it's a
// sign of a serious issue (like the data layer bug we fixed).
// We throw a generic error to avoid revealing information.
_log.severe(
'CRITICAL: Mismatch between requested email ($email) and found '
'user email (${user.email}) during dashboard login for user '
'ID ${user.id}.',
);
throw const UnauthorizedException('User account does not exist.');
}
if (!_permissionService.hasPermission(
user,
Permissions.dashboardLogin,
)) {
_log.warning(
'Dashboard login failed: User ${user.id} lacks required permission '
'during code verification.',
'Dashboard login failed: User ${user.id} lacks required '
'permission during code verification.',
);
throw const ForbiddenException(
'Your account does not have the required permissions to sign in.',
Expand Down Expand Up @@ -254,14 +266,14 @@ class AuthService {
'Created default UserContentPreferences for user: ${user.id}',
);
}
} on HtHttpException catch (e) {
_log.severe('Error finding/creating user for $email: $e');
throw const OperationFailedException(
'Failed to find or create user account.',
);
} catch (e) {
} on HtHttpException {
// Propagate known exceptions from dependencies or from this method's logic.
// This ensures that specific errors like ForbiddenException are not
// masked as a generic server error.
rethrow;
} catch (e, s) {
_log.severe(
'Unexpected error during user lookup/creation for $email: $e',
'Unexpected error during user lookup/creation for $email: $e', e, s,
);
throw const OperationFailedException('Failed to process user account.');
}
Expand Down
13 changes: 11 additions & 2 deletions routes/api/v1/auth/request-code.dart
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,17 @@ Future<Response> onRequest(RequestContext context) async {
);
}

// Check for the optional dashboard login flag. Default to false if not present.
final isDashboardLogin = (body['isDashboardLogin'] as bool?) ?? false;
// Check for the optional dashboard login flag. This handles both boolean
// `true` and string `"true"` values to prevent type cast errors.
// It defaults to `false` if the key is missing or the value is not
// recognized as true.
final isDashboardLoginRaw = body['isDashboardLogin'];
var isDashboardLogin = false;
if (isDashboardLoginRaw is bool) {
isDashboardLogin = isDashboardLoginRaw;
} else if (isDashboardLoginRaw is String) {
isDashboardLogin = isDashboardLoginRaw.toLowerCase() == 'true';
}
Comment on lines +50 to +56
Copy link

@gemini-code-assist gemini-code-assist bot Jul 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This logic is much more robust than the previous implementation. For improved conciseness and to leverage modern Dart features, you could consider using a switch expression. This also allows isDashboardLogin to be declared as final, which is generally preferred over var when the variable is not reassigned.

 final isDashboardLoginRaw = body['isDashboardLogin'];
 final isDashboardLogin = switch (isDashboardLoginRaw) {
 final bool b => b,
 final String s => s.toLowerCase() == 'true',
 _ => false,
 };


// Basic email format check (more robust validation can be added)
// Using a slightly more common regex pattern
Expand Down
Loading

AltStyle γ«γ‚ˆγ£γ¦ε€‰ζ›γ•γ‚ŒγŸγƒšγƒΌγ‚Έ (->γ‚ͺγƒͺγ‚ΈγƒŠγƒ«) /