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

Add the requested vectors of trust to ServiceProviderRequest #9991

Merged
merged 3 commits into from
Jan 29, 2024

Conversation

jmhooper
Copy link
Member

@jmhooper jmhooper commented Jan 29, 2024

We are working on imlementing a feature for partners to request identity proofing and authentication features using vectors of trust. This will involve sending param describing the vector of trust in the original SAML or OIDC request. Within the context of OIDC this param is named vtr.

This commit adds a vtr property to ServiceProviderRequest. This property is unused and unset in the persisted service provider request. This will allow us to write to it in the future and initialize ServiceProviderRequests with the value without resulting in an ArgumentError (thus avoiding a dreaded 50/50 state bug)

We are working on imlementing a feature for partners to request identity proofing and authentication features using vectors of trust. This will involve sending param describing the vector of trust in the original SAML or OIDC request. Within the context of OIDC this param is named `vtr`.

This commit adds a `vtr` property to `ServiceProviderRequest`. This property is unused and unset in the persisted service provider request. This will allow us to write to it in the future and initialize `ServiceProviderRequest`s with the value without resulting in an `ArgumentError` (thus avoiding a dreaded 50/50 state bug)

[skip changelog]
@jmhooper jmhooper requested review from zachmargolis and a team January 29, 2024 16:39
@@ -12,7 +12,8 @@ def initialize(
ial: nil,
aal: nil,
requested_attributes: [],
biometric_comparison_required: false
biometric_comparison_required: false,
vtr: nil # rubocop:disable Lint/UnusedMethodArgument
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add acr too? so we dont have to persisnt ial//aal separately?

Suggested change
vtr: nil # rubocop:disable Lint/UnusedMethodArgument
vtr: nil, # rubocop:disable Lint/UnusedMethodArgument
acr: nil # rubocop:disable Lint/UnusedMethodArgument

Copy link
Member Author

Choose a reason for hiding this comment

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

I have wavered on whether we should add ACR separately. They get split out and parsed separately by both the OIDC controller and the SAML request.

I was imagining something like this (at least for now):

class ServiceProviderRequest
  def acr_values
    [ial, aal].join(' ')
  end
end

Eventually we may get to a place where we can pass in a raw ACR value. I'm not sure whether that is better to go for now or hold off on. I haven't spent enough time hacking on either the SAML or OIDC portion to know which is better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I did some poking around and decided to go ahead and throw the raw "acr_values" prop on there

jmhooper added a commit that referenced this pull request Jan 29, 2024
In #9991 the `vtr` property is added to the `ServiceProviderRequest`. Since the `vtr` property is introduced there it is unsafe to create a `ServiceProviderRequest` record with `vtr` during a deploy since some instances may have code that is unaware of the `vtr` property and will result in an `ArgumentError` when creating a `ServiceProviderRequest`

Once the changes in #9991 are deployed it should be safe to create records with the `vtr` property. This commit does that in the `ServiceProviderRequestProxy`.

[skip changelog]
jmhooper added a commit that referenced this pull request Jan 29, 2024
In #9991 the `vtr` property is added to the `ServiceProviderRequest`. Since the `vtr` property is introduced there it is unsafe to create a `ServiceProviderRequest` record with `vtr` during a deploy since some instances may have code that is unaware of the `vtr` property and will result in an `ArgumentError` when creating a `ServiceProviderRequest`

Once the changes in #9991 are deployed it should be safe to create records with the `vtr` property. This commit does that in the `ServiceProviderRequestProxy`.

[skip changelog]
@jmhooper jmhooper merged commit 02e474b into main Jan 29, 2024
2 checks passed
@jmhooper jmhooper deleted the jmhooper-add-vtr-to-service-provider-request branch January 29, 2024 20:29
jmhooper added a commit that referenced this pull request Jan 29, 2024
In #9991 the `vtr` property is added to the `ServiceProviderRequest`. Since the `vtr` property is introduced there it is unsafe to create a `ServiceProviderRequest` record with `vtr` during a deploy since some instances may have code that is unaware of the `vtr` property and will result in an `ArgumentError` when creating a `ServiceProviderRequest`

Once the changes in #9991 are deployed it should be safe to create records with the `vtr` property. This commit does that in the `ServiceProviderRequestProxy`.

[skip changelog]
jmhooper added a commit that referenced this pull request Jan 30, 2024
In #9991 the `vtr` property is added to the `ServiceProviderRequest`. Since the `vtr` property is introduced there it is unsafe to create a `ServiceProviderRequest` record with `vtr` during a deploy since some instances may have code that is unaware of the `vtr` property and will result in an `ArgumentError` when creating a `ServiceProviderRequest`

Once the changes in #9991 are deployed it should be safe to create records with the `vtr` property. This commit does that in the `ServiceProviderRequestProxy`.

[skip changelog]
jmhooper added a commit that referenced this pull request Feb 5, 2024
In #9991 the `vtr` property is added to the `ServiceProviderRequest`. Since the `vtr` property is introduced there it is unsafe to create a `ServiceProviderRequest` record with `vtr` during a deploy since some instances may have code that is unaware of the `vtr` property and will result in an `ArgumentError` when creating a `ServiceProviderRequest`

Once the changes in #9991 are deployed it should be safe to create records with the `vtr` property. This commit does that in the `ServiceProviderRequestProxy`.

[skip changelog]
jmhooper added a commit that referenced this pull request Feb 5, 2024
#9993)

In #9991 the `vtr` property is added to the `ServiceProviderRequest`. Since the `vtr` property is introduced there it is unsafe to create a `ServiceProviderRequest` record with `vtr` during a deploy since some instances may have code that is unaware of the `vtr` property and will result in an `ArgumentError` when creating a `ServiceProviderRequest`

Once the changes in #9991 are deployed it should be safe to create records with the `vtr` property. This commit does that in the `ServiceProviderRequestProxy`.

[skip changelog]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants