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

[WIP] Backend authentication RBAC refactor #497

Open
wants to merge 52 commits into
base: v2/auth
Choose a base branch
from

Conversation

arthur-rl
Copy link
Contributor

@arthur-rl arthur-rl commented Jan 9, 2024

Description

This will eventually refactor all the current backend code into use logto's RBAC system to authenticate use of the backend API depending the permissions the logged in user has.

This will also refactor the current backend file structure for improved readability, contribution and maintenance.

Routers to refactor

  • Site Router
  • Gameplay Router
  • Clip Router
  • User Router
  • Util Router
  • API Key Router
  • Analysis API Router

Issues

  • Any associated issues or features that this PR corresponds to.

Started off by refactoring the getPageData request. Also refactoring the file structure for easier maintainence/contribution
@arthur-rl arthur-rl added backend (Backend / Server) code v2 Version 2 related issues labels Jan 9, 2024
@arthur-rl arthur-rl self-assigned this Jan 9, 2024
@arthur-rl arthur-rl linked an issue Jan 12, 2024 that may be closed by this pull request
@arthur-rl
Copy link
Contributor Author

Update: completed moving over gameplay routes to new file structure.
Also note that permissions are not final and are subject to change

@ceriddenn ceriddenn marked this pull request as ready for review January 16, 2024 02:04
@ceriddenn
Copy link
Contributor

It appears atm this PR is ready for review. File structure migrations has been completed and some major security issues were addressed in regards to database queries and not requiring enough / minimal fields to determine which records to retrieve. In some instances, if you spoofed a userId, you could gain access to apikeys or other models that were not yours...

@ceriddenn ceriddenn self-assigned this Jan 16, 2024
@ceriddenn ceriddenn self-requested a review January 16, 2024 03:36
Copy link
Contributor Author

@arthur-rl arthur-rl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't let me request changes, but I have put a few comments in.

const query = await prisma.v2Account.findFirst({
where: {
providerAccountId:
logto_user.userInfo.identities[
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we assign this to a variable like providerAccountId just to clean things up a bit

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

if (!query || query == null)
await prisma.v2Account.create({
data: {
provider: Object.keys(logto_user.userInfo.identities)[0],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Run Object.keys once and assign to a variable and grab the ID needed above from this variable

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

// always update user data on sign-in
try {
const result = await prisma.account.findFirst({
const result = await prisma.v2Account.findFirst({
where: {
providerAccountId:
logto_user.userInfo.identities[
Object.keys(logto_user.userInfo.identities)[0]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above here please.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

'read:gameplay',
])
.meta({ openapi: { method: 'GET', path: '/clip' } })
.input(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the zod input/output schemas be assigned to their own variable please. See gameplay/create.ts

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to do this if the object is that small.. I think it would be viable if the object had maybe 2-3 more values but just one doesn't make sense unless this input policy would be used in more than 1 route.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would make sense to me to match the rest of the codebase, it's more of a readability/convention issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend (Backend / Server) code dependencies v2 Version 2 related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RBAC / Server Authorization
2 participants