-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: support oidc webauthn sequencing mode #318
base: main
Are you sure you want to change the base?
Conversation
@@ -926,6 +927,7 @@ def _update_kratos_info_relation_data(self, event: RelationEvent) -> None: | |||
schemas_configmap_name = self.schemas_configmap.name | |||
configmaps_namespace = self.model.name | |||
mfa_enabled = self.config.get("enforce_mfa") | |||
oidc_webauthn_sequencing_enabled = self.config.get("enable_oidc_webauthn_sequencing") |
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.
would it be ok to provide a default value here?
oidc_webauthn_sequencing_enabled = self.config.get("enable_oidc_webauthn_sequencing") | |
oidc_webauthn_sequencing_enabled = self.config.get("enable_oidc_webauthn_sequencing", False) |
@@ -529,6 +529,7 @@ def _render_conf_file(self) -> str: | |||
enable_local_idp=self.config.get("enable_local_idp"), | |||
enforce_mfa=self.config.get("enforce_mfa"), | |||
enable_passwordless_login_method=self.config.get("enable_passwordless_login_method"), | |||
enable_oidc_webauthn_sequencing=self.config.get("enable_oidc_webauthn_sequencing"), |
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.
and here also
@@ -8,7 +8,7 @@ identity: | |||
- id: {{ schema_id }} | |||
url: '{{ identity_schema }}' | |||
{%- endfor %} | |||
{%- if enable_local_idp and enforce_mfa %} | |||
{%- if enable_oidc_webauthn_sequencing or enable_local_idp and enforce_mfa %} |
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.
suggestion: can you group the conditions here so they're easier to read and, more importantly, we make sure we don't make mistake
{%- if enable_oidc_webauthn_sequencing or enforce_mfa and enable_local_idp %} | ||
lookup_secret: | ||
enabled: True | ||
{%- endif %} | ||
{%- if enable_passwordless_login_method %} | ||
{%- if enable_passwordless_login_method or enable_oidc_webauthn_sequencing %} |
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.
same suggestion here
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.
Agree with Marco's comments. Other parts LGTM.
Just one dummy question:
do we need to increase the lib's major version since we changed the public API of the provider?
This PR adds support for authentication factors sequencing:
reset-identity-mfa
action to includewebauthn
enable_oidc_webauthn_sequencing
kratos-info
lib to include the mode - addsoidc_webauthn_sequencing_enabled
to relation data.