-
Notifications
You must be signed in to change notification settings - Fork 29
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
(DOCSP-38891) Refactor to support email/password auth #202
(DOCSP-38891) Refactor to support email/password auth #202
Conversation
- Rename /node-server/ to /express-server/
- Minor clean up
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.
Nice start! I've got a few suggestions for some minor tweaks, but I think overall it's working well and it's nice to have this ready to go - good job!
@@ -6,7 +6,7 @@ | |||
"name": "readAndWriteAll", | |||
"apply_when": { | |||
"%%user.type": { |
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.
Engineering pointed out in one of my docs PRs that we can replace this expression with something simpler:
"%%user.type": "edge"
other/edge-server/backend/data_sources/mongodb-atlas/todo/Item/rules.json
Outdated
Show resolved
Hide resolved
other/edge-server/client/.gitignore
Outdated
@@ -23,3 +23,5 @@ | |||
npm-debug.log* | |||
yarn-debug.log* | |||
yarn-error.log* | |||
|
|||
docker-compose.yml |
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 don't think we need this in the .gitignore, because with the new edgectl
stuff, this file will get written somewhere outside of this directory.
other/edge-server/client/README.md
Outdated
3. Install dependencies and start the Node.js Express server. | ||
4. Install dependencies and start the React server. | ||
1. Create an App Services App based on the `edge-server.todo` template app. | ||
2. Install Edge Server dependencies |
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.
The install script handles prompting users to install missing dependencies (I think) so I think we can remove this step from the README.
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.
Nice! That's good DX. :)
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.
Broadly, we should probably add something to the README about how disabling auth is required to use the anonymous auth type. Maybe a step in the quick start? (i.e. the edgectl config --insecure-disable-auth=true
)
I also wonder if we should mention that if you're running more than one Edge Server instance on the host hardware, you can't use the npm run start
command and must instead start all of the servers separately. (Because you'll need to start the Edge Server with a --profile
flag.)
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.
disabling auth is not actually required to use anonymous auth. if you have anonymous auth provider enabled on the cloud, you'll get to use anonymous auth locally. I think thats a reasonable thing to enable by default for this template app
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.
regarding --profile -- I'm going to try to release an update to edgectl that wont require the --profile on init, and just automatically creates a new one when you try to init
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.
@sudssm, can you help me understand the use case for --insecure-disable-auth=true
? Is it only used if people want to completely avoid a dependency on App Services?
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.
yeah, if you dont want to enable anonymous auth in the UI, then you can use this. its meant to speed up the time to connecting to the edge server via mongosh
though keep in mind there is a bug with anonymous auth right now
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.
Yeah, as mentioned in Slack, we'll stick with what we've got until the anonymous auth bug is resolved. Thank you!
|
||
This is a MERN stack that connects to the Edge Server instead of Atlas. | ||
In this version of the example app, MongoDB wire protocol connections connect | ||
directly to Edge Server without authorization. | ||
directly to Edge Server. You can use email/password authentication with an |
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.
per my previous comment, i think the app config for this should just enable anonymous auth
other/edge-server/client/README.md
Outdated
3. Start Docker | ||
4. From the project's root directory, run `npm run install-deps`. | ||
5. Add a `.env` file inside the `node-server` directory with the details | ||
3. As part of the Edge Server installation, choose to create a new user. Remember |
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.
you could skip this step if you choose to enable anon auth
- decouple Edge Server connection from Express server startup - Refactor login endpoint to handle Edge Server connection - Add logout endpoint to disconnect from Edge Server - Refactor React client to handle and persist user state
- Add 5-second refresh to React client
Co-authored-by: Dachary <[email protected]>
- More code clean up
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 really love the changes you've made here - I think the automatically reloading is a big improvement, and I like the clear communication of how it is connected to the Edge Server. The ability to log in and out with different users or auth providers is really nice, too. I've got a couple of small suggestions here, and then I think we can merge this and follow up later with more improvements.
"%%user.type": { | ||
"$eq": "mtss" | ||
} | ||
"%%user.type": "edge" | ||
}, | ||
"document_filters": { | ||
"read": true, |
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 can't comment directly on the line, but I'm wondering if we want to update the Edge Server rule to read all/write own documents, similar to the Device SDK one. I tested this write rule:
"write": {
"owner_id": "edge"
},
I was able to CRUD items we're creating in this app, but when I tried to edit an item I created from a Device SDK client, I got the expected compensating write.
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.
Good idea!
other/edge-server/client/README.md
Outdated
|
||
## Quick Start | ||
|
||
1. Create an App Services App based on the `edge-server.todo` template app, and | ||
get Edge Server enabled for your new app. | ||
1. Create an App Services App based on the `edge-server.todo` template app. |
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 link out to the docs for this, and maybe specify that you can only do this using the CLI or Admin API at this point, since there's no option to create an Edge Server app in the UI?
https://www.mongodb.com/docs/atlas/app-services/reference/template-apps/#create-a-template-app
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.
Yep. Good catch! This definitely assumes a certain level of knowledge because we don't specify how to do it.
other/edge-server/client/README.md
Outdated
|
||
## Quick Start | ||
|
||
1. Create an App Services App based on the `edge-server.todo` template app, and | ||
get Edge Server enabled for your new app. | ||
1. Create an App Services App based on the `edge-server.todo` template app. | ||
2. Download and configure the Edge Server. |
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.
There should be a step between creating an App Services App and downloading and configuring the Edge Server code for creating an Edge Server instance - i.e. https://www.mongodb.com/docs/atlas/app-services/edge-server/manage-edge-servers/#create-an-edge-server-instance
You can manage users in the App Services UI. If you don't have one yet, you | ||
can create a new user that uses email/password authentication. See the App | ||
Services [authenticate and manage users docs](https://www.mongodb.com/docs/atlas/app-services/users/) for details. | ||
|
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 think we can delete the troubleshooting section below. The "Stuck on Loading View" and "My new item" sections aren't really relevant anymore.
other/edge-server/client/README.md
Outdated
@@ -374,8 +259,23 @@ that the Edge Server is running. If you have changed the URI and/or port where | |||
the Edge Server is listening for wireprotocol connections, change the URI | |||
and/or port in your `.env` file. | |||
|
|||
## Run Multiple Edge Server Instances on a Host | |||
|
|||
If you want to run multiple Edge Servers, you can't use this project's `npm run start` script. Instead, you'll need to run each server separately, passing the `--profile` flag to differentiate the Edge Servers. |
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.
This has changed slightly with the introduction of default profiles... I suggest something like this:
If you want to run multiple Edge Servers, you can't use this project's `npm run start` script. Instead, you'll need to run each server separately, passing the `--profile` flag to differentiate the Edge Servers. | |
If your device has multiple Edge Server instances, running this project's `npm run start` script uses the default `edgectl` profile. If you want to use a different profile, you must run the Edge Server with the appropriate --profile flag, and run the express-server and react-client separately. |
@@ -374,8 +259,23 @@ that the Edge Server is running. If you have changed the URI and/or port where | |||
the Edge Server is listening for wireprotocol connections, change the URI | |||
and/or port in your `.env` file. | |||
|
|||
## Run Multiple Edge Server Instances on a Host |
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 looks like this section has gotten added in the middle of the "Troubleshooting" section. I see that Troubleshooting starts at ln 211 up above, and there's another "Troubleshooting" header at ln 291 below. I think all of the troubleshooting errors listed here no longer apply in this version of the app, so I think we can just remove all of the troubleshooting details and keep the stuff between here and ln 289 below.
(And I think since you added the listener stuff, we can remove the part of ln 288 where we say "you can reload the app" - since you don't have to do that anymore, yay!)
other/edge-server/client/README.md
Outdated
@@ -9,15 +9,15 @@ React client -> Express server (Node.js Driver) -> Edge Server -> Atlas | |||
|
|||
To use this example application, you must: | |||
|
|||
1. Create an App Services App based on the `edge-server.todo` template app, and | |||
get Edge Server enabled for your new app. | |||
1. Create an App Services App based on the `edge-server.todo` template app. |
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.
With these steps here, plus the quick start below, and then additional details as part of the quick start, it feels like we're providing three sets of instructions. I think I would just remove these steps, move the "Project npm commands" section down, and go directly into the quick start after the high-level summary stuff that is currently ln 17- ln 30.
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.
Agreed. It was definitely confusing before. I've removed that first set of instructions.
const [connectionError, setConnectionError] = useState(""); | ||
const [connectionString, setConnectionString] = useState(""); | ||
|
||
// TODO: abstract all of this one level up - to <Content>. |
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.
Is this TODO still applicable? I'm inclined to say we should not merge something to the public repo that contains a "TODO" but I don't know if you've done this work and need to remove the comment, or if this work is still outstanding.
If it still needs to be done, maybe capture the info in a Jira ticket for future work and remove it from here?
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.
Oops, nope! I forgot to remove it. Thank you for catching!
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.
Overall, LGTM! Just a couple more small tweaks for the README, and a suggestion to rename the Edge Server rule if we're going to match the restricted write permissions.
other/edge-server/backend/data_sources/mongodb-atlas/todo/Item/rules.json
Outdated
Show resolved
Hide resolved
5. Add a `.env` file inside the `node-server` directory with the details | ||
1. Create an App Services App based on the `edge-server.todo` template app. Refer | ||
to [the documentation](https://www.mongodb.com/docs/atlas/app-services/reference/template-apps/#create-a-template-app) for details. | ||
2. [Create a Service for Edge Server](https://www.mongodb.com/docs/atlas/app-services/edge-server/manage-edge-servers/#create-a-service-for-edge-server) |
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.
If they create a template app, they don't have to also create a service - this creates a Device Sync service for them. They can just go right to creating an Edge Server instance. So I think we can remove this step and update the step numbering.
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.
D'oh, that's right. Thank you!
other/edge-server/client/README.md
Outdated
5. As part of the Edge Server installation, choose to create a new user. Remember | ||
the user's email and password for later. | ||
Alternatively, you can skip user creation and disable authentication by running `edgectl config --insecure-disable-auth=true`. | ||
6. Start Docker |
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.
If you want to keep "Start Docker" in here, I'd move it before the "Install and Configure the Edge Server Instance" step. When I was just running through this with Lindsey, we found the edgectl init
step expects Docker to be running already.
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.
Ah, right. I do want to selfishly keep it. Multiple times I've forgotten to start Docker. I know the CLI will tell people they need to do it, but I think it helps to have it in the steps.
|
||
Make sure Docker is running on your local machine. Then, run the following | ||
command to start the Edge Server: | ||
1. [Create a Service for Edge Server](https://www.mongodb.com/docs/atlas/app-services/edge-server/manage-edge-servers/#create-a-service-for-edge-server) |
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.
Again, we can skip the "Create a Service" step and change the numbering on the subsequent steps if the user has created a template app.
…/rules.json Co-authored-by: Dachary <[email protected]>
This PR refactors existing code to enable an email/password auth flow. The new structure is set up to easily add more auth types in the future.
edgectl
useAdded after initial reviews:
React Client
Express server
Both