-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat(mobile): allow users to change contact details #1994
Conversation
6c730b0
to
e2afc08
Compare
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.
Generally LGTM!
I think eventually we will want a bit more control over all the IDs the user might want to provide, but I like a simple approach for now.
coordinator/src/routes.rs
Outdated
@@ -160,7 +161,11 @@ pub fn router( | |||
.route("/api/orderbook/websocket", get(websocket_handler)) | |||
.route("/api/trade", post(post_trade)) | |||
.route("/api/rollover/:dlc_channel_id", post(rollover)) | |||
// Deprecated: we just keep it for backwards compatbility as otherwise old apps won't get |
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.
// Deprecated: we just keep it for backwards compatbility as otherwise old apps won't get | |
// Deprecated: we just keep it for backwards compatbility as otherwise old apps won't |
if (!isEmailValid(value)) { | ||
return 'Please enter a valid email address'; | ||
} |
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.
Should we at least validate that it is not longer than a certain number of characters? I'm not too keen on letting users insert arbitrary stuff in the database.
let client = reqwest_client(); | ||
let response = client | ||
.get(format!( | ||
"http://{}/api/users/{}", |
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.
It would be nice if we could cache this rather than sending a HTTP request every time the user opens the user settings.
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.
I postpone this to once it is an issue :)
This removes these messages (even though they are only on trace). `Received event: Event.serviceHealthUpdate(field0: Instance of 'ServiceUpdate')` Imho they don't add much value.
This is just spam in the logs.
Also, users can register with Nostr, email or telegram. Note there is no input validation so the user can put anything :)
e2afc08
to
1db3ebb
Compare
.route("/api/register", post(post_register)) | ||
.route("/api/users", post(post_register)) | ||
.route("/api/users/:trader_pubkey", get(get_user)) |
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.
🔧 That api is concerning for me. If somebody learns about a users node id they can use this api to query user data.
I propose we either validate a users signature on that api or rather put that information into the authenticated message received by the app after a successful login. (there we already verify the users signature)
.route("/api/register", post(post_register)) | ||
.route("/api/users", post(post_register)) |
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.
same here, anybody could use this api to update the contact details of a user.
resolves #1727
Simulator.Screen.Recording.-.iPhone.15.-.2024-02-09.at.08.54.01.mp4