LG-11537 Access the active profile in the userinfo endpoint#9596
LG-11537 Access the active profile in the userinfo endpoint#9596
Conversation
In #9509 we added the ability to specify which profile to fetch PII from when reading PII from the session. This commit applies that capability to the OpenID Connect UserInfo endpoint to ensure that only the active profile's PII is used there. The user info endpoint is used by service providers to access PII after auth. As a result it is not a request made by users and does not have a user session. To read PII we use the out-of-band session accessor's `#load_pii` method. This commit adds a `profile_id` arg to that method and passes in the active profile's ID when reading PII. changelog: Internal, Active-pending profile, The active profile is now read from the session on the userinfo endpoint.
| # Only used for convenience in tests | ||
| # @param [Pii::Attributes] pii | ||
| def put_pii(pii, expiration = 5.minutes) | ||
| def put_pii(profile_id, pii, expiration = 5.minutes) |
There was a problem hiding this comment.
This method creates a user session and adds PII to it. I found that throughout the code it was used as a convenience to create a user session. Above I added the put_empty_user_session to perform that action without adding PII to help make things a little more clear.
soniaconnolly
left a comment
soniaconnolly
left a comment
There was a problem hiding this comment.
LGTM, a few comments.
| @ial2_data ||= begin | ||
| if ial2_session? || ialmax_session? | ||
| out_of_band_session_accessor.load_pii | ||
| if (ial2_session? || ialmax_session?) && active_profile.present? |
There was a problem hiding this comment.
Do we want a method that expresses ial2_session? || ialmax_session?, maybe profile_requested? or something like that?
There was a problem hiding this comment.
I think within the context of the user info controller these make sense as different request contexts where PII may be read. I think that reducing them to a single method may add some indirection.
| OutOfBandSessionAccessor.new( | ||
| identity.rails_session_id, | ||
| ).put_empty_user_session( | ||
| 5.minutes.to_i, |
There was a problem hiding this comment.
Noticed that this argument has to_i, but the default argument above is just 5.minutes, not sure if that's an issue.
There was a problem hiding this comment.
I don't think it is. I'm just trying to keep it consistent with what was there before.
| describe '#ttl' do | ||
| it 'returns the remaining time-to-live of the session data in redis' do | ||
| store.put_pii({ first_name: 'Fakey' }, 5.minutes.to_i) | ||
| store.put_pii(123, { first_name: 'Fakey' }, 5.minutes.to_i) |
There was a problem hiding this comment.
Would it make sense to let(profile_id) { 123 } for clarity?
There was a problem hiding this comment.
Hmm, I can do something like that.
| # Only used for convenience in tests | ||
| # @param [Pii::Attributes] pii | ||
| def put_pii(pii, expiration = 5.minutes) | ||
| def put_pii(profile_id, pii, expiration = 5.minutes) |
There was a problem hiding this comment.
Next time I would convert these to keyword arugments, since pii used to be first and now it's not
3 participants
In #9509 we added the ability to specify which profile to fetch PII from when reading PII from the session.
This commit applies that capability to the OpenID Connect UserInfo endpoint to ensure that only the active profile's PII is used there.
The user info endpoint is used by service providers to access PII after auth. As a result it is not a request made by users and does not have a user session. To read PII we use the out-of-band session accessor's
#load_piimethod. This commit adds aprofile_idarg to that method and passes in the active profile's ID when reading PII.