Echo: add session controllers, routes and socket-subscribe authorization (Slice 4) - #1651
Conversation
7b1e2ad to
2be6905
Compare
094abe5 to
4cecf56
Compare
2be6905 to
8467bbb
Compare
4cecf56 to
1e7c6bf
Compare
SEC-514 — PR #1655 code-review fixes (Slice 4:
|
|
@MattBudz — this PR (Echo Slice 4: add session controllers) has been open for >5 days without activity. Any blockers? Can we get this merged? |
9427a44 to
5996d2f
Compare
efddc56 to
de6bb67
Compare
de6bb67 to
47a483e
Compare
| private | ||
|
|
||
| def scoped_record(session) | ||
| collection = session.record_type == 'Issue' ? current_project.issues : current_project.notes |
There was a problem hiding this comment.
This isn't cleanly extensible if we add other records in the future. What about
| collection = session.record_type == 'Issue' ? current_project.issues : current_project.notes | |
| record_type = session.record_type.underscore.pluralize | |
| collection = current_project.public_send(record_type).find(session.record_id) |
| record = GlobalID::Locator.locate(name.split(':').first) | ||
| record if record.is_a?(Session) |
There was a problem hiding this comment.
| record = GlobalID::Locator.locate(name.split(':').first) | |
| record if record.is_a?(Session) | |
| GlobalID::Locator.locate(name.split(':').first, only: Session) |
https://www.rubydoc.info/gems/globalid/GlobalID/Locator#locate-class_method
|
|
||
| def turbo_backend_reachable? | ||
| adapter = ActionCable.server.pubsub | ||
| return true unless adapter.is_a?(ActionCable::SubscriptionAdapter::Redis) |
There was a problem hiding this comment.
| return true unless adapter.is_a?(ActionCable::SubscriptionAdapter::Redis) | |
| return true unless adapter.respond_to?(:redis_connection_for_subscriptions) |
What about checking that it responds rather than matching against the specific class name? More future-proof
| # Appends a user turn and re-opens the reply gate. The new message | ||
| # broadcasts itself into the transcript, so there's nothing to render back. | ||
| def create | ||
| message = @session.messages.build(content: params[:content], role: :user, user: current_user) |
There was a problem hiding this comment.
Can we add a callback in the Message model that sets role to :user if a user is passed? That way we don't have to remember to pass both every time
| # Appends a user turn and re-opens the reply gate. The new message | ||
| # broadcasts itself into the transcript, so there's nothing to render back. | ||
| def create | ||
| message = @session.messages.build(content: params[:content], role: :user, user: current_user) |
There was a problem hiding this comment.
Can we validate the content with something like params.expect?
params.expect(:content)https://api.rubyonrails.org/classes/ActionController/Parameters.html#method-i-expect
| private | ||
|
|
||
| def set_session | ||
| @session = Session.find(params[:session_id]) |
There was a problem hiding this comment.
Two things are odd here:
scoped_record(@session)isn't being assigned to anything, so I assume this is meant to throw an exception if it's not found? If that's the case, it's not super clear and maybe a comment or a more explicit way of handling this would be better?- This method is called
set_sessionbut it's doing more than that, it's also validating the record. Can we split it or make it clearer what this does?
There was a problem hiding this comment.
Isn't set_ a common idiom in our controllers that set and validate form user params?
|
|
||
| def set_session | ||
| @session = Session.find(params[:session_id]) | ||
| scoped_record(@session) |
There was a problem hiding this comment.
Since everywhere set_session is defined we're also validating the record via scoped_record why not include this in the concern and make it more general? Like HasSession or something like that
| end | ||
|
|
||
| def set_record | ||
| @record = current_project.send(@type.to_s.pluralize).find(record_params[:record]) |
There was a problem hiding this comment.
This is overwritten on L67
| # one. Roslin-disabled and prompts empty-state warnings are handled by the | ||
| # view. | ||
| def index | ||
| @sessions = Session.for_record(@record) |
There was a problem hiding this comment.
What about renaming this scope to something like record_scoped? Session.for_record sounds like we're creating a session based on other naming conventions we have in the app like from_rtp etc
There was a problem hiding this comment.
I'm not certain a different name is better. "record scoped" signifies it's a scope for this record, which I think "for_record" kind of already does...
The minions also noticed:
same pattern as Notification.for_user
| title: prompt.title, | ||
| user: current_user | ||
| ) | ||
| @session.messages.build(content: params[:prompt], role: :user, user: current_user) |
| <div class="list-group mb-3" id="echo-sessions" data-behavior="echo-sessions"> | ||
| <% @sessions.each do |session| %> | ||
| <%= link_to session.title.presence || "Session ##{session.id}", | ||
| echo.project_session_path(current_project, session, type: @type, record: @record.id), |
There was a problem hiding this comment.
Do we need both record id and type? Since we're validating type in the controller can we get one from the other?
| private | ||
|
|
||
| def record_params | ||
| params.permit(:id, :prompt, :prompt_id, :project_id, :record, :type) |
There was a problem hiding this comment.
record_id would be better since we're not passing the whole record
| @@ -0,0 +1,34 @@ | |||
| <turbo-frame id="<%= dom_id(@record, :echo) %>"> | |||
| <% unless @turbo_status %> | |||
There was a problem hiding this comment.
What about extracting this entire block to a shared partial and we can include it wherever we need in future with a single render partial call? I can foresee us needing this in more places down the line
|
|
||
| <div id="echo-messages" data-behavior="echo-messages"> | ||
| <%= render partial: 'dradis/plugins/echo/projects/sessions/messages/message', | ||
| collection: @session.messages.order(:created_at, :id), as: :message %> |
There was a problem hiding this comment.
why not set @collection in the controller?
|
|
||
| resources :grammar_corrections, only: [:create], controller: 'projects/grammar_corrections' | ||
| resources :grammar_suggestions, only: [:create], controller: 'projects/grammar_suggestions' | ||
|
|
There was a problem hiding this comment.
Now that we have so many controllers under projects/ it looks like it's time for something like
scope module: 'projects' do| <%# Live transcript + composer state. Slice 5 adds `channel: SessionsChannel` %> | ||
| <%# so the socket re-authorizes on subscribe. %> |
There was a problem hiding this comment.
| <%# Live transcript + composer state. Slice 5 adds `channel: SessionsChannel` %> | |
| <%# so the socket re-authorizes on subscribe. %> | |
| <%# Live transcript + composer state. %> |
66910f0 to
19b458f
Compare
c34ab03 to
7ba7a71
Compare
A private serialize(session, cutoff_id) overrode ActiveJob::Core#serialize, which every queue adapter calls when enqueuing. perform_later — and so Session#request_reply! — raised NoMethodError at runtime. Rename the private method to finalize and cover the enqueue path.
Purely additive Slice 4. Adds project-scoped Sessions/Messages controllers (Prompt read only at the controller boundary, no FK; Prompt::SCOPES whitelist honoured via set_type), their routes and index/show views, and SessionsChannel — a custom Turbo Streams channel that re-checks :use on the session's project at subscribe time and rejects revoked users, closing the non-expiring signed-stream-name hole. InteractionsController is untouched.
On initial session creation SessionsController#create called request_reply! before render :show, so ReplyJob's streaming-container broadcast raced ahead of the browser's SessionsChannel subscription; the container append was missed and the later chunk/replace broadcasts hit a non-existent node, so the reply only appeared on reload (SEC-506 Bug 4). Restore the invariant "the streaming container is broadcast only to a listening socket": create no longer starts generation. It renders show in the new Session#reply_pending? state and a dedicated RepliesController#create (POST sessions/:id/reply) starts generation, to be driven from the session Stimulus controller once it has connected. reply_pending? (idle + newest message is a user turn) guards the trigger so reconnects and extra viewers can't spawn an unsolicited reply. Follow-up messages are unchanged — the client is already subscribed there. Refs SEC-506.
Applies PR #1655 review findings on the session controllers slice: - Blank-prompt 500 (#7): sessions#create now saves without a bang and returns 422 on an invalid (blank) prompt, mirroring the messages endpoint, instead of raising RecordInvalid. - Dead index (#9): drop the unused sessions#index action and route; nothing links to it (the back-link and lazy frame use the interactions path). The dead index view is removed in the views slice. - check_turbo_config (#10): extract a shared TurboConfigCheck concern that pings only the Redis adapter and memoizes, so async/test adapters no-op silently — no spurious 'can't contact Redis' alert, no per-request round-trip. - SessionsChannel (#11): tighten the class comment to say re-auth guards new subscriptions only (comment-only).
Ask the session for its project instead of reaching through session.record.project. record is polymorphic, so the delegation is the right seam for the authorization check and keeps callers decoupled from where a session's project currently comes from.
e43a0b9 to
7bc671b
Compare
…ed scoping, duck-typed cable check
… expect content param
Summary
Factory Slice 4 of 5 — Echo Sessions ( SEC-475, milestone SEC-469). Stacked on Slice 3 (#1650, base branch
echo/add-reply-job).Purely additive — no changes to
InteractionsController, its views, or its routes (that teardown is owned by a later ticket). This slice adds the session-based interaction surface and the socket re-authorization security fix.What's added:
resources :sessions, only: [:index, :show, :create]with nestedresources :messages, only: [:create]underprojects.interactionsroutes left exactly as they were.Projects::SessionsController(AuthenticatedController+ProjectScoped,layout false):create— builds aSessionwithAgents::Roslin.instance,current_user, and atitlecopied from the selected prompt. The Prompt is read only at the controller boundary — no FK is stored — and the lookup is scoped throughfor(@type)so thePrompt::SCOPESwhitelist (enforced byset_type) is honoured. The first userMessagecarries the (Liquid-rendered, possibly edited) prompt text, thenrequest_reply!fires. Responds with the session turbo frame.show— transcript + composer; runs thecheck_turbo_configRedis ping and renders the "Redis is down" warning.index— record-scoped session list; keeps the Roslin-disabled and prompts empty-state warnings.Projects::Sessions::MessagesController#create— appends a user message and re-opens the reply gate viarequest_reply!.Dradis::Plugins::Echo::SessionsChannel— a custom Turbo Streams channel (documented turbo-rails pattern). Turbo's default channel trusts any validly-signed stream name for its (non-expiring) lifetime, so a user who loses project access keeps receiving the transcript. This channel resolves the session behind the signed name and re-checksAbility.new(current_user).can?(:use, session.record.project)at subscribe time, rejecting revoked users. The view'sturbo_stream_fromgainschannel:in Slice 5.RecordScopingconcern — resolves records through the current project's collections so cross-project/record access raisesRecordNotFound(mirrorsProjects::GrammarController).Bug fixed (Slice 3 carryover):
ReplyJobdefined a privateserialize(session, cutoff_id)that shadowedActiveJob::Core#serialize— the method every queue adapter calls when enqueuing.perform_later(and thereforeSession#request_reply!) raisedNoMethodErrorat runtime; Slice 3 never caught it because its specs stubbedperform_later. This slice is the first to enqueue for real. Renamed the private method tofinalizeand added an enqueue-path guard. Committed separately for review.Testing steps
POSTtoproject_sessions_pathwithtype=issue,record=<issue id>,prompt_id=<saved prompt id>, andprompt=<edited text>. A new session + first user message are created, a reply is enqueued, and the response renders the session frame.POSTa message to the nestedmessagesroute — a user message is appended and a reply is enqueued.POSTwith an out-of-project record id — the request is denied (404).SessionsChannelfor a session you may:use— accepted and streaming. Revoke:useon the project and re-subscribe — rejected.Automated coverage: request specs (sessions + messages, record scoping + cross-record denial), a channel spec that flips authorization between subscribe attempts, and the interactions/grammar suites (unchanged, still green). Full engine suite: 169 examples, 0 failures.
Other Information
Local CI mirror all green: bundler-audit, ruby-audit, brakeman
-w2(0 warnings), rubocop (changed files, 0 offenses), rspec (engine suite).Check List