VPR-151 feat(auth): Dynamic login screen#216
Open
rlorenzo wants to merge 2 commits into
Open
Conversation
Bundle ReportChanges will increase total bundle size by 59.38kB (2.78%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: viper-frontend-esmAssets Changed:
Files in
Files in
Files in
Files in
Files in
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #216 +/- ##
==========================================
+ Coverage 43.02% 43.16% +0.13%
==========================================
Files 881 883 +2
Lines 51437 51571 +134
Branches 4812 4841 +29
==========================================
+ Hits 22131 22259 +128
+ Misses 28780 28779 -1
- Partials 526 533 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
rlorenzo
force-pushed
the
feature/VPR-151-dynamic-login-screen
branch
from
564a68b to
e13dba7
Compare
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
📝 WalkthroughWalkthroughIntroduces a new welcome landing page with server-side return-URL validation to prevent redirect loops and open redirects. Backend validates local URLs and sanitizes return parameters in login flows. Frontend Vue components and shared views redirect to the new welcome route. Adds welcome page styling, branded header assets, and font serving infrastructure. ChangesWelcome Page and Login Flow Redesign
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
VueApp/src/composables/RequireLogin.ts (1)
15-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the stale auth-flow comments.
These comments still describe a direct login/CAS redirect, but the implementation now sends users through
/welcome. Please update them together so the next edit follows the actual flow instead of the old one.As per coding guidelines:
**/*.{cs,ts,tsx,vue}: Write comments explaining why, not what. Use sparingly for complex logic only.Also applies to: 107-118
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@VueApp/src/composables/RequireLogin.ts` around lines 15 - 18, Replace the stale descriptive comments that mention a direct CAS redirect with a concise "why" comment explaining that the code validates a requested return path to prevent open-redirects and then routes users through the centralized onboarding entrypoint (/welcome) as a stable, canonical post-login flow; update both the comment block above the login URL builder and the similar block later (the comments adjacent to the buildLoginUrl / requireLogin functions) to state why the fallback to /welcome exists and why path validation is necessary, avoiding restating what the code does.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/Classes/Utilities/WelcomePageHelperTests.cs`:
- Around line 7-23: The test suite is missing a case for tilde-rooted URLs
(e.g., "~/Area/...") which IsLocalUrl accepts; update the
WelcomePageHelperTests.ResolveDestinationLabel_KnownAndFallbackCases theory data
to include a tilde-rooted input (for example "~/RAPS/Roles") and assert the same
expected label as the non-tilde form (e.g., "RAPS") so
WelcomePageHelper.ResolveDestinationLabel is verified to handle tilde-rooted
parsing correctly.
In `@VueApp/src/CAHFS/pages/CAHFSAuth.vue`:
- Around line 5-25: Replace the raw q-banner block in CAHFSAuth.vue with the
shared StatusBanner component: remove the <q-banner> usage and render
<StatusBanner> providing the equivalent avatar (use the avatar slot to render
the <q-icon name="login" color="primary"/>), set the title to "Members sign-in",
set the message text to "Sign in with your UC Davis account to open web reports
and section pages.", and render the sign-in action using the existing loginHref
(use a button/link slot or an action prop to create the Sign in button with
href=loginHref, color="primary", no-caps, label="Sign in"); do not change the
loginHref variable name and let StatusBanner handle the correct ARIA role
mapping (type prop only if you need a specific role).
In `@VueApp/src/CTS/pages/CtsHome.vue`:
- Around line 52-55: The redirect currently hard-codes "CTS/" and drops any
query string, so the ReturnUrl loses parameters (breaking later logic that reads
route.query.sendBackTo); change the assignment to preserve the full original
path and query by using the current full route (e.g., use route.fullPath or
window.location.pathname + window.location.search) when building the ReturnUrl
instead of the fixed "CTS/". Update the window.location.href construction that
uses import.meta.env.VITE_VIPER_HOME and encodeURIComponent to append the
preserved full path (reference: window.location.href assignment,
VITE_VIPER_HOME, encodeURIComponent, and route.query.sendBackTo).
In `@VueApp/src/styles/base.css`:
- Line 254: Replace the malformed comment string "/*Left Nav styles*/" with a
properly spaced block comment so it passes Stylelint (change to "/* Left Nav
styles */"); update the comment occurrence in the CSS (the comment containing
Left Nav styles) to include a leading and trailing space inside the /* */
tokens.
In `@web/Classes/Utilities/WelcomePageHelper.cs`:
- Around line 43-60: The helper incorrectly handles "~/..." URLs because
path.TrimStart('/') leaves a leading '~', causing segments[0] == "~" and
skipping AreaLabels; update the normalization to strip a leading '~' as well
(e.g., use path = path.TrimStart('~','/')) so
AreaLabels.TryGetValue(segments[0], out var label) works for "~/"-rooted URLs;
keep the existing fallback ToTitleCase(segments[^1]) behavior and ensure this
change is applied in the method that currently calls TrimStart('/') in
WelcomePageHelper (referencing AreaLabels and ToTitleCase).
In `@web/Views/Shared/Components/ProfilePic/Default.cshtml`:
- Line 48: The q-btn currently links to Url.Content("~/welcome") without
preserving the current location; update the href on the q-btn in Default.cshtml
to include a URL-encoded ReturnUrl query parameter (e.g. return Url-encode the
current request path/Url.Action/Request.RawUrl) so the welcome/login flow can
redirect back; specifically modify the q-btn href generation (the element using
Url.Content("~/welcome")) to append ?ReturnUrl={HttpUtility.UrlEncode(/* current
path */)} or equivalent helper so existing context is preserved after auth.
In `@web/wwwroot/css/site.css`:
- Line 185: The comment block "/*Left Nav styles*/" violates the
comment-whitespace-inside rule; update that block comment (the string /*Left Nav
styles*/ in the CSS) to include spaces like "/* Left Nav styles */" so Stylelint
no longer flags it.
In `@web/wwwroot/css/welcome.css`:
- Line 107: The .sr-only rule uses the deprecated clip property; update the
.sr-only CSS selector to add a modern fallback by adding clip-path: inset(50%)
alongside the existing clip value (keep clip as the fallback), ensuring the rule
still hides visually but remains accessible; locate the .sr-only selector in
welcome.css and insert clip-path: inset(50%) (and include any necessary vendor
prefixes if desired) while preserving the existing clip line as fallback.
---
Outside diff comments:
In `@VueApp/src/composables/RequireLogin.ts`:
- Around line 15-18: Replace the stale descriptive comments that mention a
direct CAS redirect with a concise "why" comment explaining that the code
validates a requested return path to prevent open-redirects and then routes
users through the centralized onboarding entrypoint (/welcome) as a stable,
canonical post-login flow; update both the comment block above the login URL
builder and the similar block later (the comments adjacent to the buildLoginUrl
/ requireLogin functions) to state why the fallback to /welcome exists and why
path validation is necessary, avoiding restating what the code does.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 08d96d9a-c8a2-46ce-9a92-8e0dff022f87
⛔ Files ignored due to path filters (19)
VueApp/src/assets/UCDSVMLogo.pngis excluded by!**/*.pngVueApp/src/assets/fonts/proxima-nova/proximanova-bold.woff2is excluded by!**/*.woff2VueApp/src/assets/fonts/proxima-nova/proximanova-extrabold.woff2is excluded by!**/*.woff2VueApp/src/assets/fonts/proxima-nova/proximanova-medium.woff2is excluded by!**/*.woff2VueApp/src/assets/fonts/proxima-nova/proximanova-regular.woff2is excluded by!**/*.woff2VueApp/src/assets/logo-vetmed-stacked-lockup.pngis excluded by!**/*.pngVueApp/src/assets/rod-of-asclepius-white.pngis excluded by!**/*.pngweb/wwwroot/fonts/proxima-nova/proximanova-bold.woff2is excluded by!**/*.woff2web/wwwroot/fonts/proxima-nova/proximanova-extrabold.woff2is excluded by!**/*.woff2web/wwwroot/fonts/proxima-nova/proximanova-medium.woff2is excluded by!**/*.woff2web/wwwroot/fonts/proxima-nova/proximanova-regular.woff2is excluded by!**/*.woff2web/wwwroot/images/UCDSVMLogo.pngis excluded by!**/*.pngweb/wwwroot/images/login/photo-guinea-pig.jpgis excluded by!**/*.jpgweb/wwwroot/images/login/photo-horse-foal.jpgis excluded by!**/*.jpgweb/wwwroot/images/login/photo-ophthalmology.jpgis excluded by!**/*.jpgweb/wwwroot/images/login/photo-svm-building.jpgis excluded by!**/*.jpgweb/wwwroot/images/login/photo-vetmed-admin.jpgis excluded by!**/*.jpgweb/wwwroot/images/login/rod-of-asclepius-white.pngis excluded by!**/*.pngweb/wwwroot/images/logo-vetmed-stacked-lockup.pngis excluded by!**/*.png
📒 Files selected for processing (28)
VueApp/src/CAHFS/pages/CAHFSAuth.vueVueApp/src/CTS/pages/CtsHome.vueVueApp/src/assets/logo-vetmed-stacked-lockup.avifVueApp/src/assets/rod-of-asclepius-white.avifVueApp/src/components/GenericError.vueVueApp/src/composables/RequireLogin.tsVueApp/src/layouts/ViperLayout.vueVueApp/src/layouts/ViperLayoutSimple.vueVueApp/src/styles/base.csstest/Classes/Utilities/WelcomePageHelperTests.cstest/Controllers/HomeControllerTests.csweb/Classes/Utilities/WelcomePageHelper.csweb/Controllers/HomeController.csweb/Program.csweb/Views/Home/Welcome.cshtmlweb/Views/Shared/Components/ProfilePic/Default.cshtmlweb/Views/Shared/Components/SessionTimeout/Default.cshtmlweb/Views/Shared/_VIPERLayout.cshtmlweb/Views/Shared/_ViperBrand.cshtmlweb/wwwroot/css/site.cssweb/wwwroot/css/welcome.cssweb/wwwroot/images/login/photo-guinea-pig.avifweb/wwwroot/images/login/photo-horse-foal.avifweb/wwwroot/images/login/photo-ophthalmology.avifweb/wwwroot/images/login/photo-svm-building.avifweb/wwwroot/images/login/photo-vetmed-admin.avifweb/wwwroot/images/login/rod-of-asclepius-white.avifweb/wwwroot/images/logo-vetmed-stacked-lockup.avif
💤 Files with no reviewable changes (1)
- VueApp/src/layouts/ViperLayoutSimple.vue
rlorenzo
force-pushed
the
feature/VPR-151-dynamic-login-screen
branch
from
e13dba7 to
cc03e60
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a new unauthenticated “Welcome” landing page at /welcome (and uses it as the cookie auth LoginPath) to replace immediate redirects to CAS /login, while also refreshing SVM branding (new shared lockup, font change, and updated layout styling) across both Razor and Vue/Quasar surfaces.
Changes:
- Add
/welcomelanding page flow (Razor + Vue) and route unauthenticated entry points to it, while tighteningReturnUrlvalidation to mitigate open redirects and redirect loops. - Refresh branding and layout chrome (shared
_ViperBrandlockup, Proxima Nova self-hosting, updated header/footer styles, and environment badges). - Add unit tests covering the welcome flow, redirect-loop guard, and
ReturnUrlprotections.
Reviewed changes
Copilot reviewed 19 out of 47 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| web/wwwroot/css/welcome.css | New standalone CSS for the unauthenticated welcome splash (fonts, tokens, responsive hero). |
| web/wwwroot/css/site.css | Adds Proxima Nova + shared .viper-brand* styling and a few layout tweaks. |
| web/Views/Shared/Components/SessionTimeout/Default.cshtml | Session-timeout login redirect now targets /welcome. |
| web/Views/Shared/Components/ProfilePic/Default.cshtml | Profile button login link now targets /welcome with ReturnUrl. |
| web/Views/Shared/_VIPERLayout.cshtml | Replaces legacy logo with _ViperBrand, adds env badges, updates login link to /welcome. |
| web/Views/Shared/_ViperBrand.cshtml | New shared partial for the SVM brand lockup used by welcome + layout. |
| web/Views/Home/Welcome.cshtml | New unauthenticated welcome page view with hero + sign-in CTA. |
| web/Program.cs | Sets cookie LoginPath to /welcome and adds /fonts static-file mapping with cache headers. |
| web/Controllers/HomeController.cs | Adds Welcome action + hero selection, validates ReturnUrl in login/CAS flow. |
| web/Classes/Utilities/WelcomePageHelper.cs | New helper to derive a user-friendly destination label from ReturnUrl. |
| VueApp/src/styles/base.css | Adds Proxima Nova + .viper-brand* styling for SPA header chrome. |
| VueApp/src/layouts/ViperLayoutSimple.vue | Removes dead placeholder markup in the simple layout. |
| VueApp/src/layouts/ViperLayout.vue | Adds new brand lockup markup and adjusts unauthenticated-view toggling. |
| VueApp/src/CTS/pages/CtsHome.vue | Redirects unauthenticated users to /welcome with encoded ReturnUrl. |
| VueApp/src/composables/RequireLogin.ts | Updates SPA login URL builder to route through /welcome. |
| VueApp/src/components/GenericError.vue | Updates login link to /welcome with encoded ReturnUrl. |
| VueApp/src/CAHFS/pages/CAHFSAuth.vue | Adds a “Members sign-in” card that links to the new welcome/login flow. |
| test/Controllers/HomeControllerTests.cs | New tests for welcome/login redirect protections and loop-guard behavior. |
| test/Classes/Utilities/WelcomePageHelperTests.cs | New tests for destination label resolution and local-URL rejection. |
Comments suppressed due to low confidence (1)
VueApp/src/styles/base.css:193
#mainLayoutHeaderis set tofont-weight: 500for Proxima Nova (andweb/wwwroot/css/site.csssets.mainLayoutViperto 500), but the SPA.mainLayoutViperrule forcesfont-weight: normal, overriding the intended medium weight in the header text. This causes inconsistent typography between Razor and SPA chrome.
.mainLayoutViper {
font-size: 1.6em !important;
font-weight: normal;
vertical-align: bottom;
margin-left: 0.6em;
}
176f830 to
b0e750d
Compare
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
rlorenzo
force-pushed
the
feature/VPR-151-dynamic-login-screen
branch
from
b0e750d to
99a921f
Compare
- Replace direct /login redirect with /welcome landing page across Razor and Vue (cookie LoginPath, profile button, session timeout, generic error, RequireLogin composable) - Validate ReturnUrl with Url.IsLocalUrl on Login/TwoFactor to close open-redirect paths - Refresh SVM branding: shared _ViperBrand lockup (rod mark + stacked school name) replaces UCDSVMLogo on the welcome page and main layout; self-host Proxima Nova (woff2) in place of Google Fonts Roboto - Add AVIF hero/brand images with JPEG/PNG fallback and recompress the login photos (~60% smaller); preload the AVIF hero with a type hint - Serve /fonts with a year-long immutable Cache-Control header - Add Dev/Test environment badges to the layout toolbar and a members sign-in banner to the CAHFS auth page - Drop the dead v-show="false" loading-placeholder markup duplicated in both Vue layouts (the real skeleton lives in the Razor shells)
rlorenzo
force-pushed
the
feature/VPR-151-dynamic-login-screen
branch
from
99a921f to
3f36a85
Compare
Self-hosted fonts and the login hero photos used root-absolute URLs (/fonts/..., /images/login/...) that resolve against the origin root, bypassing the /2 path base on TEST/PROD and hitting the legacy site — which returns HTML, so the fonts fail to decode and the hero background never loads. Local dev (served at root) masked this. Base-relative ../ URLs resolve against the stylesheet location and work under any base. - Also fixes the mirrored Proxima Nova block in site.css, so the authenticated chrome stops falling back to a system font on TEST
Contributor
Author
|
@bsedwards This has been merged into TEST and is ready for user evaluation/feedback. |
Collaborator
I like the way this looks. Do you think it's possible for certain routes / pages to go directly to CAS? I'm thinking of:
|
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
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.
Summary
Sends unauthenticated users to a new
/welcomelanding page instead of redirecting straight to CAS/login, and refreshes the SVM branding across the welcome page and app chrome.Changes
Auth / routing
/loginredirect with a/welcomelanding page across Razor and Vue (cookieLoginPath, profile button, session timeout, generic error,RequireLogincomposable)ReturnUrlwithUrl.IsLocalUrlon Login/TwoFactor to close open-redirect pathsBranding
_ViperBrandlockup (rod-of-asclepius mark + stacked school name) replacesUCDSVMLogoon the welcome page and main layout/fontsserved with a 1-year immutableCache-ControlCleanup
v-show="false"loading-placeholder markup duplicated in both Vue layouts (the real skeleton lives in the Razor shells)Testing
npm run lint,npm run test(backend + frontend), and build verification.Deployment
Per repo flow, merge into
Developmentto deploy to TEST for review before merging tomain.