ory
fix: Update persister_oauth2.go to handle special character | coming in t…
…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
- [x] I have read the contributing guidelines.
- [ ] I have referenced an issue containing the design document if my change introduces a new feature.
- [x] I am following the contributing code guidelines.
- [x] I have read the security policy.
- [x] I confirm that this pull request does not address a security 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.
- [ ] I have added tests that prove my fix is effective or that my feature works.
- [ ] I have added or changed the documentation.
Further Comments
t's probably easier to just adjust the scope charset
@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? Also, dont think the current scopes with "|" will be working anyways, so dont think this should have backward compatibility issues
@Ajayn84 Have you found a workaround with this? We are also facing the same issue for same SMART on FHIR implementation.
@aeneasr - Could you please share the other suggestion that you may have on how to go about fixing this in hydra?
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, | is not a valid url character and would need to be url-en/decoded in URLs, which makes it not that great of an option for use in the scope field, further reducing the likelihood of getting this merged.
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
@Ajayn84 Have you found a workaround with this? We are also facing the same issue for same SMART on FHIR implementation.
@aeneasr - Could you please share the other suggestion that you may have on how to go about fixing this in hydra?
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 :)
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 can you write me an email at [email protected] to discuss the options further please?
Sure @vinckr