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

NN API Route #90

Merged
merged 33 commits into from
Dec 25, 2023
Merged

NN API Route #90

merged 33 commits into from
Dec 25, 2023

Conversation

WillNilges
Copy link
Collaborator

@WillNilges WillNilges commented Dec 10, 2023

Provide a Building ID (not a BIN, we need the ID of a building already in our database). Look for a free NN among the buildings already in our DB, and assign a number. I wrote a basic test.

TODO:

  • Add more tests

@WillNilges WillNilges changed the title First stab at NN API route [DRAFT] NN API Route Dec 10, 2023
@WillNilges WillNilges changed the title [DRAFT] NN API Route NN API Route Dec 10, 2023
Copy link
Collaborator

@andybaumgar andybaumgar left a comment

Choose a reason for hiding this comment

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

Looks good. Left a few minor comments.

src/meshapi/permissions.py Outdated Show resolved Hide resolved
src/meshapi/views.py Outdated Show resolved Hide resolved
src/meshapi/tests/test_nn.py Outdated Show resolved Hide resolved
src/meshapi/tests/test_nn.py Outdated Show resolved Hide resolved
src/meshapi/permissions.py Outdated Show resolved Hide resolved
src/meshapi/urls.py Outdated Show resolved Hide resolved
src/meshapi/views.py Outdated Show resolved Hide resolved
src/meshapi/views.py Outdated Show resolved Hide resolved
src/meshapi/views.py Outdated Show resolved Hide resolved
@WillNilges
Copy link
Collaborator Author

WillNilges commented Dec 22, 2023

I still need to update the Network Number Assignment route to take an Install Number.

@WillNilges
Copy link
Collaborator Author

WillNilges commented Dec 22, 2023

The tests (and the nn route) will need to consider secondary node numbers on buildings.

They also need to be updated to take network numbers and not building IDs.

src/meshapi/models.py Outdated Show resolved Hide resolved
src/meshapi/models.py Outdated Show resolved Hide resolved
src/meshapi/models.py Show resolved Hide resolved
src/meshapi/models.py Show resolved Hide resolved
src/meshapi/views.py Show resolved Hide resolved
src/meshapi/views.py Outdated Show resolved Hide resolved
src/meshapi/views.py Outdated Show resolved Hide resolved
src/meshapi/views.py Outdated Show resolved Hide resolved
src/meshapi/views.py Outdated Show resolved Hide resolved
src/meshapi/views.py Outdated Show resolved Hide resolved
@WillNilges
Copy link
Collaborator Author

WillNilges commented Dec 23, 2023

Tests to write:

  • Search for NN w/ gaps

Do we want an option to "Force" a network number?

@WillNilges
Copy link
Collaborator Author

# Assuming you have a Building instance called 'my_building'
related_installs = Install.objects.filter(building=my_building)

Oh, duh.

src/meshapi/views.py Outdated Show resolved Hide resolved
src/meshapi/views.py Show resolved Hide resolved
src/meshapi/migrations/0001_initial.py Outdated Show resolved Hide resolved
src/meshapi/models.py Outdated Show resolved Hide resolved
src/meshapi/models.py Outdated Show resolved Hide resolved
Comment on lines 204 to 205
join_form_member.first_name = r.first_name
join_form_member.last_name = r.last_name
Copy link
Member

Choose a reason for hiding this comment

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

Flagging this again, sorry for the churn, but as I reflect on this I think we probably should make the admins be responsible for updating these if they need changing. This "malicious name editing" problem is a big security hole that we likely wouldn't even notice was being exploited (and would have a hard time repairing)

I guess if they give us an email that we already know about, we should just ignore the contents of the first_name, last_name, and phone fields for safety? I don't like it but I think it's the best we can do without making them validate their email or something (which we don't want to do for friction reasons)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I suppose so. If we're going to do that, then we probably ought to return a flag that indicates, "hey, we know who this person is in some capacity", probably if_deduplicated_on_email: bla bla bla

Copy link
Member

@Andrew-Dickinson Andrew-Dickinson Dec 24, 2023

Choose a reason for hiding this comment

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

Like return it to the caller? Doesn't that expose yet another vulnerability? 🫤

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think people would be confused if they join, get installed, three years go by, they change their email, they move, they fill out the join form again, and they get a "yup hey thanks we got you thanks very much", and then they get deadnamed in the email.

That's one example but YKWIM? If we're just gonna drop the information on the floor then we need to somehow let somebody know that we did that.

So right now we have a few options:

  • Create a new Member with every join form submission
  • Destructively update Member information deduped on email
  • Destructively update Member information deduped on email but we have some kind of validation maybe
  • Dedupe on email and throw peoples' updated information on the floor and tell them
  • Dedupe on email and throw peoples' updated information on the floor and don't tell them
  • ??? Something else :0 ???

Something to keep in mind while you think about vulnerabilities is that we'll have this gated with capchas. We have to at least do that.

Copy link
Member

Choose a reason for hiding this comment

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

I think we take this away into the issue I created: #93

src/meshapi/views.py Outdated Show resolved Hide resolved
src/meshapi/views.py Outdated Show resolved Hide resolved
@WillNilges WillNilges merged commit a40ed1c into main Dec 25, 2023
3 checks passed
@WillNilges WillNilges deleted the willnilges/nn branch February 19, 2024 02:03
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.

3 participants