StrafesNET/maps-service
4
Fork 4
Code Issues 23 Pull Requests 2 Packages Projects Releases Wiki Activity

Switch to using /api/session/validate for determining if the user is not logged in #34

Merged
Quaternions merged 2 commits from frontend/login into staging 2025-03-27 21:47:04 +00:00
Conversation 12 Commits 2 Files Changed 1 +24 -5
interpreterK commented 2025-03-27 01:29:43 +00:00
Member
Copy Link

My apologies for being stupid not knowing the NextJS framework fully, as I have little experience with it and its non intuitive SSR and CSR workflow

Code successfully built locally running bun run build

My apologies for being stupid not knowing the NextJS framework fully, as I have little experience with it and its non intuitive SSR and CSR workflow Code successfully built locally running `bun run build`
interpreterK requested review from Quaternions 2025-03-27 01:29:53 +00:00
Quaternions commented 2025-03-27 01:38:55 +00:00
Owner
Copy Link

Why is this checking logged_in.code === 500? Shouldn't the json just be a boolean value?

Why is this checking `logged_in.code === 500`? Shouldn't the json just be a boolean value?
interpreterK commented 2025-03-27 01:42:40 +00:00
Author
Member
Copy Link

Why is this checking logged_in.code === 500? Shouldin't the json just be a boolean value?

I used what the response would be if you're not logged in

> Why is this checking `logged_in.code === 500`? Shouldin't the json just be a boolean value? I used what the response would be if you're not logged in
Quaternions commented 2025-03-27 01:42:57 +00:00
Owner
Copy Link

Should login_check() be await login_check() in the useEffect(() => { body?

Should `login_check()` be `await login_check()` in the `useEffect(() => {` body?
Quaternions commented 2025-03-27 01:43:48 +00:00
Owner
Copy Link

Why is this checking logged_in.code === 500? Shouldin't the json just be a boolean value?

I used what the response would be if you're not logged in

Why is the not logged in response an internal server error 😭

> > Why is this checking `logged_in.code === 500`? Shouldin't the json just be a boolean value? > > I used what the response would be if you're not logged in Why is the not logged in response an internal server error 😭
interpreterK commented 2025-03-27 01:50:35 +00:00
Author
Member
Copy Link

Should login_check() be await login_check() in the useEffect(() => { body?

According to:

maps-service/web/src/app/submissions/[submissionId]/page.tsx Line 87 in 251a24efae
getSubmission()

Its valid and should work the same

> Should `login_check()` be `await login_check()` in the `useEffect(() => {` body? According to: https://git.itzana.me/StrafesNET/maps-service/src/commit/251a24efae6916bdd17fdc998d6d4d3fdc3ce3c6/web/src/app/submissions/%5BsubmissionId%5D/page.tsx#L87 Its valid and should work the same
interpreterK commented 2025-03-27 01:55:05 +00:00
Author
Member
Copy Link

Why is this checking logged_in.code === 500? Shouldin't the json just be a boolean value?

I used what the response would be if you're not logged in

Why is the not logged in response an internal server error 😭

Is it going to be changed away from a code 500 error? the message it returned along side seemed like a reasonable error message to be seeing from an API if you're not logged in so I assumed it was ok to use that,
I liked the idea of using a .code === 500 over a if (logged_in) because JS is a highly unsafe and unpredictable language

> > > Why is this checking `logged_in.code === 500`? Shouldin't the json just be a boolean value? > > > > I used what the response would be if you're not logged in > > Why is the not logged in response an internal server error 😭 Is it going to be changed away from a code 500 error? the message it returned along side seemed like a reasonable error message to be seeing from an API if you're not logged in so I assumed it was ok to use that, I liked the idea of using a `.code === 500` over a `if (logged_in)` because JS is a highly unsafe and unpredictable language
Quaternions commented 2025-03-27 02:10:11 +00:00
Owner
Copy Link

I looked into it and the reason it does that is because the openapi spec defaults to requiring security for all requests, so you need to have a valid session to check if your session is valid 💀

I don't know if there is a way to have opt-out security for requests, I have only seen opt-in.

I looked into it and the reason it does that is because the openapi spec defaults to requiring security for all requests, so you need to have a valid session to check if your session is valid 💀 I don't know if there is a way to have opt-out security for requests, I have only seen opt-in.
interpreterK added the frontend label 2025-03-27 02:26:26 +00:00
interpreterK changed title from Switch to using `/api/session/validate` for determining if the user is not logged in to WIP: Switch to using `/api/session/validate` for determining if the user is not logged in 2025-03-27 03:22:44 +00:00
Quaternions force-pushed frontend/login from 0d2cd5fb49 to 8b06a1414d 2025-03-27 03:23:03 +00:00 Compare
Quaternions commented 2025-03-27 03:26:01 +00:00
Owner
Copy Link

Working on making the validate endpoint make sense. I've discovered a way to specify opt-out security for the openapi spec (#35). I also rebased this branch top of the conflicting patch.

Working on making the validate endpoint make sense. I've discovered a way to specify opt-out security for the openapi spec (#35). I also rebased this branch top of the conflicting patch.
interpreterK commented 2025-03-27 03:34:54 +00:00
Author
Member
Copy Link

I saw itzaname's suggestion in #development about doing oauth2/login?redirect=https://maps.staging.strafes.net
Im gonna implement that here and utilize NextJS's libraries for reading the url and appending the current page url to ?redirect= instead of just https://maps.staging.strafes.net

I saw itzaname's suggestion in `#development` about doing `oauth2/login?redirect=https://maps.staging.strafes.net` Im gonna implement that here and utilize NextJS's libraries for reading the url and appending the current page url to `?redirect=` instead of just `https://maps.staging.strafes.net`
Quaternions commented 2025-03-27 04:04:04 +00:00
Owner
Copy Link

I've disabled implicit session validation inside the cookie auth handler (#36). This means that the request doesn't fail at the security stage anymore and can go ahead and check the session validity itself. This applies to all requests, but other auth requests (user, roles) with an invalid session will fail as expected, so it does not need to be pre-checked as it was.

The /api/session/validate endpoint now returns true or false with errors only occurring due to other factors.

I've disabled implicit session validation inside the cookie auth handler (#36). This means that the request doesn't fail at the security stage anymore and can go ahead and check the session validity itself. This applies to all requests, but other auth requests (user, roles) with an invalid session will fail as expected, so it does not need to be pre-checked as it was. The `/api/session/validate` endpoint now returns true or false with errors only occurring due to other factors.
Quaternions commented 2025-03-27 04:27:40 +00:00
Owner
Copy Link

Should login_check() be await login_check() in the useEffect(() => { body?

According to:

maps-service/web/src/app/submissions/[submissionId]/page.tsx Line 87 in 251a24efae
getSubmission()

Its valid and should work the same

Ok here's my theory:

  • No await = greedy futures like calling tokio::spawn(login_check) that does not wait for it to complete
  • With await = polled future like calling login_check().await that waits for it to complete

Rust wins again

> > Should `login_check()` be `await login_check()` in the `useEffect(() => {` body? > > According to: > https://git.itzana.me/StrafesNET/maps-service/src/commit/251a24efae6916bdd17fdc998d6d4d3fdc3ce3c6/web/src/app/submissions/%5BsubmissionId%5D/page.tsx#L87 > Its valid and should work the same Ok here's my theory: - No `await` = greedy futures like calling `tokio::spawn(login_check)` that does not wait for it to complete - With `await` = polled future like calling `login_check().await` that waits for it to complete Rust wins again
interpreterK commented 2025-03-27 04:48:03 +00:00
Author
Member
Copy Link

Should login_check() be await login_check() in the useEffect(() => { body?

According to:

maps-service/web/src/app/submissions/[submissionId]/page.tsx Line 87 in 251a24efae
getSubmission()

Its valid and should work the same

Ok here's my theory:

  • No await = greedy futures like calling tokio::spawn(login_check) that does not wait for it to complete
  • With await = polled future like calling login_check().await that waits for it to complete

Rust wins again

Yep, async functions without calling an await on them will also return a Promise<T>, allowing shenanigans like this:

fetch("/api/session/validate") //Promise<Response>
	.then(response => response.json())
	.then(response_json => response_json.code === 500)
	.then(equals_500 => equals_500 && redirect("https://auth.staging.strafes.net"))
	.catch(error => console.error("Error getting login status:", error))
	.finally(() => console.log("/api/session/validate has ran!"))
> > > Should `login_check()` be `await login_check()` in the `useEffect(() => {` body? > > > > According to: > > https://git.itzana.me/StrafesNET/maps-service/src/commit/251a24efae6916bdd17fdc998d6d4d3fdc3ce3c6/web/src/app/submissions/%5BsubmissionId%5D/page.tsx#L87 > > Its valid and should work the same > > Ok here's my theory: > - No `await` = greedy futures like calling `tokio::spawn(login_check)` that does not wait for it to complete > - With `await` = polled future like calling `login_check().await` that waits for it to complete > > Rust wins again Yep, async functions without calling an `await` on them will also return a `Promise<T>`, allowing shenanigans like this: ```js fetch("/api/session/validate") //Promise<Response> .then(response => response.json()) .then(response_json => response_json.code === 500) .then(equals_500 => equals_500 && redirect("https://auth.staging.strafes.net")) .catch(error => console.error("Error getting login status:", error)) .finally(() => console.log("/api/session/validate has ran!")) ```
interpreterK added 1 commit 2025-03-27 16:17:38 +00:00
web: use param ?redirect= with auth redirection and switch to a boolean check for /api/session/validate
Some checks failed
continuous-integration/drone/push Build is passing
Details
continuous-integration/drone/pr Build is failing
Details
d089872432
interpreterK changed title from WIP: Switch to using `/api/session/validate` for determining if the user is not logged in to Switch to using `/api/session/validate` for determining if the user is not logged in 2025-03-27 19:29:32 +00:00
Quaternions merged commit b02b3d205e into staging 2025-03-27 21:47:04 +00:00
Quaternions deleted branch frontend/login 2025-03-27 21:47:04 +00:00
Quaternions declined to review 2025-03-29 07:02:00 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
backend
Submissions api related
 bug
Something is not working
 duplicate
This issue or pull request already exists
 enhancement
New feature
 frontend
Website related
 help wanted
Need some help
 invalid
Something is wrong
 question
More information is needed
 validator
Map validation related
 wontfix
This won't be fixed
No Label frontend
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
aidan9382 CI dowoge ic3w0lf22 interpreterK itzaname Quaternions
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: StrafesNET/maps-service#34
Delete Branch "frontend/login"

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?