Skip to content
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.

Sign up for GitHub

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

Enhance authentication flow with loading indicators and error handlin… #2346

Merged
merged 3 commits into from
Dec 15, 2024
+129 −37
Merged

Enhance authentication flow with loading indicators and error handlin… #2346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
de2cb03
Enhance authentication flow with loading indicators and error handlin…
Mozart299 Dec 15, 2024
7de82ad
Refactor authentication handling with improved error messages and tok…
Mozart299 Dec 15, 2024
47bb227
Add JWT validation for token expiration and improve error handling in…
Mozart299 Dec 15, 2024
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
65 changes: 53 additions & 12 deletions mobile-v3/lib/main.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,11 @@ import 'package:path_provider/path_provider.dart';
import 'package:airqo/src/app/shared/pages/no_internet_banner.dart';
import 'package:loggy/loggy.dart';
import 'src/app/shared/repository/hive_repository.dart';
import 'package:jwt_decoder/jwt_decoder.dart';

void main() async {
WidgetsFlutterBinding.ensureInitialized();
await Hive.initFlutter();

Loggy.initLoggy(
logPrinter: const PrettyPrinter(),
Expand Down Expand Up @@ -157,21 +159,46 @@ class _DeciderState extends State<Decider> {
logDebug('Current connectivity state: $connectivityState');
return Stack(
children: [
FutureBuilder(
FutureBuilder<String?>(
future: HiveRepository.getData('token', HiveBoxNames.authBox),
builder: (context, snapshot) {
if (snapshot.connectionState == ConnectionState.done) {
if (!snapshot.hasData) {
logInfo('No authentication token found. Navigating to WelcomeScreen');
return WelcomeScreen();
} else {
logInfo('Authentication token found. Navigating to NavPage');
return NavPage();
}
} else {
logError('Error loading authentication state');
// Handle loading state
if (snapshot.connectionState == ConnectionState.waiting) {
return Scaffold(
body: Center(child: CircularProgressIndicator()),
);
}

// Handle errors
if (snapshot.hasError) {
logError(
'Error loading authentication state: ${snapshot.error}');
return Scaffold(
body: Center(child: Text('An Error occurred.')));
body: Center(
child: Column(
mainAxisAlignment: MainAxisAlignment.center,
children: [
Text('Unable to verify authentication status'),
SizedBox(height: 16),
ElevatedButton(
onPressed: () => setState(() {}),
child: Text('Retry'),
),
],
),
),
);
}

// Check if token exists and is not null
final token = snapshot.data;
if (!isValidToken(token)) {
logInfo(
'No authentication token found. Navigating to WelcomeScreen');
return WelcomeScreen();
} else {
logInfo('Authentication token found. Navigating to NavPage');
return NavPage();
Comment on lines +193 to +201
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Token validation implementation needs improvement

The current token validation is too basic and lacks proper JWT validation as indicated by the commented code. Here are the key issues:

  • isValidToken() only checks for null/empty values but doesn't validate the token's integrity or expiration
  • Direct navigation to NavPage() exists in login_page.dart which could potentially bypass proper token validation
  • Token storage is implemented but there's no proper validation between storage and usage

Consider:

  • Implementing proper JWT token validation including signature and expiration checks
  • Adding token refresh mechanism
  • Ensuring all routes to NavPage go through proper token validation
🔗 Analysis chain

Verify token validation flow

The token validation logic looks good, but let's verify the integration with the auth system.

Let me gather more information about the token validation implementation.

Let me check the login flow to ensure proper token handling.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for proper token usage across the codebase
echo "Checking token validation patterns..."

# Look for token retrieval patterns
rg -l "HiveRepository.getData.*token.*authBox" 

# Check for potential token validation bypasses
ast-grep --pattern 'if ($TOKEN == null || $TOKEN.isEmpty)' 

# Look for direct NavPage navigation that might bypass token validation
ast-grep --pattern 'return NavPage();'

Length of output: 320

Script:

#!/bin/bash
# Check the implementation of isValidToken function
ast-grep --pattern 'isValidToken($TOKEN) {
  $$$
}'

# Look for token validation related code with context
rg "isValidToken|token.*validation" -A 3

# Check for other NavPage navigations with context
rg "NavPage\(\)" -A 2

# Examine token retrieval implementation
rg "getData.*token.*authBox" -A 3

Length of output: 1755

Script:

#!/bin/bash
# Check login implementation and token storage
rg "login.*token" -A 5

# Look for token storage implementation
rg "setData.*token.*authBox" -A 3

# Check for token cleanup/logout
rg "removeData.*token.*authBox|clearToken|logout" -A 3

# Look for any direct NavPage navigation in auth flows
ast-grep --pattern 'onPressed: () {
  $$$
  NavPage();
  $$$
}'

Length of output: 13443

}
},
),
Expand All @@ -195,3 +222,17 @@ class _DeciderState extends State<Decider> {
);
}
}

bool isValidToken(String? token) {
if (token == null || token.isEmpty) return false;
try {
if (JwtDecoder.isExpired(token)) {
logInfo('Token has expired');
return false;
}
return true;
} catch (e) {
logError('Token validation failed', e, StackTrace.current);
return false;
}
}
Comment on lines +226 to +238
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance JWT validation checks

While the current implementation handles expiration and basic validation, consider adding more comprehensive JWT checks.

 bool isValidToken(String? token) {
   if (token == null || token.isEmpty) return false;
   try {
+    final Map<String, dynamic> decodedToken = JwtDecoder.decode(token);
+    
+    // Validate required claims
+    if (!decodedToken.containsKey('sub') || !decodedToken.containsKey('iat')) {
+      logInfo('Token missing required claims');
+      return false;
+    }
+
     if (JwtDecoder.isExpired(token)) {
       logInfo('Token has expired');
       return false;
     }
+
+    // Check if token is not used before its issuance
+    final iat = DateTime.fromMillisecondsSinceEpoch(decodedToken['iat'] * 1000);
+    if (iat.isAfter(DateTime.now())) {
+      logInfo('Token used before issuance');
+      return false;
+    }
+
     return true;
   } catch (e) {
     logError('Token validation failed', e, StackTrace.current);
     return false;
   }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
bool isValidToken(String? token) {
if (token == null || token.isEmpty) return false;
try {
if (JwtDecoder.isExpired(token)) {
logInfo('Token has expired');
return false;
}
return true;
} catch (e) {
logError('Token validation failed', e, StackTrace.current);
return false;
}
}
bool isValidToken(String? token) {
if (token == null || token.isEmpty) return false;
try {
final Map<String, dynamic> decodedToken = JwtDecoder.decode(token);
// Validate required claims
if (!decodedToken.containsKey('sub') || !decodedToken.containsKey('iat')) {
logInfo('Token missing required claims');
return false;
}
if (JwtDecoder.isExpired(token)) {
logInfo('Token has expired');
return false;
}
// Check if token is not used before its issuance
final iat = DateTime.fromMillisecondsSinceEpoch(decodedToken['iat'] * 1000);
if (iat.isAfter(DateTime.now())) {
logInfo('Token used before issuance');
return false;
}
return true;
} catch (e) {
logError('Token validation failed', e, StackTrace.current);
return false;
}
}

17 changes: 12 additions & 5 deletions mobile-v3/lib/src/app/auth/pages/login_page.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import 'package:airqo/src/meta/utils/colors.dart';
import 'package:flutter/material.dart';
import 'package:flutter_bloc/flutter_bloc.dart';
import 'package:flutter_svg/flutter_svg.dart';
import 'package:loggy/loggy.dart';

class LoginPage extends StatefulWidget {
const LoginPage({super.key});
Expand All @@ -28,8 +29,12 @@ class _LoginPageState extends State<LoginPage> {

@override
void initState() {
authBloc = context.read<AuthBloc>();
super.initState();
try {
authBloc = context.read<AuthBloc>();
} catch (e) {
logError('Failed to initialize AuthBloc: $e');
}
}

@override
Expand Down Expand Up @@ -82,7 +87,7 @@ class _LoginPageState extends State<LoginPage> {
),
),
validator: (value) {
if (value!.length == 0) {
if (value == null || value.isEmpty) {
return "This field cannot be blank.";
}
return null;
Expand All @@ -100,7 +105,7 @@ class _LoginPageState extends State<LoginPage> {
),
),
validator: (value) {
if (value!.length == 0) {
if (value == null || value.isEmpty) {
return "This field cannot be blank.";
}
return null;
Expand Down Expand Up @@ -130,8 +135,10 @@ class _LoginPageState extends State<LoginPage> {
onTap: loading
? null
: () {
if (formKey.currentState!.validate()) {
authBloc!.add(LoginUser(
final currentForm = formKey.currentState;
if (currentForm != null &&
currentForm.validate()) {
authBloc?.add(LoginUser(
emailController.text.trim(),
passwordController.text.trim()));
}
Expand Down
2 changes: 1 addition & 1 deletion mobile-v3/lib/src/app/other/theme/repository/theme_repository.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class ThemeImpl extends ThemeRepository {
await HiveRepository.saveData("themeBox", "theme", "light");
}

String newTheme = await HiveRepository.getData("theme", "themeBox");
String newTheme = await HiveRepository.getData("theme", "themeBox") ?? "light";
return newTheme;
}

Expand Down
8 changes: 5 additions & 3 deletions mobile-v3/lib/src/app/profile/repository/user_repository.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,17 @@ abstract class UserRepository extends BaseRepository {
class UserImpl extends UserRepository {
@override
Future<ProfileResponseModel> loadUserProfile() async {
String userId =
await HiveRepository.getData("userId", HiveBoxNames.authBox);
final userId = await HiveRepository.getData("userId", HiveBoxNames.authBox);

if (userId == null) {
throw Exception("User ID not found");
}

Response profileResponse =
await createAuthenticatedGetRequest("/api/v2/users/${userId}", {});

ProfileResponseModel model =
profileResponseModelFromJson(profileResponse.body);

return model;
}
}
49 changes: 39 additions & 10 deletions mobile-v3/lib/src/app/shared/repository/base_repository.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,21 @@ import 'package:http/http.dart' as http;
import 'package:http/http.dart';

class BaseRepository {
String? _cachedToken;

Future<String?> _getToken() async {
if (_cachedToken == null) {
_cachedToken = await HiveRepository.getData("token", HiveBoxNames.authBox);
}
return _cachedToken;
}

Future<Response> createPostRequest(
{required String path, dynamic data}) async {
String? token = await HiveRepository.getData("token", HiveBoxNames.authBox);
final token = await _getToken();
if (token == null) {
throw Exception('Authentication token not found');
}

String url = ApiUtils.baseUrl + path;

Expand All @@ -16,43 +28,61 @@ class BaseRepository {
Response response = await http.post(Uri.parse(url),
body: json.encode(data),
headers: {
"Authorization": "${token}",
"Authorization": "Bearer ${token}",
"Accept": "*/*",
"contentType": "application/json"
});

print(response.statusCode);

if (response.statusCode != 200) {
throw new Exception(json.decode(response.body)['message']);
final responseBody = json.decode(response.body);
final errorMessage = responseBody is Map && responseBody.containsKey('message')
? responseBody['message']
: 'An error occurred';
throw new Exception(errorMessage);
}
return response;
}

Future<Response> createGetRequest(
String path, Map<String, String> queryParams) async {
// String token = await HiveRepository.getData("token", HiveBoxNames.authBox);
String? token = await _getToken();

String url = ApiUtils.baseUrl + path;

Response response = await http
.get(Uri.parse(url).replace(queryParameters: queryParams), headers: {
Map<String, String> headers = {
"Accept": "*/*",
"Content-Type": "application/json",
});
};

if (token != null) {
headers["Authorization"] = "Bearer $token";
}

Response response = await http.get(
Uri.parse(url).replace(queryParameters: queryParams),
headers: headers);

print(response.statusCode);

if (response.statusCode != 200) {
throw new Exception(json.decode(response.body)['message']);
final responseBody = json.decode(response.body);
final errorMessage = responseBody is Map && responseBody.containsKey('message')
? responseBody['message']
: 'An error occurred';
throw new Exception(errorMessage);
}

return response;
}

Future<Response> createAuthenticatedGetRequest(
String path, Map<String, String> queryParams) async {
String token = await HiveRepository.getData("token", HiveBoxNames.authBox);
String? token = await _getToken();
if (token == null) {
throw Exception('Authentication token not found');
}

print(token);

Expand All @@ -65,7 +95,6 @@ class BaseRepository {
"Content-Type": "application/json",
});


if (response.statusCode != 200) {
throw new Exception(json.decode(response.body)['message']);
}
Expand Down
16 changes: 10 additions & 6 deletions mobile-v3/lib/src/app/shared/repository/hive_repository.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import 'package:hive/hive.dart';
import 'package:loggy/loggy.dart';

class HiveBoxNames {
const HiveBoxNames._();
Expand All @@ -14,12 +15,15 @@ class HiveRepository {
await box.put(key, value);
}

static Future<dynamic>? getData(String key, String boxName) async {
var box = await Hive.openBox(boxName);

var value = box.get(key);

return value;
static Future<String?> getData(String key, String boxName) async {
try {
var box = await Hive.openBox(boxName);
var value = box.get(key);
return value is String ? value : null;
} catch (e) {
logError('Error retrieving data from Hive: $e');
return null;
}
Comment on lines +18 to +26
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Potential null safety concerns in getData usage 🚨

The verification reveals several instances where getData's null return isn't properly handled:

  • user_repository.dart: Unsafe cast with as String which could throw at runtime
  • base_repository.dart: Dangerous non-null assertion (!) in authenticated request
  • Other usages properly handle null with null-coalescing (??) or null-aware patterns

Suggested fixes:

  • In user_repository.dart: Replace as String with null check and error handling
  • In base_repository.dart: Add proper null handling for token instead of force unwrapping
🔗 Analysis chain

Excellent improvements to type safety and error handling! 👍

The changes to getData method significantly improve robustness:

  • Type-safe return value with Future<String?>
  • Proper error handling with try-catch
  • Explicit type checking for returned values
  • Error logging for debugging

Let's verify the usage of this method across the codebase to ensure we're handling null returns:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for getData calls that might need null handling
rg "getData\(" --type dart -B 2 -A 2

Length of output: 4827

}

static Future<void>? deleteData(String boxName, String key) async {
Expand Down
8 changes: 8 additions & 0 deletions mobile-v3/pubspec.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,14 @@ packages:
url: "https://pub.dev"
source: hosted
version: "6.9.0"
jwt_decoder:
dependency: "direct main"
description:
name: jwt_decoder
sha256: "54774aebf83f2923b99e6416b4ea915d47af3bde56884eb622de85feabbc559f"
url: "https://pub.dev"
source: hosted
version: "2.0.1"
leak_tracker:
dependency: transitive
description:
Expand Down
1 change: 1 addition & 0 deletions mobile-v3/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ dependencies:
logger: ^2.5.0
connectivity_plus: ^6.1.0
flutter_loggy: ^2.0.3+1
jwt_decoder: ^2.0.1

dev_dependencies:
flutter_test:
Expand Down
Loading