-
Notifications
You must be signed in to change notification settings - Fork 10
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
[ADMINAPI-1117] - Admin API should provide default auth strategy for resource claims #218
Conversation
* Add Db Contexts * Add correct injection of context * Add GenericRepository * Add tenantId * Fix program.cs * Fix db provider issue * Fix mssql action * Fix github actions * Fix single tenant pg * Fix github actions * Update System.Text.Json nugget * Add not found exception Changed the way services were registered
…nstances) (#178) * Add Db Contexts * Add correct injection of context * Add GenericRepository * Add tenantId * Fix program.cs * Fix github actions * Fix single tenant pg * Fix github actions * Admin Console - Instances endpoint * Fixes after rebase. Added db migration for pg. * Fixes after rabase * Some final cleanup * Fix on program.cs file * Fixes based on comments. --------- Co-authored-by: Danny Fernandez A <dfernandeza@growthaccelerationpartners.com>
* Add Db Contexts * Add correct injection of context * Add GenericRepository * Add tenantId * Fix program.cs * Fix db provider issue * Tenants Endpoints * Add Db Contexts * Add correct injection of context * Add GenericRepository * Add tenantId * Tenants Endpoints * Resolve conflicts * get tenant changes * clean up and pgsql migration * change tenant endpoint name to tenants * change addTenant result creation url and delete tenant model --------- Co-authored-by: Danny Fernandez A <dfernandeza@growthaccelerationpartners.com>
…permissions) (#181) Add models Update context Add artifacts for mssql and postgresql Add Get and Add
…permissions) (#185) Fix problem with postgres migration
* Add UserProfile Consolidate migrations Add UserProfile services * Add UserProfile Endpoints * Add Tests
#187) * Add Steps endpoint * Rename context objects to use Mssql and PgSql instead of Sql and Pg * Update endpoints to return 404 when document is not found * Update routes to query by TenantId and by DocumentId * Update response to return document as a JsonDocument instead of a string
* Add on-pr-dockerfile-ac workflow * yml file fixes * fix image tag * Add env variables * var image tag creation
Refactor adminconsole library to support multitenancy feature.
* Add YAML and markdown file api-specification version 2.3.0 * Add the adminconsole version to generate the definition in the build.ps1 * Update and add the openapi definition for adminapi and adminconsole endpoints * Rename files to add '-pre' * Disable to push new docker image to the repository when a PR is issued --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Juan Agudelo <jagudelo@wearegap.com>
* ADMINAPI-1085 [SPIKE]: Create a shared library code for Admin API and Admin Console - Create EdFi.Ods.AdminApi.Common library - Move common components to the shared library - Remove redundant code * Fix CORS settings * Update nugets to fix vulnerabilities * Update version * Rebase from main branch * Update docker files to include the new library * Update workflow to include new library * Update yml files to include the new library * Delete FeaturesExtensions.cs Removed unused class
…st (#198) Standardize error message format to application/problem+json for Admin API client applications
* Add Update and Delete Instance feature * Add Update and Delete Instance feature * Fix GetInstanceById Fix EditInstance * Fix EditInstanceCommand * Change response code in PATCH verb --------- Co-authored-by: Juan Agudelo <jagudelo@wearegap.com>
* Fix merge conflict * Fix the instances service to create initial data
* Creates Admin Console ClaimSet.
… authorization (#206) * add new policy * enable admin console condition * remove not needed using * add EnableServerCertificateCustomValidationCallback * security extensions changes * appsettings clean up
Add OIDC config to authentication section
Creates Admin Console ClaimSet - Multitenant fix
Add parameter to appsettings Add migration to create a new column to store key and secret Update AdminConsole InitialData to create, if not exists, a vendor and an application Update Instances synchronization to include the insert of key and secret
…#219) Fix multitenat initial configuration
Add parameters to appsettings.docker.json
* ADMINAPI-1085 [SPIKE]: Create a shared library code for Admin API and Admin Console - Create EdFi.Ods.AdminApi.Common library - Move common components to the shared library - Remove redundant code * Fix CORS settings * Update nugets to fix vulnerabilities * Update version * Rebase from main branch * Update docker files to include the new library * Update workflow to include new library * Update yml files to include the new library * Delete FeaturesExtensions.cs Removed unused class
…st (#198) Standardize error message format to application/problem+json for Admin API client applications
* Add Update and Delete Instance feature * Add Update and Delete Instance feature * Fix GetInstanceById Fix EditInstance * Fix EditInstanceCommand * Change response code in PATCH verb --------- Co-authored-by: Juan Agudelo <jagudelo@wearegap.com>
* Fix merge conflict * Fix the instances service to create initial data
* Creates Admin Console ClaimSet.
… authorization (#206) * add new policy * enable admin console condition * remove not needed using * add EnableServerCertificateCustomValidationCallback * security extensions changes * appsettings clean up
Add OIDC config to authentication section
Creates Admin Console ClaimSet - Multitenant fix
Add parameter to appsettings Add migration to create a new column to store key and secret Update AdminConsole InitialData to create, if not exists, a vendor and an application Update Instances synchronization to include the insert of key and secret
…#219) Fix multitenat initial configuration
Add parameters to appsettings.docker.json
🔍 Vulnerabilities of
|
digest | sha256:511420ba123911885786a9488733853a9dcca13a5c33591036fc88df65cb016e |
vulnerabilities | |
platform | linux/amd64 |
size | 112 MB |
packages | 68 |
📦 Base Image postgres:16-alpine
also known as |
|
digest | sha256:ad47523c5154f587f0be492539c76c6c3394e8a7b02f2d86f7b8b32297a862a6 |
vulnerabilities |
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
|
Test Results6 tests 6 ✅ 0s ⏱️ Results for commit 12bb844. |
Wow, that's a lot of commits that came into this PR 😆 . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really too big to digest in a single pull request. I recommend trying to do several smaller pull requests instead.
@@ -43,6 +43,9 @@ jobs: | |||
- name: Copy admin console folder to docker context | |||
run: cp -r ../EdFi.Ods.AdminApi.AdminConsole ../../Docker/Application | |||
|
|||
- name: Copy admin api common folder to docker context | |||
run: cp -r ../EdFi.Ods.AdminApi.Common ../../Docker/Application |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine for a short term solution, but you don't need it anymore. The Dockerfile now has a capability for introducing context in a different directory. NO NEED TO CHANGE IN THIS PULL REQUEST 😄 .
Take a look at the DMS dockerfile, lines 18-20. They allow copying files from a different context, other than the current directory.
You provide the additional context at the command line:
&docker buildx build -t $dockerTagDMS -f Dockerfile . --build-context parentdir=../
Or in a docker-compose file:
services:
dms:
build:
context: ../../src/dms/
additional_contexts:
parentdir: ../../src/
on: | ||
push: | ||
branches: | ||
- adminapi23-rc.* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this change to main
?
Should there be an on-pullrequest
trigger?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is different about this workflow compared to on-pullrequest-dockerfile.yml?
<Compile Remove="Mockdata\**" /> | ||
<EmbeddedResource Remove="Mockdata\**" /> | ||
<None Remove="Mockdata\**" /> | ||
</ItemGroup> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call - thank you for excluding the mock data.
I wonder if we should simply delete the files before merging to main
?
{ | ||
public void MapEndpoints(IEndpointRouteBuilder endpoints) | ||
{ | ||
AdminApiEndpointBuilder.MapPost(endpoints, "/healthcheck", Execute) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this actually going to be GET /adminconsole/healthcheck
? Or will it be GET /healthcheck
?
public class EditInstanceRequest : IEditInstanceModel | ||
{ | ||
[Required] | ||
public ExpandoObject Document { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No change requested right now: In the future, I recommend looking at making this a JsonNode instead of a dynamic / expando. @CSR2017 can help show how we use JsonNode and JSON schema in the Data Management Service. More powerful than a simple ExpandoObject
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this? Since the onboarding wizard was removed from Admin Console. No harm including it in this PR just to keep the PR moving forward. But we should be careful about including this if we don't need to support the functionality.
Danny has replaced this PR with #221 |
No description provided.