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

New POST create entity endpoint - also authentication #67

Merged
merged 94 commits into from
Jan 31, 2024

Conversation

CasperWA
Copy link
Collaborator

@CasperWA CasperWA commented Jan 4, 2024

Closes #63

Note, this is based on the work done in #55, so it depends on that being merged first.


The authentication pathway has been updated to utilize OAuth2 from SINTEF's GitLab instance.
Whether a user then has write access or not to the entities database backend depends on whether a user is a member of a particular group and has a sufficiently high role in said group.

Copy link

codecov bot commented Jan 5, 2024

Codecov Report

Attention: 72 lines in your changes are missing coverage. Please review.

Comparison is base (22dae4f) 98.22% compared to head (786efe4) 91.66%.

Files Patch % Lines
dlite_entities_service/service/security.py 61.66% 23 Missing ⚠️
dlite_entities_service/service/backend/mongodb.py 88.11% 17 Missing ⚠️
dlite_entities_service/service/config.py 80.85% 9 Missing ⚠️
dlite_entities_service/cli/_utils/generics.py 82.50% 7 Missing ⚠️
dlite_entities_service/service/backend/__init__.py 86.48% 5 Missing ⚠️
dlite_entities_service/service/backend/backend.py 91.22% 5 Missing ⚠️
dlite_entities_service/cli/main.py 92.85% 4 Missing ⚠️
dlite_entities_service/cli/config.py 95.65% 1 Missing ⚠️
dlite_entities_service/service/routers/__init__.py 92.30% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #67      +/-   ##
==========================================
- Coverage   98.22%   91.66%   -6.57%     
==========================================
  Files          14       17       +3     
  Lines         508      984     +476     
==========================================
+ Hits          499      902     +403     
- Misses          9       82      +73     
Flag Coverage Δ
docker 71.44% <70.68%> (-26.20%) ⬇️
local 90.24% <84.99%> (-7.99%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@CasperWA CasperWA force-pushed the cwa/close-63-post-entity-endpoint branch from 40beaf7 to 2a4b037 Compare January 9, 2024 21:14
@CasperWA CasperWA force-pushed the cwa/close-63-post-entity-endpoint branch from 1c7b6b7 to 715d059 Compare January 10, 2024 09:40
The struggle now is to make them run with a TestClient against a live
MongoDB backend
@CasperWA CasperWA marked this pull request as ready for review January 29, 2024 01:19
@CasperWA
Copy link
Collaborator Author

@quaat I've implemented authentiation through SINTEF's GitLab here now, determining write access rights through membership of a specific group (as well as the role one has in the group) :)

Fix docker compose. Only run *.js file when starting mongo service.
Instruct developers to run the self-signed certificate creation script
manually.
@CasperWA CasperWA removed the request for review from DanielMarchand January 30, 2024 09:07
Wrong auth-level used for backend in create endpoint.
Make all configuration options optional to (for now) reuse it in the
CLI.
Use the path from base_url config as the root_path for the REST API
service.
Ensure pyyaml is installed together with CLI dependencies.
Copy link

@quaat quaat left a comment

Choose a reason for hiding this comment

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

This is a rather large review, and due to time constraints it is hard to go into every detail. However the overall structure, code, comments and clarity seems very good. As for features I think it will be good in future update to separate the uri/identity of the entity and the url of the hosting service, as we will need to host entities created elsewhere with different namespaces. Very good job, I look forward to using this.

Dockerfile Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
dlite_entities_service/models/auth.py Outdated Show resolved Hide resolved
dlite_entities_service/service/routers/auth.py Outdated Show resolved Hide resolved
@CasperWA
Copy link
Collaborator Author

This is a rather large review, and due to time constraints it is hard to go into every detail. However the overall structure, code, comments and clarity seems very good. As for features I think it will be good in future update to separate the uri/identity of the entity and the url of the hosting service, as we will need to host entities created elsewhere with different namespaces. Very good job, I look forward to using this.

I've opened issue #76 to follow up on this and continue any discussion on the subject matter there.

@CasperWA CasperWA merged commit 061117e into main Jan 31, 2024
9 checks passed
@CasperWA CasperWA deleted the cwa/close-63-post-entity-endpoint branch January 31, 2024 12:37
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.

Change target for the CLI upload
4 participants