fix: Update persister_oauth2.go to handle special character | coming in t… #3849
Conversation
…e scopes as part of consent request Url encoded and decoded while fetching values from the table, as "|" is a seperator used to store scopes
|
|
Ajayn84
changed the title
Ajayn84
changed the title
Ajayn84
changed the title
Ajayn84
changed the title
Ajayn84
changed the title
|
Links to #3829 |
@aeneasr can you please elaborate, i did try to encode , but since this logic is more controlled in the Hydra, it just took that param as "%7C" and responded with scopes having "%7C" instead of "|" We cant replace the | (pipe) char ,as its a requirement from Hl7 Smart App launch https://www.hl7.org/fhir/smart-app-launch/scopes-and-launch-context.html#finer-grained-resource-constraints-using-search-parameters Were you suggesting some other way to fix this issue? |
| @@ -118,8 +118,8 @@ func (p *Persister) sqlSchemaFromRequest(ctx context.Context, signature string, | |||
| RequestedAt: r.GetRequestedAt(), | |||
| InternalExpiresAt: sqlxx.NullTime(expiresAt), | |||
| Client: r.GetClient().GetID(), | |||
| Scopes: strings.Join(r.GetRequestedScopes(), "|"), | |||
| GrantedScope: strings.Join(r.GetGrantedScopes(), "|"), | |||
| Scopes: strings.Join(escapeDelimiter(r.GetRequestedScopes()), "|"), | |||
There was a problem hiding this comment.
we are escaping delimter "|" using queryescape and then joining with "|" delimiter, so that scopes with "|" are replaced with and then joined using "|"
| return &fosite.Request{ | ||
| ID: r.Request, | ||
| RequestedAt: r.RequestedAt, | ||
| // ExpiresAt does not need to be populated as we get the expiry time from the session. | ||
| Client: c, | ||
| RequestedScope: stringsx.Splitx(r.Scopes, "|"), | ||
| GrantedScope: stringsx.Splitx(r.GrantedScope, "|"), | ||
| RequestedScope: scopes, |
There was a problem hiding this comment.
we are splitting the data from db with "|" and then unescaping to get "|" back in the scopes if any
|
Hello, we already have methods to properly delimit string arrays using JSON encoding if I'm not mistaken in our sqlxx package. However, changing the way these things are parsed is a breaking change and requires a careful migration path as it can break all existing records. Given the risk associated, we just ask you to use a different delimiter in scopes. Furthermore, |
|
We cant replace the "|" pipe char with other char, as its a regulatory ask in healthcare. Hl7 Smart App launch link where it indicates the same https://www.hl7.org/fhir/smart-app-launch/scopes-and-launch-context.html#finer-grained-resource-constraints-using-search-parameters |
No @sagarshah1983 , we are also trying to implement Smart on FHIR and specs indicate use of "|" in scopes, without a workaround |
|
Thank you for pointing me to the spec - in that case the use case is legitimate in my view. Still, it's not trivial to implement this and if we fix it it should be a long-term solution (i.e. using the appropriate data types). I will synchronize with @vinckr to figure out what to best do here as I'm actually PTO :) |
Thanks @aeneasr. |
|
@Ajayn84 |
Sure @vinckr |
…he scopes as part of consent request
Url encoded and decoded while fetching values from the table, as "|" is a seperator used to store scopes
Related issue(s)
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
[email protected]) from the maintainers to push
the changes.
works.
Further Comments