SAK-50562 Section Info Students can skip date-based access restrictions with URLs

[st-manu]

https://sakaiproject.atlassian.net/browse/SAK-50562

Summary by CodeRabbit

• Bug Fixes

  • Student section views now respect the site's open date: access is allowed only after the designated open time.
  • Sites with no configured open date are treated as open (students can access).
  • Fixed potential runtime error when evaluating access timing to ensure reliable routing.

• Chores

  • Improved diagnostic logging to aid troubleshooting.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Added null-safe open-date checks and enforcement for student view routing and authorization, replaced a servletPath indexOf check with contains, and updated logging and imports in RoleFilter and EntryServlet.

Changes

Cohort / File(s)	Summary
RoleFilter — authorization & imports sections/sections-app/src/java/org/sakaiproject/tool/section/filter/RoleFilter.java	Added Calendar (and SectionManager usage) import and reorganized imports; replaced servletPath.indexOf("/") >= 0 with servletPath.contains("/"); updated debug logging to parameterized messages and added context logs; added open-date retrieval and authorization logic for studentView under ViewOwnSections (open == null → authorized; now before open → denied; otherwise authorized).
EntryServlet — null-safe routing & logging sections/sections-app/src/java/org/sakaiproject/tool/section/EntryServlet.java	Removed guarded isDebugEnabled checks and now logs unconditionally at two call sites; added null-check for section open date before time comparison and routes to studentView when open is null, otherwise routes to closed or studentView based on current time.

Suggested reviewers

• ottenhoff
• jesusmmp

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly references the security issue (SAK-50562) and accurately describes the main change: fixing a vulnerability where students could bypass date-based access restrictions using URLs.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f8e632f and d0402ef.

📒 Files selected for processing (2)

• sections/sections-app/src/java/org/sakaiproject/tool/section/EntryServlet.java (2 hunks)
• sections/sections-app/src/java/org/sakaiproject/tool/section/filter/RoleFilter.java (5 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• sections/sections-app/src/java/org/sakaiproject/tool/section/EntryServlet.java
• sections/sections-app/src/java/org/sakaiproject/tool/section/filter/RoleFilter.java

⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

• GitHub Check: maven-build
• GitHub Check: maven-build
• GitHub Check: sakai-deploy

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

• Provide your own instructions using the high_level_summary_instructions setting.
• Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
• Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
4. 📊 Contributor Summary — Include a Markdown table showing contributions:
   | Contributor | Lines Added | Lines Removed | Files Changed |
5. ✔️ Additional Notes — Add any extra reviewer context.
   Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d18b952 and b5984db.

📒 Files selected for processing (1)

• sections/sections-app/src/java/org/sakaiproject/tool/section/filter/RoleFilter.java (4 hunks)

🧰 Additional context used

🧠 Learnings (1)

📓 Common learnings

Learnt from: ottenhoff
Repo: sakaiproject/sakai PR: 0
File: :0-0
Timestamp: 2025-10-07T15:11:27.298Z
Learning: In samigo’s Total Scores view (samigo/samigo-app/src/webapp/jsf/evaluation/totalScores.jsp), mailto links were hidden after commit dee05746 (PR #12312, SAK-49674) added a render check requiring email.fromEmailAddress to be non-empty; PR #14154 (SAK-52058) restores visibility by checking only description.email.

⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

• GitHub Check: maven-build
• GitHub Check: maven-build
• GitHub Check: sakai-deploy

🔇 Additional comments (2)

sections/sections-app/src/java/org/sakaiproject/tool/section/filter/RoleFilter.java (2)

24-24: LGTM! Necessary imports for open-date validation.

The additional imports (Calendar, SectionManager, Spring context classes) are required for the new date-based authorization logic.

Also applies to: 35-35, 39-42

---

81-81: Good refactor for readability.

Replacing indexOf("/") >= 0 with contains("/") is more idiomatic and makes the intent clearer while preserving the same behavior.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

sections/sections-app/src/java/org/sakaiproject/tool/section/EntryServlet.java (1)

64-78: Date-based student routing looks correct; consider centralizing logic and normalizing log text

The open-date handling here is null-safe and matches the intended semantics (no date ⇒ allow, before open ⇒ /closed.jsf, after open ⇒ /studentView.jsf), aligning with RoleFilter’s authorization logic.

To reduce future drift, consider extracting this open-date check into a shared helper/service so EntryServlet and RoleFilter don’t need to duplicate the rules. Also, the debug string "Grupos Cerrados..." is the only non-English text in this block; you may want to switch to English or an i18n message key for consistency with the rest of the tool.

sections/sections-app/src/java/org/sakaiproject/tool/section/filter/RoleFilter.java (1)

121-138: Date-based authorization for studentView correctly enforces access; consider minor cleanup and logging

The new block under the viewOwnSections branch:

• Properly treats open == null as “no restriction” and authorizes.
• Denies access when now.before(open) and allows otherwise.
• Aligns with EntryServlet’s routing behavior and removes the previous NPE risk around now.before(open) with a null open.

Two small, non-blocking suggestions:

• You could simplify the inner logic slightly, e.g. isAuthorized = !now.before(open);, to reduce branching.
• Consider adding a specific log message when access is denied due to the open date (vs general authorization failure), which would help distinguish configuration issues from true permission problems in logs.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b5984db and f8e632f.

📒 Files selected for processing (2)

• sections/sections-app/src/java/org/sakaiproject/tool/section/EntryServlet.java (1 hunks)
• sections/sections-app/src/java/org/sakaiproject/tool/section/filter/RoleFilter.java (4 hunks)

🧰 Additional context used

🧠 Learnings (1)

📓 Common learnings

Learnt from: ottenhoff
Repo: sakaiproject/sakai PR: 0
File: :0-0
Timestamp: 2025-10-07T15:11:27.298Z
Learning: In samigo’s Total Scores view (samigo/samigo-app/src/webapp/jsf/evaluation/totalScores.jsp), mailto links were hidden after commit dee05746 (PR #12312, SAK-49674) added a render check requiring email.fromEmailAddress to be non-empty; PR #14154 (SAK-52058) restores visibility by checking only description.email.

🧬 Code graph analysis (1)

sections/sections-app/src/java/org/sakaiproject/tool/section/filter/RoleFilter.java (1)

sections/sections-app/src/java/org/sakaiproject/tool/section/EntryServlet.java (1)

• Slf4j (35-95)

⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

• GitHub Check: sakai-deploy
• GitHub Check: maven-build
• GitHub Check: maven-build

🔇 Additional comments (2)

sections/sections-app/src/java/org/sakaiproject/tool/section/filter/RoleFilter.java (2)

23-42: ApplicationContext field and Lombok logger integration look fine

Storing ApplicationContext ac at init time and using Lombok’s @Slf4j for logging is appropriate for a servlet filter and doesn’t introduce threading issues, since ac is effectively read-only after init. No concerns with the added imports.

Also applies to: 60-60

---

81-81: servletPath.contains("/") keeps behavior while improving readability

Switching from servletPath.indexOf("/") >= 0 to servletPath.contains("/") after stripping the leading slash preserves the existing behavior (only top-level paths are protected) and is easier to read. No functional issues here.