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

IAM-1047 provider defaults to non existant schema #456

Merged
merged 3 commits into from
Nov 14, 2024
Merged

IAM-1047 provider defaults to non existant schema #456

merged 3 commits into from
Nov 14, 2024

Conversation

BarcoMasile
Copy link
Contributor

@BarcoMasile BarcoMasile commented Nov 12, 2024 •
edited
Loading

Description

The only way to address #405 right now is to remove the default value.

I opened issue #455 to address the lack of a dropdown and more info about how to populate the Mapper field in the Create Provider form.

Changes

  • Since Kratos supports OIDC discovery, for the generic provider type we are allowing AuthURL and TokenURL to be empty in case IssuerURL is non empty
  • Also, now IssuerURL can be empty if AuthURL and TokenURL are both provided
  • Mapper is also allowed to be empty since Kratos will use the default schema in case that field is empty
  • added a custom validation function to validate AuthURL and TokenURL presence in case IssuerURL is not specified
  • added test entry to test the changes

@BarcoMasile BarcoMasile requested a review from a team as a code owner November 12, 2024 16:28
nsklikas
nsklikas reviewed Nov 12, 2024
View reviewed changes
Copy link
Contributor

@nsklikas nsklikas left a comment

Choose a reason for hiding this comment

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

Haven't tried it, so I am probably wrong on some things

pkg/idp/third_party.go Outdated Show resolved Hide resolved
pkg/idp/third_party.go Outdated Show resolved Hide resolved
pkg/idp/validation_test.go Show resolved Hide resolved
@BarcoMasile BarcoMasile force-pushed the IAM-1047-provider-defaults-to-non-existant-schema branch from 104b360 to f3d1bac Compare November 12, 2024 16:52
pik4ez-canonical
pik4ez-canonical requested changes Nov 13, 2024
View reviewed changes
pkg/idp/third_party.go Outdated Show resolved Hide resolved
@BarcoMasile BarcoMasile force-pushed the IAM-1047-provider-defaults-to-non-existant-schema branch 2 times, most recently from 01fa6d9 to 098280d Compare November 13, 2024 16:40
@BarcoMasile
Copy link
Contributor Author

BarcoMasile commented Nov 13, 2024 •
edited
Loading

@nsklikas @pik4ez-canonical
I went the custom validation func way. Basically, the new function gets added to the other validation so "regular" tags still work as expected.

The validation is for the generic provider only.

I'll update the description of this PR and request reviews again. Also, I added the tests suggested by Nikos (thanks for that).

@BarcoMasile BarcoMasile force-pushed the IAM-1047-provider-defaults-to-non-existant-schema branch from 098280d to a28056a Compare November 14, 2024 08:44
pik4ez-canonical
pik4ez-canonical previously approved these changes Nov 14, 2024
View reviewed changes
Copy link
Contributor

@pik4ez-canonical pik4ez-canonical left a comment

Choose a reason for hiding this comment

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

Looks clean and readable thanks to the comments.

Thank you Marco, good job!

@BarcoMasile BarcoMasile force-pushed the IAM-1047-provider-defaults-to-non-existant-schema branch from a28056a to 6452cd2 Compare November 14, 2024 14:10
@BarcoMasile
refactor: fix default value for mapper_url in ProviderCreate.tsx
5826d76
@BarcoMasile
refactor: allow for empty AuthURL and TokenURL if IssueURL is provide…
f42102a 
…d, allow Mapper to be empty

since Kratos will use default Mapper if its value is empty
and will try to use OIDC discovery when IssuerURL is provided
@BarcoMasile
test: add tests for changed spec
cde3d09
@BarcoMasile BarcoMasile force-pushed the IAM-1047-provider-defaults-to-non-existant-schema branch from 6452cd2 to cde3d09 Compare November 14, 2024 14:11
@BarcoMasile
Copy link
Contributor Author

I consider this PR final, all comments are addressed and already rebased on main, please review again @nsklikas @pik4ez-canonical

@BarcoMasile BarcoMasile merged commit 8d43266 into main Nov 14, 2024
10 checks passed
@BarcoMasile BarcoMasile deleted the IAM-1047-provider-defaults-to-non-existant-schema branch November 14, 2024 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pik4ez-canonical pik4ez-canonical pik4ez-canonical approved these changes

@nsklikas nsklikas nsklikas approved these changes

@wood-push-melon wood-push-melon Awaiting requested review from wood-push-melon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@BarcoMasile @nsklikas @wood-push-melon @pik4ez-canonical