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

Revisited auth and user management endpoints #74

Merged
merged 11 commits into from
Jul 14, 2020
Merged

Conversation

psrok1
Copy link
Member

@psrok1 psrok1 commented Jul 6, 2020

Your checklist for this pull request

  • I've read the contributing guideline.
  • I've tested my changes by building and running the project, and testing changed functionality (if applicable)
  • I've added automated tests for my change (if applicable, optional)
  • I've updated documentation to reflect my change (if applicable)

REST API refactor checklist:

  • Schemas:
    • All related schemas should be moved from schema.py to proper schema/<endpoint group>.py directory
    • Schemas should have separate Request and Response classes.
    • Inheritance is evil here, don't use it too much.
    • Remember that missing key, null and empty string could have different meaning:
      • missing key is viable for update requests when we don't want to set some attributes or want to use a default
      • null is viable for nullable arguments, non-persisted in database. Can be used in the same meaning as missing key.
      • empty string can be used for persisted fields
  • Don't use obj.data.get(...) for required fields, use obj.data[...] and missing= defaults in Schema
  • Don't call db.session.add(...) for persistent object (objects that are not new, but fetched from database)
  • Whole data validation should be done by Schema's
  • Complex, reusable parts of logic should be moved to model class methods or separate util functions
  • All write actions must be properly logged

What is the new behaviour?

  • Breaking change: /auth/profile is used instead of /user/<current login> for gathering current profile information.
    • /user/* and /group/* endpoints should be used only for user/group management.
    • /user/<login> needs manage_users capability even if login matches the name of currently authenticated user
  • api_key issuer login is available for all users
  • /api_key/* checks if provided id is correct UUID identifier
  • correct /auth/register request body schema:
    image
  • /user/{login}/change_password moved from auth.py to user.py (/user prefixed, user management endpoint)
  • correct /group/<name> response schema (it returns information about single group, not all groups):
    image
  • added missing POST/PUT /group/<name> missing request body schema
  • correct response schema for PUT/DELETE /group/<name> and POST/PUT /group/<name>/member/<login>
  • correct request schema for POST/PUT /user/<login>

Test plan

  • Check if all auth/user/group management features available in frontend work correctly
  • Most things should be covered by automated tests

@psrok1 psrok1 marked this pull request as ready for review July 6, 2020 18:15
@psrok1 psrok1 requested a review from msm-code July 6, 2020 18:15
@psrok1 psrok1 added this to the 2.0.0-alpha2 milestone Jul 6, 2020
Copy link
Contributor

@BonusPlay BonusPlay left a comment

Choose a reason for hiding this comment

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

Initial pass, not tested, just styling/typing/convention review.

You said that

Inheritance is evil here, don't use it too much.

which is true, inheritance makes schemas much harder to read, but (sadly) I believe that it's the only way to have all schemas properly validated without deduplicating code.

Additionally I'd propose to add constraints on outgoing schemas. Example: UserItemResponseSchema. There's no reason not to add more constraints in there, as they might catch uninitialized variables later down the development.

resources/group.py Show resolved Hide resolved
schema/user.py Outdated Show resolved Hide resolved
schema/user.py Outdated Show resolved Hide resolved
schema/user.py Outdated Show resolved Hide resolved
schema/user.py Outdated Show resolved Hide resolved
schema/group.py Outdated Show resolved Hide resolved
schema/auth.py Outdated Show resolved Hide resolved
schema/auth.py Outdated Show resolved Hide resolved
schema/auth.py Outdated Show resolved Hide resolved
schema/auth.py Outdated Show resolved Hide resolved
@psrok1
Copy link
Member Author

psrok1 commented Jul 13, 2020

Agree that response schemas should also have non-nullable constraints.

I found that some fields missed NOT NULL on database model level, added appropriate migration.

@psrok1
Copy link
Member Author

psrok1 commented Jul 13, 2020

Migration tested using production database backup data: works well ✔️

@psrok1 psrok1 requested a review from BonusPlay July 13, 2020 18:25
Copy link
Contributor

@BonusPlay BonusPlay left a comment

Choose a reason for hiding this comment

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

Not tested, just styling/typing/convention review.

PR too big for me to approve PR (as I'm not familiar enough with project).

@BonusPlay BonusPlay requested review from BonusPlay and removed request for BonusPlay July 13, 2020 20:04
Copy link
Contributor

@BonusPlay BonusPlay left a comment

Choose a reason for hiding this comment

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

github hard, I need to approve PR otherwise it won't merge ._.

model/user.py Show resolved Hide resolved
resources/group.py Outdated Show resolved Hide resolved
resources/user.py Outdated Show resolved Hide resolved
resources/user.py Outdated Show resolved Hide resolved
resources/user.py Show resolved Hide resolved
name = fields.Str(required=True, allow_none=False)

@validates("name")
def validate_name(self, name):
Copy link
Contributor

Choose a reason for hiding this comment

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

unrelated, but can we start addding type annotations to malwarecage? :duck001:

Copy link
Member Author

Choose a reason for hiding this comment

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

Along with Pydantic instead of Marshmallow? :duck001:

tests/backend/utils.py Outdated Show resolved Hide resolved
resources/auth.py Show resolved Hide resolved
@psrok1 psrok1 merged commit c7dd4e2 into master Jul 14, 2020
@psrok1 psrok1 linked an issue Jul 14, 2020 that may be closed by this pull request
16 tasks
@psrok1 psrok1 deleted the refactor/auth-endpoints branch July 16, 2020 12:52
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.

Backend API code and documentation refactor
3 participants