Fix: use LoggedInInfo session validation in Scratch2Action#2175
Draft
LiamStanziani wants to merge 2 commits intomaintenancefrom
Draft
Fix: use LoggedInInfo session validation in Scratch2Action#2175LiamStanziani wants to merge 2 commits intomaintenancefrom
LiamStanziani wants to merge 2 commits intomaintenancefrom
Conversation
Replace raw session attribute access with LoggedInInfo pattern for proper session management and validation. Add authorization checks to ensure users can only access their own scratch pad data. Changes: - Add LoggedInInfo import - Update execute() to use LoggedInInfo.getLoggedInInfoFromSession() - Update delete() with session validation and ownership verification - Update showVersion() with session validation and ownership verification - Return appropriate HTTP status codes (401/403) for auth failures - Add PHI-safe logging with OWASP encoding Fixes #1889 Co-authored-by: Liam Stanziani <LiamStanziani@users.noreply.github.com>
Reviewer's GuideRefactors Scratch2Action to use the centralized LoggedInInfo session helper, adds authentication and authorization checks for scratch pad access/modification, returns appropriate HTTP status codes on failure, and hardens logging with OWASP encoding and safer error handling. Sequence diagram for Scratch2Action showVersion with session validation and ownership checksequenceDiagram
actor User
participant Scratch2Action
participant LoggedInInfo
participant ScratchPadDao
participant HttpServletResponse
participant Logger
User->>Scratch2Action: showVersion(request)
Scratch2Action->>LoggedInInfo: getLoggedInInfoFromSession(request)
alt loggedInInfo is null
Scratch2Action->>Logger: error(Invalid or expired session)
Scratch2Action->>HttpServletResponse: setStatus(401)
Scratch2Action-->>User: null
else loggedInInfo not null
Scratch2Action->>LoggedInInfo: getLoggedInProviderNo()
alt providerNo is null
Scratch2Action->>Logger: error(Provider number not found in session)
Scratch2Action->>HttpServletResponse: setStatus(401)
Scratch2Action-->>User: null
else providerNo not null
Scratch2Action->>Scratch2Action: getParameter(id)
alt id is null or empty
Scratch2Action->>Logger: error(Invalid scratch pad id)
Scratch2Action-->>User: error view
else id valid
Scratch2Action->>ScratchPadDao: find(id)
alt scratchPad is null
ScratchPadDao-->>Scratch2Action: null
Scratch2Action->>Scratch2Action: throw IllegalArgumentException
else scratchPad found
ScratchPadDao-->>Scratch2Action: ScratchPad
alt providerNo != scratchPad.providerNo
Scratch2Action->>Logger: error(User attempted to view scratch pad owned by another)
Scratch2Action->>HttpServletResponse: setStatus(403)
Scratch2Action-->>User: null
else providerNo == scratchPad.providerNo
Scratch2Action->>Scratch2Action: setAttribute(ScratchPad)
Scratch2Action-->>User: scratchPadVersion view
end
end
end
end
end
Sequence diagram for Scratch2Action delete with session validation and ownership checksequenceDiagram
actor User
participant Scratch2Action
participant LoggedInInfo
participant ScratchPadDao
participant JSONObject
participant HttpServletResponse
participant Logger
User->>Scratch2Action: delete(request)
Scratch2Action->>JSONObject: new JSONObject()
Scratch2Action->>LoggedInInfo: getLoggedInInfoFromSession(request)
alt loggedInInfo is null
Scratch2Action->>Logger: error(Invalid or expired session)
Scratch2Action->>HttpServletResponse: setStatus(401)
Scratch2Action->>JSONObject: put(success, false)
Scratch2Action-->>User: jsonResponse(JSONObject)
else loggedInInfo not null
Scratch2Action->>LoggedInInfo: getLoggedInProviderNo()
alt providerNo is null
Scratch2Action->>Logger: error(Provider number not found in session)
Scratch2Action->>HttpServletResponse: setStatus(401)
Scratch2Action->>JSONObject: put(success, false)
Scratch2Action-->>User: jsonResponse(JSONObject)
else providerNo not null
Scratch2Action->>Scratch2Action: getParameter(id)
alt id is null or empty
Scratch2Action->>JSONObject: put(success, false)
Scratch2Action-->>User: jsonResponse(JSONObject)
else id present
Scratch2Action->>ScratchPadDao: find(id)
alt scratch is null
ScratchPadDao-->>Scratch2Action: null
Scratch2Action->>JSONObject: put(success, false)
Scratch2Action-->>User: jsonResponse(JSONObject)
else scratch found
ScratchPadDao-->>Scratch2Action: ScratchPad
alt providerNo != scratch.providerNo
Scratch2Action->>Logger: error(User attempted to delete scratch pad owned by another)
Scratch2Action->>HttpServletResponse: setStatus(403)
Scratch2Action->>JSONObject: put(success, false)
Scratch2Action-->>User: jsonResponse(JSONObject)
else providerNo == scratch.providerNo
Scratch2Action->>ScratchPad: setStatus(false)
Scratch2Action->>ScratchPadDao: merge(scratch)
Scratch2Action->>JSONObject: put(id, encodedId)
Scratch2Action->>JSONObject: put(success, true)
Scratch2Action-->>User: jsonResponse(JSONObject)
end
end
end
end
end
Sequence diagram for Scratch2Action execute using LoggedInInfo and provider validationsequenceDiagram
actor User
participant Scratch2Action
participant LoggedInInfo
participant HttpServletResponse
participant Logger
User->>Scratch2Action: execute(request)
alt method is delete
Scratch2Action->>Scratch2Action: delete()
Scratch2Action-->>User: delete result
else normal execute
Scratch2Action->>LoggedInInfo: getLoggedInInfoFromSession(request)
alt loggedInInfo is null
Scratch2Action->>Logger: error(Invalid or expired session)
Scratch2Action->>HttpServletResponse: setStatus(401)
Scratch2Action-->>User: null
else loggedInInfo not null
Scratch2Action->>LoggedInInfo: getLoggedInProviderNo()
alt providerNo is null
Scratch2Action->>Logger: error(Provider number not found in session)
Scratch2Action->>HttpServletResponse: setStatus(401)
Scratch2Action-->>User: null
else providerNo not null
Scratch2Action->>Scratch2Action: getParameter(providerNo)
alt providerNo equals request providerNo
Scratch2Action->>Scratch2Action: process scratch pad create or update
Scratch2Action-->>User: success response
else providerNo mismatch
Scratch2Action->>Logger: error(User attempted to act on another provider scratch pad)
Scratch2Action->>HttpServletResponse: setStatus(403)
Scratch2Action-->>User: null
end
end
end
end
Updated class diagram for Scratch2Action session and authorization handlingclassDiagram
class Scratch2Action {
- ScratchPadDao scratchPadDao
+ String showVersion()
+ String execute()
+ String delete()
}
class LoggedInInfo {
+ static LoggedInInfo getLoggedInInfoFromSession(HttpServletRequest request)
+ String getLoggedInProviderNo()
}
class ScratchPadDao {
+ ScratchPad find(Integer id)
+ void merge(ScratchPad scratchPad)
}
class ScratchPad {
+ String getProviderNo()
+ void setStatus(Boolean status)
}
class HttpServletRequest
class HttpServletResponse {
+ void setStatus(int status)
}
class JSONObject {
+ void put(String key, Object value)
}
Scratch2Action --> ScratchPadDao : uses
Scratch2Action --> LoggedInInfo : uses
Scratch2Action --> HttpServletRequest : reads parameters
Scratch2Action --> HttpServletResponse : sets status
Scratch2Action --> JSONObject : builds json responses
ScratchPadDao --> ScratchPad : manages
LoggedInInfo --> HttpServletRequest : reads session
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can make CodeRabbit's review stricter and more nitpicky using the `assertive` profile, if that's what you prefer.Change the |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes #1889 by replacing raw session attribute access with the proper
LoggedInInfopattern for session management and validation inScratch2Action.java.Changes
LoggedInInfoimportexecute()to useLoggedInInfo.getLoggedInInfoFromSession()delete()with session validation and ownership verificationshowVersion()with session validation and ownership verificationSecurity Improvements
Testing
Please test:
Fixes #1889
Generated with Claude Code
Summary by Sourcery
Strengthen Scratch2Action session handling and authorization by standardizing on LoggedInInfo-based validation for scratch pad operations.
Bug Fixes:
Enhancements:
Summary by cubic
Replaced raw session access in
Scratch2Actionwith theLoggedInInfopattern and enforced strict auth checks. Prevents unauthorized access and returns proper 401/403 responses. Fixes #1889.Refactors
LoggedInInfo.getLoggedInInfoFromSession()inexecute,delete, andshowVersion.Bug Fixes
Written for commit 5dc0a58. Summary will update on new commits.