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

Add user registration events #87

Merged
merged 53 commits into from
Oct 30, 2022
Merged

Conversation

TheAndrewJackson
Copy link
Contributor

@TheAndrewJackson TheAndrewJackson commented Oct 27, 2022

Closes #82

Changes

  • Add UserRegistrationEvent route and model to the api
    • Add tests for new schema
  • Add UserRegistrationEvent to sdk
  • Update makefile and docker compose file

@TheAndrewJackson TheAndrewJackson marked this pull request as ready for review October 27, 2022 19:48
@TheAndrewJackson TheAndrewJackson self-assigned this Oct 27, 2022
@PSalant726 PSalant726 added the enhancement New feature or request label Oct 27, 2022
Copy link
Contributor

@PSalant726 PSalant726 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 my below comments, it's important to keep the docs updated. Can you comb through https://github.com/ethyca/fideslog/blob/main/README.md and https://github.com/ethyca/fideslog/blob/main/fideslog/sdk/python/README.md, and make any necessary changes to those files as well?

docker-compose.yml Outdated Show resolved Hide resolved
fideslog/api/models/models.py Outdated Show resolved Hide resolved
fideslog/api/models/models.py Outdated Show resolved Hide resolved
fideslog/api/database/data_access.py Outdated Show resolved Hide resolved
fideslog/api/database/data_access.py Outdated Show resolved Hide resolved
fideslog/sdk/python/client.py Outdated Show resolved Hide resolved
fideslog/sdk/python/event.py Outdated Show resolved Hide resolved
fideslog/sdk/python/event.py Outdated Show resolved Hide resolved
fideslog/sdk/python/event.py Outdated Show resolved Hide resolved
fideslog/sdk/python/event.py Outdated Show resolved Hide resolved
The `FIDESLOG__DATABASE_ENCRYPTION_KEY` ENV variable should be set to a
non-default value.
NevilleS
NevilleS previously approved these changes Oct 29, 2022
Copy link
Contributor

@NevilleS NevilleS left a comment

Choose a reason for hiding this comment

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

This looks solid, thanks @TheAndrewJackson and @PSalant726. Just one or two minor docs nits.

Separately, I don't think there's an API here for us to use to fetch decrypted data from the system. Can we create a follow-up for that?


assert value.find("@") == -1, "client_id must not be identifiable"
return value
_check_in_the_past: classmethod = validator(
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to check here - these timestamps are generated server-side right? I feel like I saw them in the client code as well, and while it's likely to work fine most of the time eventually a bad system clock could be an issue...

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct - these timestamps are created during the database interactions that create/update the registrations records. The validation needs to exist on the off chance that a user tries to populate these fields manually.

fideslog/sdk/python/README.md Outdated Show resolved Hide resolved
ECS_CLUSTER_NAME: ${{ secrets.ECS_CLUSTER_NAME }}
FIDESLOG__SECURITY__ACCESS_TOKEN: ${{secrets.FIDESLOG__SECURITY__ACCESS_TOKEN}}
Copy link
Contributor

Choose a reason for hiding this comment

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

This secret has been added to the repository and the engineering 1pw vault.

@seanpreston
Copy link
Contributor

Thanks for the work here @PSalant726 + @TheAndrewJackson

Change the wording slightly to allow CLI some flexibility
Copy link
Contributor

@seanpreston seanpreston left a comment

Choose a reason for hiding this comment

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

None of the comments are blocking so I've logged them as follow-up issues.

@seanpreston seanpreston mentioned this pull request Oct 30, 2022
5 tasks
@seanpreston seanpreston merged commit 2b4affb into main Oct 30, 2022
@seanpreston seanpreston deleted the ajackson/82/user_registration branch October 30, 2022 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update fideslog with an endpoint and Snowflake table for user registration
5 participants