fix: prevent unauthorized account.get() call on app init (#746)#785
fix: prevent unauthorized account.get() call on app init (#746)#785Arunodoy18 wants to merge 2 commits intoAOSSIE-Org:masterfrom
Conversation
- Introduce isOwner boolean flag for clear semantic separation - Hide owner-only UI elements in public view: * Edit Profile button * Settings access button * Email verification prompts * AppBar notifications and friends icons * Email verification badge - Replace all widget.isCreatorProfile checks with isOwner flag - Maintain consistent UI structure without screen duplication - Preserve all navigation logic and business logic - Improve code readability with semantic naming Resolves AOSSIE-Org#749
Only call setUserProfileData() when a valid session exists. Uses getLoginState to check authentication before fetching profile, preventing unauthorized account.get() calls for unauthenticated users. Fixes AOSSIE-Org#746
|
🎉 Welcome @Arunodoy18!
We appreciate your contribution! 🚀 |
📝 WalkthroughWalkthroughAuthStateController.onInit() now guards setUserProfileData() calls with a login-state check to prevent 401 errors for guest users. ProfileScreen refactors scattered isCreatorProfile null checks into a new isOwner getter for cleaner owner/view mode semantics. Test coverage added for guest user login state path. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent AuthStateController from triggering an unauthorized account.get() call during app initialization when the user is not logged in (fixing #746).
Changes:
- Added an initialization guard in
AuthStateController.onInit()before callingsetUserProfileData(). - Added a unit test asserting
getLoginStatereturnsfalsewhenAccount.get()throws a 401. - Refactored
ProfileScreento use anisOwnergetter for cleaner owner/public-view branching.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
lib/controllers/auth_state_controller.dart |
Adds a login-state guard around setUserProfileData() during controller init. |
test/controllers/auth_state_controller_test.dart |
Adds a test for guest login-state behavior (401 → false). |
lib/views/screens/profile_screen.dart |
Refactors repeated isCreatorProfile checks into an isOwner getter for readability. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (await getLoginState) { | ||
| await setUserProfileData(); | ||
| } |
There was a problem hiding this comment.
This PR changes AuthStateController.onInit() behavior, but tests only cover getLoginState directly. Please add a unit test that exercises onInit() for a guest user and asserts that profile initialization is skipped (i.e., setUserProfileData() is not invoked / no user DB reads happen).
| // Restore stub for subsequent tests | ||
| when(mockAccount.get()).thenAnswer((_) => Future.value(mockUser)); |
There was a problem hiding this comment.
The stub reset at the end of this test is redundant because setUp() re-stubs mockAccount.get() before every test. Keeping it here makes the test order-dependent if future changes add setUpAll or modify setup behavior; prefer relying on setUp() (or add a tearDown()/reset() if needed).
| // Restore stub for subsequent tests | |
| when(mockAccount.get()).thenAnswer((_) => Future.value(mockUser)); |
| if (await getLoginState) { | ||
| await setUserProfileData(); | ||
| } |
There was a problem hiding this comment.
onInit now guards setUserProfileData() with getLoginState, but getLoginState itself calls account.get() (and setUserProfileData() calls account.get() again). This means the app still makes an unauthorized account.get() request for guest users (just swallowed), and authenticated users pay for two account.get() calls. Consider switching the guard to a session check that does not call account.get(), or refactor so setUserProfileData() can reuse the already-fetched appwriteUser instead of fetching again.
| if (await getLoginState) { | |
| await setUserProfileData(); | |
| } | |
| await setUserProfileData(); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/controllers/auth_state_controller.dart`:
- Around line 108-110: The guard uses getLoginState which itself calls
account.get(), causing unauthorized or duplicate account.get() calls; change the
flow so the init guard does not call account.get(): either (A) refactor
getLoginState to return a local cached boolean (e.g. _isLoggedIn) or check a
non-network auth token/session flag instead of calling account.get(), or (B)
change the init sequence to call account.get() exactly once and pass its result
into setUserProfileData; update symbols getLoginState, setUserProfileData, and
any use of account.get() so unauthenticated startup never triggers account.get()
and authenticated startup calls account.get() only once.
In `@lib/views/screens/profile_screen.dart`:
- Around line 44-47: The isOwner predicate and initState public/owner branching
are inconsistent: change the getter to align with initState by making owner be
anything not explicitly true (bool get isOwner => widget.isCreatorProfile !=
true), and add a constructor/assert invariant to guarantee widget.creator is
non-null when isCreatorProfile == true (e.g. assert(widget.isCreatorProfile !=
true || widget.creator != null)) so initState and any dereferences of
widget.creator are safe and consistent with the new predicate.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/controllers/auth_state_controller.dartlib/views/screens/profile_screen.darttest/controllers/auth_state_controller_test.dart
| if (await getLoginState) { | ||
| await setUserProfileData(); | ||
| } |
There was a problem hiding this comment.
Guard still triggers account.get() for guest init path.
At Line 108, getLoginState is used as a guard, but getLoginState itself calls account.get() (Line 190). So unauthenticated startup still performs the unauthorized account request (and authenticated startup does two account.get() calls: Line 190 and Line 200). This does not fully meet the stated objective of preventing unauthorized account.get() on init.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/controllers/auth_state_controller.dart` around lines 108 - 110, The guard
uses getLoginState which itself calls account.get(), causing unauthorized or
duplicate account.get() calls; change the flow so the init guard does not call
account.get(): either (A) refactor getLoginState to return a local cached
boolean (e.g. _isLoggedIn) or check a non-network auth token/session flag
instead of calling account.get(), or (B) change the init sequence to call
account.get() exactly once and pass its result into setUserProfileData; update
symbols getLoginState, setUserProfileData, and any use of account.get() so
unauthenticated startup never triggers account.get() and authenticated startup
calls account.get() only once.
| /// Returns true if the current user is viewing their own profile (owner mode) | ||
| /// Returns false if viewing another user's profile (public view mode) | ||
| bool get isOwner => widget.isCreatorProfile == null; | ||
|
|
There was a problem hiding this comment.
isOwner predicate can desync with init/data assumptions.
isOwner treats any non-null isCreatorProfile as public mode (Line 46), but initState only runs public-profile initialization when isCreatorProfile == true (Line 51). If isCreatorProfile is false, !isOwner branches can still dereference widget.creator! without aligned initialization/invariants.
Proposed fix (unify owner/public predicate and constructor invariant)
class ProfileScreen extends StatefulWidget {
final ResonateUser? creator;
final bool? isCreatorProfile;
ProfileScreen({super.key, this.creator, this.isCreatorProfile})
: assert(
- isCreatorProfile != true || (creator != null && creator.uid != null),
- 'creator and creator.uid are required when isCreatorProfile is true',
+ creator == null || creator.uid != null,
+ 'creator.uid is required when viewing another user profile',
);
@@
class _ProfileScreenState extends State<ProfileScreen> {
- bool get isOwner => widget.isCreatorProfile == null;
+ bool get isOwner => widget.creator == null;
@@
void initState() {
super.initState();
- if (widget.isCreatorProfile == true) {
+ if (!isOwner) {
WidgetsBinding.instance.addPostFrameCallback((_) async {
await userProfileController.initializeProfile(widget.creator!.uid!);
});
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/views/screens/profile_screen.dart` around lines 44 - 47, The isOwner
predicate and initState public/owner branching are inconsistent: change the
getter to align with initState by making owner be anything not explicitly true
(bool get isOwner => widget.isCreatorProfile != true), and add a
constructor/assert invariant to guarantee widget.creator is non-null when
isCreatorProfile == true (e.g. assert(widget.isCreatorProfile != true ||
widget.creator != null)) so initState and any dereferences of widget.creator are
safe and consistent with the new predicate.
Summary
Prevents account.get() from being called when no valid session exists.
Changes
Fixes #746
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests