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.

Sign up for GitHub

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

Jump to bottom

[IDP-1183, partial] Revise "external groups" API endpoints #358

Merged
merged 38 commits into from
Sep 5, 2024
Merged

[IDP-1183, partial] Revise "external groups" API endpoints #358

merged 38 commits into from
Sep 5, 2024

Conversation

forevermatt
Copy link
Contributor

@forevermatt forevermatt commented Aug 28, 2024 •
edited
Loading

IDP-1183 Sync prefixed-groups from Google Sheet to ID Broker's groups_external field

Added

  • Add a way to run the personnel-sync locally, for dev and testing
  • Add an API endpoint for listing users with groups with a specific prefix
  • Add separate API endpoint for creating a user's prefixed external groups
    • This avoids the need to include the email address in the URL path for creates, which personnel-sync does not currently support.
    • This actually makes the POST behavior more typical, as you might normally POST to an endpoint for a type of resource (without a unique identifier), rather than posting to a specific resource.
  • Add tests for these various API endpoints

Changed (non-breaking)

  • Put email and app-prefix in URL for update-external-groups API
    • This would have been a breaking change, but we had not yet released that new API endpoint.
  • Use comma-delimited string for external groups in API endpoints
    • This also would have been a breaking change, if the previous version had already been released.
    • This makes it easier to sync this data with a spreadsheet (e.g. a Google Sheet), where values are just strings, not arrays.
  • Enable updating a user's external groups for an app to be an empty list

Feature PR Checklist

  • Documentation (README, local.env.dist, api.raml, etc.)
  • Tests created or updated
  • Run make composershow
  • Run make psr2

@forevermatt
Add a way to run the personnel-sync locally, for dev and testing
2a02afe 
To do so...

1. Run `docker compose run --rm externalgroupssync bash`
2. In that, run `~/.aws-lambda-rie/aws-lambda-rie ./personnel-sync`
3. In other terminal (on your host), run another
   `docker compose exec externalgroupssync bash`
4. In that, run `curl -XPOST "http://localhost:8080/2015-03-31/functions/function/invocations" -d '{}'`
   each time you want to invoke the personnel-sync.
@forevermatt
Put email and app-prefix in URL for update-external-groups API
c02e402
@forevermatt
Correct the query parameter definition in api.raml
5cb92fc
@forevermatt
Add an API endpoint for getting users with groups with a specific prefix
2cf104b
@forevermatt
Add test for listing users and external groups for a given app-prefix
2afb7fe
@forevermatt
Refactor GroupsExternalContext for reuse of some test methods
1991af6
@forevermatt
Begin implementing list-external-groups test
81d6473
@forevermatt
Add endpoint for listing users with external-groups with specific prefix
bb033db
@forevermatt forevermatt requested a review from a team August 28, 2024 13:46
@forevermatt
Use comma-delimited string for external groups in API endpoints
07cd6bb 
As an example, rather than having `["prefix-group1", "prefix-group2"]`,
it will now have `"prefix-group1,prefix-group2"` as the value for the
external groups API endpoints (both incoming, such as for updates, and
outgoing, such as in the list endpoint's response).

This makes it easier to sync this data with a spreadsheet (e.g. a Google
Sheet).
@forevermatt
Include an id of the email address too, to bypass personnel-sync bug
c8742bd 
This is intended to be temporary.
@forevermatt
Fix groups-external-list test to ignore the temporary id attribute
806b4de
@forevermatt
Enable updating a user's external groups for an app to be an empty list
615e6e3
@forevermatt
Add separate API endpoint for creating a user's prefixed external groups
c35198d 
This avoids the need to include the email address in the URL path, which
personnel-sync does not currently support.
@forevermatt
Improve the RAML documentation for POST /user/external-groups
507ac90
@forevermatt
Update the PR template to mention the api.raml file
5ba12ba
@forevermatt forevermatt marked this pull request as draft September 3, 2024 13:56
@forevermatt forevermatt removed the request for review from a team September 3, 2024 13:57
@forevermatt forevermatt marked this pull request as ready for review September 4, 2024 15:15
@forevermatt forevermatt requested a review from a team September 4, 2024 15:19
briskt
briskt approved these changes Sep 5, 2024
View reviewed changes
Copy link
Contributor

@briskt briskt left a comment

Choose a reason for hiding this comment

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

Looks good. I offer a couple suggestions for documentation and naming. Take 'em or leave 'em.

application/common/models/User.php Show resolved Hide resolved
application/common/models/User.php Show resolved Hide resolved
application/common/models/User.php Outdated Show resolved Hide resolved
application/common/models/User.php Show resolved Hide resolved
@forevermatt
Improve the documentation of User::syncExternalGroups()
364edb4
@forevermatt
Rename syncExternalGroups() to updateUsersExternalGroups()
f179ccd 
It is not doing a sync with any external system. It is merely updating
records based on the given data.
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Sep 5, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@forevermatt forevermatt merged commit b4829f4 into develop Sep 5, 2024
3 checks passed
@forevermatt forevermatt deleted the feature/IDP-1183-sync-google-sheet-to-external-groups branch September 5, 2024 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@briskt briskt briskt approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@forevermatt @briskt