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

Limit tech admin contact operations via IS config #6378

Merged
merged 19 commits into from
Jul 7, 2023

Conversation

nicholaspcr
Copy link
Contributor

Summary

Closes #6323

Changes

  • Add an contact info restriction error
  • Add restrictions field to IS config, alongside the contact info restriction
  • Update entities registries to validate contact if config is enabled

Testing

Unit tests and manual testing

Regressions

This is a new restriction on any flow that sets the admin/tech contacts which aren't done through the console. So I can't imagine a regression and most likely CLI users aren't setting those contacts since the old way still works (even with the deprecated warning).

Notes for Reviewers

Was going to do a testcase just for restrictions for each registry test file but it felt like overkill, let me know if the smaller tests found currently on the PR are appropriate enough.

If there are any lint errors on CI, I'll run the tests again after the format merge of the separate repository. Made the lint changes into a different PR just to remove it from this PR.

Checklist

  • Scope: The referenced issue is addressed, there are no unrelated changes.
  • Compatibility: The changes are backwards compatible with existing API, storage, configuration and CLI, according to the compatibility commitments in README.md for the chosen target branch.
  • Documentation: Relevant documentation is added or updated.
  • Changelog: Significant features, behavior changes, deprecations and fixes are added to CHANGELOG.md.
  • Commits: Commit messages follow guidelines in CONTRIBUTING.md, there are no fixup commits left.

@nicholaspcr nicholaspcr added this to the v3.27.0 milestone Jul 5, 2023
@nicholaspcr nicholaspcr self-assigned this Jul 5, 2023
@github-actions github-actions bot added c/identity server This is related to the Identity Server compat/api This could affect API compatibility compat/db This could affect Database compatibility compat/config This could affect Configuration compatibility labels Jul 5, 2023
@nicholaspcr nicholaspcr force-pushed the feature/6323-limit-tech-admin-contact-ops branch from 5758e84 to 235a825 Compare July 5, 2023 13:57
Copy link
Contributor

@adriansmares adriansmares left a comment

Choose a reason for hiding this comment

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

Implementation LGTM in general. I have some naming concerns only.

pkg/identityserver/config.go Outdated Show resolved Hide resolved
pkg/identityserver/application_registry_test.go Outdated Show resolved Hide resolved
pkg/identityserver/store/errors.go Show resolved Hide resolved
@nicholaspcr nicholaspcr force-pushed the feature/6323-limit-tech-admin-contact-ops branch from 235a825 to f65c670 Compare July 5, 2023 20:38
api/identityserver.proto Outdated Show resolved Hide resolved
pkg/identityserver/config.go Outdated Show resolved Hide resolved
pkg/identityserver/gateway_registry_test.go Outdated Show resolved Hide resolved
pkg/identityserver/store/errors.go Show resolved Hide resolved
@nicholaspcr nicholaspcr force-pushed the feature/6323-limit-tech-admin-contact-ops branch 2 times, most recently from aef2064 to eb316df Compare July 7, 2023 13:48
@nicholaspcr nicholaspcr force-pushed the feature/6323-limit-tech-admin-contact-ops branch from eb316df to 2ad5c6e Compare July 7, 2023 13:53
CHANGELOG.md Outdated Show resolved Hide resolved
@nicholaspcr nicholaspcr force-pushed the feature/6323-limit-tech-admin-contact-ops branch 3 times, most recently from 61b9ca3 to 2635b5b Compare July 7, 2023 17:57
nicholaspcr added a commit that referenced this pull request Jul 7, 2023
@nicholaspcr nicholaspcr force-pushed the feature/6323-limit-tech-admin-contact-ops branch from cb5670f to 5245e76 Compare July 7, 2023 18:17
@nicholaspcr nicholaspcr merged commit 644376a into v3.27 Jul 7, 2023
13 of 16 checks passed
@nicholaspcr nicholaspcr deleted the feature/6323-limit-tech-admin-contact-ops branch July 7, 2023 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/identity server This is related to the Identity Server compat/api This could affect API compatibility compat/config This could affect Configuration compatibility compat/db This could affect Database compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IS config for limiting who can be a admin/tech contact for entities
3 participants