-
Notifications
You must be signed in to change notification settings - Fork 784
Link unfurling without SignalID #1196
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
base: main
Are you sure you want to change the base?
Conversation
e667f4e to
d2b336a
Compare
d2b336a to
492e8a4
Compare
|
@monorkin this is good to go from my end. Here's the flow:
|
57856b9 to
27f42ff
Compare
kevinmcconnell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good 👏
I imagine we have to update a few bits with regards to the removal of tenanting. And also the Basecamp portions should probably move into the SaaS gem?
But, the big picture of how this works looks great!
| } | ||
|
|
||
| get #cookie() { | ||
| const name = `link-accounts-prompt-${Current.user.id}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the user ID part of the cookie name? Is it to handle switching between multiple accounts? (And if it is, should we scope it to the account slug instead?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. That was to scope it to account. I'll update it to scope it via path segments
| document.at_css('meta[property="og:description"]')&.get_attribute("content") || | ||
| document.at_css('meta[name="twitter:description"]')&.get_attribute("content") || | ||
| document.at_css('meta[name="description"]')&.get_attribute("content") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like there's a recurring pattern to grab the content attribute from the element matched by a CSS selector.
Maybe this is scope to extract some of this behaviour to DRY it up?
| end | ||
|
|
||
| test "create" do | ||
| sign_in_as :kevin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the setup already did this?
114c7e8 to
c4e579b
Compare
Implement content fetching Originally implemented a SAX parser to avoid the body size limit, but it turns out that Nokogiri doesn't support SAX parsers for HTML5. This was quite confusing because the parser is called HTML::SAX::Parser and everywhere else HTML is aliased to HTML5. I had to abandon this approach after spending a few hours on it, and then opted for the same approach that Campfire uses - fetch the whole body and parse it. But I lowered the body size to 2MB after looking up what the median web page body size is - 160KB 50%%, 285KB 90%%.
c4e579b to
3c171c9
Compare
The Basecamp unfurler depends on a trussted OAuth app so it isn't usable by people that don't have access to our OAuth server. Therefore I'm moving it to the SAAS part of the app. I also took the opportunity to remove any secrets that caused gitleaks to trigger. These aren't real production secrets but secrets I used locally during development. To avoid HackerOne and other kinds of reports I'm removing them and marking them as clearly being just for development.
3c171c9 to
3122011
Compare




It's easiest to think of this PR as having two parts:
Generic Link Unfurling
The link unfurling logic takes inspiration from @flavorjones prior attempt in #743 and a bit from Campfire, bit it also extends the logic a bit to support different kinds of unfurling, which I'll get to later.
This is how the unfurling process goes:
For this to work I had to extend Lexxy to emit an event when someone pastes a link - basecamp/lexxy#281 (comment)
After that, the application logic was fairly straight forward.
One downside of this unfurling method is that it's synchronous: A malicious person could create multiple accounts and intentionally paste links to a URL that is very slow to resolve. This would exhaust our thread pool and result in denial of service.
A better way to do this would be to unfurl the link in a job and relay the result via ActionCable to the frontend.
I decided not to go down this route, for now, because all our apps do synchronous unfurling and so far it didn't seem to be a problem. As a, albeit weak, protection measure I adopted the rate limit from Basecamp and RichLink.
The unfurler objects allow us to plug in unfurling for different 3rd party apps - like Sentry & HackerOne - but for now they are only used to unfurl links from Basecam and Fizzy itself.
Basecamp integration via OAuth
Since the goal of this PR is to avoid using Signal ID I had to use regular OAuth to access Basecamp.
For Fizzy to be able to read someone's Basecamp data the person has to to link their Fizzy and Basecamp accounts first.
The linking process is a regular OAuth2 token exchange.
To make the exchange I needed an untennated route - the callback. It can't be tenanted because OAuth return URLs can't have wildcards in them. Another twist here is that I couldn't pass the tenant ID as a param to the URL because Signal ID / Launchpad does strict comparison of the return URL (think
==instead ofuri.host == redirect.host && uri.path == redirect.path) which isn't uncommon in OAuth. To work around that I used the OAuth state, a mechanism for preventing token hijacking, to pass the exact integration I'm setting up and the tenant around. The state gets echoed back from the OAuth server with the callback. I used a signed global ID of the integration as the state. Thanks to AR::Tenanted the SGID contains the tenant ID in it and I can just ask for it, in addition ot that I can set a short expiry to prevent hijacking, and I can ignore tokens for integrations that have already been setup before.Once the integration is setup and Fizzy has a Basecamp access token to use it can just make a regular HTTP request to the Basecamp URL with that access token and Basecamp will respond with HTML. This works because Basecamp has a special case in its auth logic which treats OAuth access tokens of trusted OAuth apps (which Fizzy is) as if they were cookies.
To make this request, Basecamp has its own link unfurler object that adds the user's access token to the request. It also handles the failure case if the user doesn't have a Basecamp integration setup but wants to unfurl a Basecamp link. And it handles token refreshes in a lazy manner.
If everything is setup, the unfurler returns a Metadata object. If the integration isn't setup it raises an error that the controller catches and turns into a JSON response to the Stimulus controller. The Stimulus controller, when it encounters the error, renders a prompt for the user to link their account.
The linking is done in a popup so that we don't interrupt whatever the user was doing if they decide to link their accounts.
I took care that the popup is triggered by a user interaction so that it gets shown even on browsers with aggressive popup protections, and I styled it as much as I could such that it looks like a "native" element instead of a browser window.
I also extended the rich textarea tag to always include the link unfurling controller and elements, unless specifically instructed not to do that. I feel like that's something we'd want in all rich text areas, and didn't want to copy paste the same incantation code and HTML elements around. That said, the override feels like a heavy handed solution but I couldn't think of a better one. After looking around I found a very similar solution in Basecamp.
How to style this
The prompt is rendered using a helper in rich_text_helper.rb - if it gets more involved I can move it to a partial, just let me know.
The positioning of the promt is controlled via CSS in app/assets/stylesheets/lexxy.css:43
The first screen that is rendered in the popup is in app/views/integrations/basecamps/new.html.erb
The "done" screen is in app/views/integrations/basecamps/callbacks/show.html.erb
To get the integration to work locally
Run the following in launchpad:
Reviewer's note
I'm opening this as a drive sinc it's gotten quite big. But there are a few things I still want to adress: