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.

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

Fix sign out button #78

Closed
wants to merge 1 commit into from
Closed

Conversation

overwatcheddude
Copy link
Contributor

Currently, the "Sign out" button doesn't actually log the user out. It merely sets text.

This PR allows the user to log out using Firebase auth.

@hmellor
Copy link
Owner

hmellor commented May 20, 2024

This was actually intentional, but the comment explaining it (if there ever was one) has been lost to refactoring.

The idea here is that we don't want the user to be able to change their uid, which happens every time an anonymous user signs out and signs in again. Users are actually anonymously signed in as soon as they open the page by AutoSignIn:

export const AutoSignIn = () => {
const [user, setUser] = useState(null);
const [admin, setAdmin] = useState(false);
useEffect(() => {
const unsubscribe = onAuthStateChanged(auth, (user) => {
if (user && user.displayName) {
console.debug(`Signed-in: name=${user.displayName}, uid=${user.uid}`);
setUser(user);
// Check if user is admin
const userDocRef = doc(db, "users", user.uid);
getDoc(userDocRef).then((docSnap) => {
if (docSnap.exists() && docSnap.data().admin) {
console.debug("User is admin");
setAdmin(true);
}
});
} else {
signInAnonymously(auth);
}
});
// Clean up the onAuthStateChanged listener when the component unmounts
return () => unsubscribe();
}, []);
return { user, admin };
};

Then, when a user signs up we, actually just set their user account's displayName and their user documents name in handleSignUp:

const handleSignUp = () => {
const user = auth.currentUser;
updateProfile(user, { displayName: username });
setDoc(doc(db, "users", user.uid), { name: username, admin: "" });
console.debug(`signUp() write to users/${user.uid}`);
setValid("is-valid");
setTimeout(() => {
closeModal();
setValid("");
}, 1000);
};

If I accept this PR then it means that a new account is created every every time a user signs out (I believe AutoSignIn will create one as soon as their auth state changes when signing out), meaning that one person might end up with many accounts. This could be a potential attack vector if a malicious actor wanted to create thousands of users and run up your Firebase bill.

What do you think?

@overwatcheddude
Copy link
Contributor Author

Hello Harry,

Thank you for the auction website, it is the only viable free and open source solution available. Also, your detailed explanation on the PR is appreciated.

I initially thought that this would be a simple fix, similar to the cancel button fix, but I guess it is a bit more than that.

Indeed, this PR will allow malicious actors to easily create multiple accounts and run up the Firebase bill, as you mentioned. However, breaking the "Sign out" button to prevent abuse may not be the best solution.

If you reject this PR, then:

  • The "Sign out" button will remain broken.
  • Abuse will still occur because bad actors can still create multiple accounts by opening new browser sessions (such as using incognito mode).

@hmellor hmellor closed this Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants