Skip to content

Feat: Add dashboard router to store Dashboard User Settings #7588

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

Closed
wants to merge 4 commits into from

Conversation

dblythy
Copy link
Member

@dblythy dblythy commented Sep 21, 2021

New Pull Request Checklist

Issue Description

Currently, creating a Parse Server user has to be done via command line.

Related issue: #7479

Approach

Creates endpoint to signup and login as a dashboard user. This would have to be implemented in the dashboard. Would also allow features per user, beforeDashboardLogin hooks, etc.

Draft PR to get thoughts before I work on Dashboard implementation.

TODOs before merging

  • Add test cases
  • Add entry to changelog

@parse-github-assistant
Copy link

Thanks for opening this pull request!

  • 🎉 We are excited about your hands-on contribution!

@codecov
Copy link

codecov bot commented Sep 21, 2021

Codecov Report

Merging #7588 (b688cc8) into master (c3da290) will decrease coverage by 9.89%.
The diff coverage is 86.48%.

❗ Current head b688cc8 differs from pull request most recent head 5385a48. Consider uploading reports for the commit 5385a48 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7588      +/-   ##
==========================================
- Coverage   93.94%   84.05%   -9.90%     
==========================================
  Files         181      182       +1     
  Lines       13281    13355      +74     
==========================================
- Hits        12477    11225    -1252     
- Misses        804     2130    +1326     
Impacted Files Coverage Δ
src/rest.js 98.86% <ø> (ø)
src/Controllers/DatabaseController.js 92.27% <33.33%> (-1.44%) ⬇️
src/Routers/DashboardRouter.js 87.69% <87.69%> (ø)
src/Controllers/SchemaController.js 96.98% <100.00%> (-0.38%) ⬇️
src/ParseServer.js 90.05% <100.00%> (+0.05%) ⬆️
src/RestQuery.js 95.74% <100.00%> (+0.03%) ⬆️
...dapters/Storage/Postgres/PostgresStorageAdapter.js 2.47% <0.00%> (-92.90%) ⬇️
src/Adapters/Cache/RedisCacheAdapter.js 12.28% <0.00%> (-75.44%) ⬇️
src/Adapters/Storage/Postgres/PostgresClient.js 5.00% <0.00%> (-65.00%) ⬇️
src/batch.js 75.86% <0.00%> (-17.25%) ⬇️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3da290...5385a48. Read the comment docs.

@dblythy
Copy link
Member Author

dblythy commented Sep 21, 2021

Related: parse-community/parse-dashboard#1618

@dblythy
Copy link
Member Author

dblythy commented Sep 22, 2021

I'm thinking with our focus on security for V5, it could be good to have a beforeDashboardLogin trigger that can allow a developer can add their own auth flow to dashboard logins

@mtrezza
Copy link
Member

mtrezza commented Sep 22, 2021

Do we need an own trigger for that, or should dashboard login be handled the same (and invoke the same triggers) as any other user login?

@dblythy
Copy link
Member Author

dblythy commented Sep 22, 2021

I would propose a different trigger, as a dashboard user would have different column requirements and handling to a standard Parse app user.

@mtrezza
Copy link
Member

mtrezza commented Sep 22, 2021

What do you mean by column requirements?

@dblythy
Copy link
Member Author

dblythy commented Sep 22, 2021

Well if you're using the default login flow, any user that could login to your app, could log into your dashboard. Which i'm not sure is a desired outcome.

From a triggers perspective, how would a developer be able to differentiate between an admin logging in to the dashboard, vs the app? Perhaps req.dashboard?

I meant dashboard specific user settings, such as MFA secret, features, etc. But I guess they could be stored in a different collection as a pointer.

@mtrezza
Copy link
Member

mtrezza commented Sep 22, 2021

Well if you're using the default login flow, any user that could login to your app, could log into your dashboard. Which i'm not sure is a desired outcome.

Not necessarily, I was referring only to the trigger usage. If we found a way to reuse existing auth triggers, we wouldn't have to duplicate triggers just for dashboard.

From a triggers perspective, how would a developer be able to differentiate between an admin logging in to the dashboard, vs the app? Perhaps req.dashboard?

Yes, we can add a parameter to identify it as you suggested. Maybe we can look at this in context of privacy compliant logging. A topic that we will have to address if we want to make the dashboard a practical tool for production applications (which is currently isn't). There we will probably want to identify a client (dashboard, admin app, etc) by a UUID. Passing such a UUID can allow the server to identify not only the dashboard but any other custom admin app. Currently, the server can only differentiate by user (and SDK type if using client keys), indifferent of where the user signs in.

I meant dashboard specific user settings, such as MFA secret, features, etc. But I guess they could be stored in a different collection as a pointer.

If we move the dashboard user auth process to the server side, there may be good reasons to not build a parallel auth system with distinct rules. The dashboard can be treated as just another app. Dashboard users can be grouped with roles. It seems we have everything there. This would prepare us for dashboard feature permissions based on roles / users. This approach becomes more obvious if we look at custom admin apps, like dashboard but for app specific admin purposes. They probably already exist today for many parse servers, and they are likely also using the existing user / roles mechanisms.

@dblythy dblythy closed this Mar 10, 2022
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.

2 participants