-
Notifications
You must be signed in to change notification settings - Fork 9
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
Refactor: AuthProvider
to ClaimsProvider
#2256
Conversation
Preview url: https://benefits-2256--cal-itp-previews.netlify.app |
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
76322fe
to
12b6943
Compare
6508029
to
fa01aa9
Compare
Ready for review |
Not sure if this one ( Line 41 in 58abd80
|
I think |
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.
The code changes look good! These were verified:
- For all
EligibilityVerifier
s that with a reference to anAuthProvider
, confirm those references were maintained, only now the field is called "Claims provider" in the admin interface,claims_provider
in the database - Confirm the
Can X auth provider
permissions are removed from the database - Confirm the
Can X claims provider
permissions are added to the database - Confirm the Cal-ITP group has
Can [view|change] claims provider
permissions - Flows using claims from IdG should still work
Just saw a minor typo in oauth.md
.
docs/configuration/oauth.md
Outdated
|
||
The [data migration file](./data.md) contains sample values for an `AuthProvider` configuration. You can set values for a real Open ID Connect provider in environment variables so that they are used instead of the sample values. | ||
The [data migration file](./data.md) contains sample values for an `ClaimsProvider` configuration. You can set values for a real Open ID Connect provider in environment variables so that they are used instead of the sample values. |
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.
Sorry, tiny typo, maybe change:
an
ClaimsProvider
to
a
ClaimsProvider
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.
Thanks! Made that change in ee3df8ec
the distinction between "requiring login" and "using claims to verify" is unnecessary. this makes for one less method to rename.
to say "claims verification" instead. also rename test fixtures related to scope and claim to be clearer.
c0a4a90
to
7c042a5
Compare
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 looks great 👍 !!!
Thanks for the reviews! |
Closes #2233 - see Acceptance Criteria on that ticket.
Testing locally
main
branchlocal_fixtures.json
(or whatever fixture you are using) has anAuthProvider
and anEligiblityVerifier
that has a foreign key reference to that auth providerbin/reset_db.sh
(now your local DB mirrors the structure of thedev
environment)bin/init.sh
(running this PR's migration on top of existing DB structure)EligibilityVerifier
s that with a reference to anAuthProvider
, confirm those references were maintained, only now the field is called "Claims provider" in the admin interface,claims_provider
in the databaseCan X auth provider
permissions are removed from the databaseCan X claims provider
permissions are added to the databaseCan [view|change] claims provider
permissions