Roflin/gamenight
1
0
Fork 2
Code Issues Pull Requests Projects Releases Wiki Activity

Added a user system with no proper user validation but working authorisation. #1

Merged
Roflin merged 6 commits from user-system into main 2022-04-23 13:17:32 +02:00
Conversation 8 Commits 6 Files Changed 42 +28740 -237
Roflin commented 2022-04-20 22:22:27 +02:00
Owner
Copy Link
No description provided.
Roflin force-pushed user-system from 8a41ddec86 to a1b0611abf 2022-04-20 22:23:28 +02:00 Compare
Roflin force-pushed user-system from a1b0611abf to 33ed736f0c 2022-04-20 22:24:00 +02:00 Compare
Roflin force-pushed user-system from 33ed736f0c to b8c246f521 2022-04-20 22:25:13 +02:00 Compare
Roflin force-pushed user-system from b8c246f521 to af0dcee159 2022-04-20 22:26:14 +02:00 Compare
Roflin force-pushed user-system from 5bf581356a to a3f70b8ad7 2022-04-21 13:32:12 +02:00 Compare
Roflin force-pushed user-system from a3f70b8ad7 to df8b553345 2022-04-21 18:49:25 +02:00 Compare
Roflin added 1 commit 2022-04-21 19:10:44 +02:00
yorick requested changes 2022-04-21 19:18:13 +02:00
yorick left a comment
First-time contributor
Copy Link
  • Je hebt readme.md veranderd naar backend/readme.md, maar het is niet echt een backend readme.
- Je hebt readme.md veranderd naar backend/readme.md, maar het is niet echt een backend readme.
backend/src/schema.rs
@ -0,0 +301,4 @@
pub email: String,
#[validate(length(min = 10), must_match = "password_repeat")]
pub password: String,
pub password_repeat: String,
yorick commented 2022-04-21 19:17:08 +02:00
First-time contributor
Copy Link

Ik zou password_repeat checks in de frontend doen, als je die dan toch hebt.

Ik zou password_repeat checks in de frontend doen, als je die dan toch hebt.
Roflin commented 2022-04-21 19:31:32 +02:00
Author
Owner
Copy Link

Het plan is om beide te doen, want als je dan de frontend checks omzeilt krijg je alsnog narigheid. want je kan ook gewoon tegen de api aan praten.

Het plan is om beide te doen, want als je dan de frontend checks omzeilt krijg je alsnog narigheid. want je kan ook gewoon tegen de api aan praten.
Roflin marked this conversation as resolved
backend/src/site.rs Outdated
@ -0,0 +1,42 @@
use rocket::fs::NamedFile;
use rocket::http::Method;
use rocket_cors::{AllowedHeaders, AllowedOrigins, Cors, CorsOptions};
yorick commented 2022-04-21 19:17:37 +02:00
First-time contributor
Copy Link

| ^^^^^^^^^^^ use of undeclared crate or module rocket_cors

| ^^^^^^^^^^^ use of undeclared crate or module `rocket_cors`
Roflin marked this conversation as resolved
yorick reviewed 2022-04-21 19:22:23 +02:00
frontend/public/index.html Outdated
@ -0,0 +1,43 @@
ex<!DOCTYPE html>
yorick commented 2022-04-21 19:22:23 +02:00
First-time contributor
Copy Link

ex

ex
Roflin marked this conversation as resolved
Lucus reviewed 2022-04-21 19:24:48 +02:00
backend/migrations/2022-04-17-175115_user-system/up.sql Outdated
@ -0,0 +6,4 @@
);
CREATE TABLE pwd (
id INTEGER NOT NULL PRIMARY KEY,
Lucus commented 2022-04-21 19:19:36 +02:00
First-time contributor
Copy Link

I recommend calling the column user_id in both tables. The column in pwd should also have a foreign key constraint like REFERENCES user ON DELETE CASCADE. Or it could be in the same table: Using a separate table is usually only worth it if the rows are big or the relation is not one on one.

I recommend calling the column `user_id` in both tables. The column in `pwd` should also have a foreign key constraint like `REFERENCES user ON DELETE CASCADE`. Or it could be in the same table: Using a separate table is usually only worth it if the rows are big or the relation is not one on one.
Roflin marked this conversation as resolved
backend/src/schema.rs Outdated
@ -0,0 +129,4 @@
.values((pwd::id.eq(ids[0]), pwd::password.eq(&password_hash)))
.execute(c)
})
}).await {
Lucus commented 2022-04-21 19:05:38 +02:00
First-time contributor
Copy Link

Wel mooi om de grote expression waar je hier op matcht even een naam te geven zodat de match leesbaar blijft.

Wel mooi om de grote expression waar je hier op matcht even een naam te geven zodat de match leesbaar blijft.
Roflin marked this conversation as resolved
backend/templates/register.html.hbs Outdated
@ -0,0 +16,4 @@
<input type="submit">
</form>
</body>
</html>1
Lucus commented 2022-04-21 18:58:43 +02:00
First-time contributor
Copy Link

1

1
Roflin marked this conversation as resolved
Roflin force-pushed user-system from 865ff9ac0e to 87c2e02380 2022-04-21 19:50:12 +02:00 Compare
Roflin force-pushed user-system from 87c2e02380 to 216571a45f 2022-04-21 19:50:52 +02:00 Compare
Roflin force-pushed user-system from 216571a45f to 5f73d556c6 2022-04-21 20:00:23 +02:00 Compare
Lucus reviewed 2022-04-21 20:12:11 +02:00
backend/src/api.rs
@ -0,0 +17,4 @@
use validator::ValidateArgs;
#[derive(Debug, Responder)]
pub enum ApiResponseVariant {
Lucus commented 2022-04-21 20:12:11 +02:00
First-time contributor
Copy Link

You can probably use a Result<Value, Status> for most endpoints and avoid a custom enum. I also recommend using json::Value qualified like that because Value by itself is not very descriptive.

You can probably use a `Result<Value, Status>` for most endpoints and avoid a custom enum. I also recommend using `json::Value` qualified like that because `Value` by itself is not very descriptive.
Roflin commented 2022-04-21 21:44:41 +02:00
Author
Owner
Copy Link

True, but in the future we might want to return a status on a non error condition, or return a Redirect, I understand it is a bit overkill now, but in a previous iteration I was also returning Redirects and then this becomes a nice solution imho.

True, but in the future we might want to return a status on a non error condition, or return a Redirect, I understand it is a bit overkill now, but in a previous iteration I was also returning Redirects and then this becomes a nice solution imho.
Lucus reviewed 2022-04-21 20:34:40 +02:00
backend/src/api.rs
@ -0,0 +113,4 @@
}
#[get("/gamenights")]
pub async fn gamenights(conn: DbConn, user: Option<schema::User>) -> ApiResponseVariant {
Lucus commented 2022-04-21 20:34:40 +02:00
First-time contributor
Copy Link

I think you can use a Request Guard (see https://api.rocket.rs/v0.5-rc/rocket/request/trait.FromRequest.html) to authenticate the user and role: For example, endpoints that require admin privileges could accept a non-optional Admin struct containing a user id and the request guard that generates it would only return Success if the user is logged and has the admin role.

I think you can use a Request Guard (see https://api.rocket.rs/v0.5-rc/rocket/request/trait.FromRequest.html) to authenticate the user and role: For example, endpoints that require admin privileges could accept a non-optional `Admin` struct containing a user id and the request guard that generates it would only return `Success` if the user is logged and has the admin role.
Lucus commented 2022-04-21 20:39:53 +02:00
First-time contributor
Copy Link

See also the examples under the header "Request-Local State" in the above link.

See also the examples under the header "Request-Local State" in the above link.
Lucus commented 2022-04-21 20:54:39 +02:00
First-time contributor
Copy Link

Reading more carefully I see you're already doing this, just that you're accepting an Option<User> and then checking it's not None while you could accept a User and be sure.

Reading more carefully I see you're already doing this, just that you're accepting an `Option<User>` and then checking it's not `None` while you could accept a `User` and be sure.
Roflin marked this conversation as resolved
yorick approved these changes 2022-04-21 20:36:44 +02:00
yorick reviewed 2022-04-21 20:56:16 +02:00
backend/src/schema.rs Outdated
@ -0,0 +149,4 @@
.select(user::id)
.get_results(c)
{
Ok(id) => id[0],
yorick commented 2022-04-21 20:56:16 +02:00
First-time contributor
Copy Link

generates a panic if the user does not exist

generates a panic if the user does not exist
Roflin marked this conversation as resolved
yorick requested changes 2022-04-21 20:59:24 +02:00
backend/src/api.rs Outdated
@ -0,0 +149,4 @@
{
Ok(()) => (),
Err(error) => {
return ApiResponseVariant::Value(json!(ApiResponse::error(error.to_string())))
yorick commented 2022-04-21 20:57:56 +02:00
First-time contributor
Copy Link

Value is not an error response

Value is not an error response
Roflin commented 2022-04-21 21:43:12 +02:00
Author
Owner
Copy Link

We'll it is, it's an application level error, so it's a valid request and you will get a valid http response with an "Failure" result. So that's why it returns an actual Json value

We'll it is, it's an application level error, so it's a valid request and you will get a valid http response with an "Failure" result. So that's why it returns an actual Json value
backend/src/api.rs Outdated
@ -0,0 +155,4 @@
match schema::insert_user(conn, register).await {
Ok(_) => ApiResponseVariant::Value(json!(ApiResponse::SUCCES)),
Err(err) => ApiResponseVariant::Value(json!(ApiResponse::error(err.to_string()))),
yorick commented 2022-04-21 20:58:18 +02:00
First-time contributor
Copy Link

Value is not an error response

Value is not an error response
backend/src/schema.rs
@ -0,0 +121,4 @@
.eq(&new_user.username)
.and(user::email.eq(&new_user.email)),
)
.select(user::id)
yorick commented 2022-04-21 20:58:52 +02:00
First-time contributor
Copy Link

called user_id now

called `user_id` now
Roflin marked this conversation as resolved
Roflin added 1 commit 2022-04-21 21:33:46 +02:00
Roflin added 1 commit 2022-04-21 21:50:21 +02:00
Roflin merged commit d80f705b5d into main 2022-04-23 13:17:32 +02:00
Roflin deleted branch user-system 2022-04-23 13:17:32 +02:00
Sign in to join this conversation.
Reviewers
No Reviewers
yorick
Labels
No items
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
Roflin
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: Roflin/gamenight#1
No description provided.
Delete Branch "user-system"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?