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

feat: v1 users and groups service layer and unit tests #468

Merged
merged 1 commit into from
May 24, 2024

Conversation

keithmanville
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@jkglasbrenner jkglasbrenner left a comment

Choose a reason for hiding this comment

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

Made my first pass and found some things to look at. A couple of general bullets to supplement the above in-line comments:

  • We should verify that all modification service methods update the last_modified_on field in the ORM object (if one exists).
  • I am having a change of mind about the logging labels, our API is very chatty by default in the logs it emits and upon reflection things we are flagged as "errors" aren't service-level errors (i.e. these aren't problems in the application logic, instead they're problems in user input/output). I think we should change most/all of these to LOGGER.debug so that people running Dioptra have the option to make the app quieter.

src/dioptra/restapi/v1/auth/service.py Outdated Show resolved Hide resolved
src/dioptra/restapi/v1/auth/service.py Outdated Show resolved Hide resolved
src/dioptra/restapi/v1/users/service.py Show resolved Hide resolved
src/dioptra/restapi/v1/users/service.py Outdated Show resolved Hide resolved
src/dioptra/restapi/v1/users/service.py Show resolved Hide resolved
src/dioptra/restapi/v1/users/service.py Outdated Show resolved Hide resolved
src/dioptra/restapi/v1/users/service.py Outdated Show resolved Hide resolved
src/dioptra/restapi/v1/groups/service.py Show resolved Hide resolved
src/dioptra/restapi/v1/groups/service.py Outdated Show resolved Hide resolved
src/dioptra/restapi/v1/groups/service.py Outdated Show resolved Hide resolved
@keithmanville keithmanville changed the title [WIP] feat: users service layer for v1 feat: v1 users service layer and unit tests May 20, 2024
@keithmanville keithmanville force-pushed the v1-users-groups-service-layer branch from 2ba5e8f to 3e8339d Compare May 20, 2024 23:36
@keithmanville keithmanville changed the title feat: v1 users service layer and unit tests feat: v1 users and groups service layer and unit tests May 20, 2024
@keithmanville keithmanville force-pushed the v1-users-groups-service-layer branch from 3e8339d to f69bf1c Compare May 20, 2024 23:42
Copy link
Collaborator

@jkglasbrenner jkglasbrenner left a comment

Choose a reason for hiding this comment

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

In addition to the inline comments, we have non-functional endpoints in the groups endpoint. We should deactivate these endpoints until they're implemented.

src/dioptra/restapi/v1/groups/service.py Outdated Show resolved Hide resolved
src/dioptra/restapi/v1/groups/service.py Outdated Show resolved Hide resolved
src/dioptra/restapi/v1/groups/service.py Outdated Show resolved Hide resolved
src/dioptra/restapi/v1/users/service.py Outdated Show resolved Hide resolved
src/dioptra/restapi/v1/groups/service.py Outdated Show resolved Hide resolved
src/dioptra/restapi/v1/users/service.py Show resolved Hide resolved
src/dioptra/restapi/v1/users/service.py Show resolved Hide resolved
src/dioptra/restapi/v1/users/controller.py Outdated Show resolved Hide resolved
src/dioptra/restapi/v1/auth/controller.py Outdated Show resolved Hide resolved
src/dioptra/restapi/v1/groups/controller.py Outdated Show resolved Hide resolved
@keithmanville keithmanville force-pushed the v1-users-groups-service-layer branch 4 times, most recently from 16adcd7 to 85ef61f Compare May 22, 2024 15:01
@jkglasbrenner
Copy link
Collaborator

Made some changes, most things are a refactor of some kind. Still need to get the docstrings updated.

This was referenced May 23, 2024
@jkglasbrenner
Copy link
Collaborator

jkglasbrenner commented May 23, 2024

@keithmanville Capturing remaining todos:

  • Update the return behavior in actions.login and the auth_account fixture
  • Ensure the docstrings are updated everywhere
  • Ensure a commit=True option is available in every create() and modify() service method

@jkglasbrenner
Copy link
Collaborator

jkglasbrenner commented May 23, 2024

@keithmanville I've gone through and updated all the docstrings I noticed were either missing or incomplete. I also added the commit argument to all of the create and modify methods. I've opted to not add it for deletes for now, since I'm a little unsure of the consequences of allowing a delete lock to be added as part of a larger database transaction.

Finally, I did a quick test of the User and Group implementations using the Swagger Docs. I tested it against a Postgres database, everything worked as expected and the records in the database looked the way they should.

I think this is ready to merge, when I have a moment later tonight I'll squash everything one more time and get this into dev before Friday.

Also, as a heads up, I ended up having to extract the load_user method into a standalone function. I noticed it was changed to accept an integer and filter by user_id. I switched it back to accepting a string that represents a UUID and filter by alternative_id:

def load_user(user_id: str) -> models.User | None:
    """Load the user associated with a provided id.

    This function is intended for use with Flask-Login. The use of the alternative ID is
    needed to support the "logout everywhere" functionality and provides a mechanism for
    expiring sessions.

    Args:
        user_id: A string containing a UUID that matches the user's alternative ID.

    Returns:
        A user object if the user is found, otherwise None.
    """
    stmt = select(models.User).filter_by(
        alternative_id=uuid.UUID(user_id), is_deleted=False
    )
    return db.session.scalars(stmt).first()

The docstring copied above explains why its necessary to do it this way. This is also necessary to match up with this method in the User ORM class:

def get_id(self) -> str:
"""Get the user's session identifier.
Returns:
The user's identifier as a string.
"""
return str(self.alternative_id)

See here in the Flask-Login documentation for more info: https://flask-login.readthedocs.io/en/latest/#alternative-tokens

This feature adds an implementation of the service layer for v1 of the
REST API for the Users, Groups, and Auth endpoints. It includes unit
tests that cover common user interactions with these resources.

The primary implementations are in `restapi/<endpoint>/service.py`. The
services are injected into controller layer in
`restapi/<endpoint>/service.py` which calls the appropriate service
layer functionality. The controller layer uses helper functions from
`restapi/utils.py` to translate ORM objects back to the response types
expected by the schema. Error handling is added to v1 routes by
registering error handlers in `restapi/errors.py`.

This feature also disables some Group functionality. At this stage a
single "public" group is created alongside the first user. All
subsequent users are added to this group. Creation of additional groups
or modifying the public group is not supported. Group creation and
sharing features will be added in a future release.

A `DIOPTRA_RESTAPI_VERSION` environment variable has been introduced to
configure either v0 or v1 of the REST API. The test environments have
been configured to handle the correct version of the API.

Some tests remain disabled by default in cases where functionality is
not yet implemented. They can be enabled with `--run-v1` pytest flag.

Co-authored-by: Paul Scemama <pscemama@mitre.org>
Co-authored-by: Andrew Hand <ahand@mitre.org>
Co-authored-by: James K. Glasbrenner <jglasbrenner@mitre.org>
@jkglasbrenner jkglasbrenner force-pushed the v1-users-groups-service-layer branch from 229b50f to b879947 Compare May 24, 2024 02:12
@jkglasbrenner jkglasbrenner merged commit f8076f8 into dev May 24, 2024
15 checks passed
@jkglasbrenner jkglasbrenner deleted the v1-users-groups-service-layer branch May 24, 2024 02:14
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.

None yet

2 participants