Use PAR#53
Conversation
✅ Deploy Preview for criipto-verify-react-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
75f2ae8 to
4a372c5
Compare
|
Tagging @jlndk for review alongside mhantheman here. Just to easy the review burden on Mick as bit :) |
|
Few commits would benefit from the "why" |
aec1b49 to
33afe99
Compare
|
@mickhansen - rdy for re-review. I reworded some commits, so no fixups unfortunately, |
| width: 24px; | ||
| height: 24px; | ||
| border: 3px solid #fff; | ||
| border-bottom-color: transparent; | ||
| border-radius: 50%; |
There was a problem hiding this comment.
For visual consistency it may be nice to use a real font-awesome icon instead of using css to make a shape (unless there's some icon being added elsewhere which I can't see in this diff).
There was a problem hiding this comment.
I agree, but I don't think we have permission to distribute it :). The spinner we use is part of the pro package
There was a problem hiding this comment.
Ah, right!
Probably slightly out of scope, but it would be great if we could set up package resolution such that we can use the free FA icons by default and then overwrite them with the pro packages at distribution-/deploy- time.
Would also be great for docs 🙏🏻
For now I would almost vote for a free icon being better than none. Opinion @Trinurt?
There was a problem hiding this comment.
@jlndk The problem is that the free Fontawesome icons do not include Sharp and Light. Maybe we need to make a separate PR/discussion for this issue and look in to smart options?
There was a problem hiding this comment.
@jlndk The problem is that the free Fontawesome icons do not include Sharp and Light. Maybe we need to make a separate PR/discussion for this issue and look in to smart options?
Yes that was my entire point, that we could consider developing with free-regular here in the public repos and then it would be swapped out with the real icons at deploy-time.
But yes, could definitely qualify for out-of-scope in terms of this PR :D @Trinurt
There was a problem hiding this comment.
@jlndk How about I add the PRO spinner icon to our custom Fontawesome icon set (takes 10 min) so we can use it for all loading states on all buttons everywhere? (maybe till out of scope for this pr though)
@Trinurt That sounds like it could still violate the terms of redistribution?
Or do you mean you would custom design your own spinner icon - i.e. not extract it from Fontawesome? (What makes something a custom fontawesome icon?)
There was a problem hiding this comment.
@janmeier Aye the free we can, the question was whether we can publish an npm package with the pro icons :) I would think probably not.
Definitely not :)
Creators may not make, share, or publish standalone copies of Pro Icons or Pro Software for non-Creators. For the purpose of clarity (and not for limitation), the use of Pro Icons or Pro Icons Software must constitute a new or separate work. Making, sharing, or publishing copies of Pro Icons or Pro Software in a non-transformative way or in a manner where the Pro Icons or Pro Software are the primary part of what is being made, shared, or published is expressly prohibited.
Creators may not give non-Creators permission to Embed Pro Icons in new Projects of their own, or to Embed them in Projects in new ways. Similarly, Creators may not give non-Creators permission to use Pro Icons or Projects that embed Pro Icons for further use or resale.
There was a problem hiding this comment.
I think finding a way to use the pro spinner in our own products (ie Verify) is out of scope for this PR.
So we have the following options:
- Keep using the existing "moving dots" loader
- Use a free font awesome icon everywhere
- Use another free CSS spinner (which is have I've done here)
There was a problem hiding this comment.
Custom icons are custom icons - I have already made (designed as svg in Figma and uploaded to FA) three custom icons because FA PRO didn't offer the illustration I was looking for.
As for the spinner I can see that I was a little too smart - I can of course not copy the pro and add it to our custom icon kit. But I can design a custom spinner with an IDURA twist and upload it to our custom kit so we can use it on all buttons in all apps and channels. Is that part of this PR though?
There was a problem hiding this comment.
But I can design a custom spinner with an IDURA twist and upload it to our custom kit so we can use it on all buttons in all apps and channels.
Sounds like a good way forward.
But out of scope for PR I would say.
|
@Trinurt Deploy preview updated with HSL, and increasing lightness for FTN and OneID |
c7290b1 to
5dcae51
Compare
|
Request me when ready again :) |
9bd8b22 to
79701c1
Compare
dbc5ca8 to
17fe9fe
Compare
|
@mickhansen Should be ready for another review now. I split the disabled state handling into a separate PR, which is already merged. To recap:
|
c94a34f to
d356fa6
Compare
|
@janmeier conflicts |
This will allow us to use PAR.
Also known as [pushed authorization request](https://datatracker.ietf.org/doc/html/rfc9126). Pushing the authorization request upfront has several benefits: * Prevents an attacker from modifying OIDC parameters * Does not leak any parameters in logs * Prevents OIDC URLs from growing too large
…loading When we show several buttons together in a group, we want to disable _all_ buttons, when the user clicks one of them. Therefore, we add a disabled state on `AuthButtonGroupContextInterface`. `AuthButtonGroupContextInterface` provides a stub default value, so even if a button is _not_ rendered in a group, it is safe to call `setDisabled()`.
This will try to verify that the domain and client ID are correct, and throw if not. This should be considered a breaking change, since just calling the `useCriiptoVerify` hook will now throw an error, if the domain is not valid.
With the introduction of PAR, we will now always do a CORS request (the /par/init request). So we move the CORS notice further up in the README.


This should probably be considered a breaking change, since the PAR request is susceptible to CORS. So if you start on
https://login.example.com, and the redirect URL ishttps://app.example.com, and onlyhttps://app.example.comis allowed as redirect URL - the PAR request will be blocked by CORS.