Skip to content
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

Allow error pages to use the default theme specified in cas.properties #4963

Merged
merged 1 commit into from Oct 22, 2020

Conversation

marwatk
Copy link
Contributor

@marwatk marwatk commented Oct 21, 2020

  • Brief description of changes applied
  • Test cases for all modified changes, where applicable
  • The same pull request targeted at the master branch, if applicable
  • Any documentation on how to configure, test
  • Any possible limitations, side effects, etc
  • Reference any other pull requests that might be related

This is the master branch version of #4962:

We noticed a bug that error pages (404, 500) wouldn't honor the default theme set in cas.properties. You can verify this by running a custom theme (even just a different CSS background color) and then trying to visit login2 instead of login.

This MR fixes that issue along with test cases to prove it. I'll follow up with a master version as well.

We also noticed that error page handling falls through to SpringTemplateResolver instead of using ChainingTemplateViewResolver which then caches the resulting view. This prevents custom theme views from ever appearing again (specifically the header). I spent a little time trying to determine why only the error pages would exhibit that behavior but came up empty (though, admittedly I didn't spend a ton of time since we only use a simple theme and fixing the error page 'solves' it for us). Future work may want to investigate that behavior.

@codecov
Copy link

codecov bot commented Oct 21, 2020

Codecov Report

Merging #4963 into master will decrease coverage by 37.20353%.
The diff coverage is 0.00000%.

Impacted file tree graph

@@                  Coverage Diff                  @@
##                master      #4963          +/-   ##
=====================================================
- Coverage     40.33519%   3.13166%   -37.20353%     
+ Complexity        7386        697        -6689     
=====================================================
  Files             2410       2410                  
  Lines            55371      55370           -1     
  Branches          4544       4544                  
=====================================================
- Hits             22334       1734       -20600     
- Misses           31475      53473       +21998     
+ Partials          1562        163        -1399     
Flag Coverage Δ Complexity Δ
#actuator ? ?
#attributes ? ?
#audit ? ?
#cas ? ?
#cas-config ? ?
#cassandra ? ?
#cosmosdb 0.04334% <0.00000%> (ø) 10.00000 <0.00000> (ø)
#couchbase ? ?
#ehcache ? ?
#expiration-policy ? ?
#hazelcast ? ?
#ignite 2.99440% <0.00000%> (+0.00006%) 660.00000 <0.00000> (ø)
#infinispan 2.77407% <0.00000%> (-0.01439%) 639.00000 <0.00000> (-4.00000)
#influxdb ? ?
#jms ? ?
#jmx ? ?
#kafka ? ?
#logout ? ?
#mail ? ?
#mariadb ? ?
#memcached ? ?
#metrics ? ?
#mssql ? ?
#oauth ? ?
#oidc ? ?
#oracle 0.00722% <0.00000%> (ø) 1.00000 <0.00000> (ø)
#password-ops ? ?
#radius ? ?
#services ? ?
#shell ? ?
#sms ? ?
#spnego ? ?
#tickets ? ?
#uma ? ?
#util ? ?
#web ? ?
#zookeeper ? ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
...cas/web/view/ThemeClassLoaderTemplateResolver.java 0.00000% <0.00000%> (-70.58823%) 0.00000 <0.00000> (-4.00000)
...apereo/cas/web/view/ThemeFileTemplateResolver.java 0.00000% <0.00000%> (-73.33333%) 0.00000 <0.00000> (-4.00000)
...i/src/main/java/org/apereo/cas/util/JsonUtils.java 0.00000% <0.00000%> (-100.00000%) 0.00000% <0.00000%> (-4.00000%)
.../src/main/java/org/apereo/cas/util/RegexUtils.java 0.00000% <0.00000%> (-100.00000%) 0.00000% <0.00000%> (-13.00000%)
...rc/main/java/org/apereo/cas/util/LoggingUtils.java 0.00000% <0.00000%> (-100.00000%) 0.00000% <0.00000%> (-4.00000%)
...ain/java/org/apereo/cas/util/http/HttpMessage.java 0.00000% <0.00000%> (-100.00000%) 0.00000% <0.00000%> (-4.00000%)
...in/java/org/apereo/cas/audit/AuditableContext.java 0.00000% <0.00000%> (-100.00000%) 0.00000% <0.00000%> (-10.00000%)
.../java/org/apereo/cas/web/view/DynamicHtmlView.java 0.00000% <0.00000%> (-100.00000%) 0.00000% <0.00000%> (-2.00000%)
...n/java/org/apereo/cas/ticket/ExpirationPolicy.java 0.00000% <0.00000%> (-100.00000%) 0.00000% <0.00000%> (-1.00000%)
...n/java/org/apereo/cas/ticket/TicketDefinition.java 0.00000% <0.00000%> (-100.00000%) 0.00000% <0.00000%> (-1.00000%)
... and 1289 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88019ce...55f1f03. Read the comment docs.

@mmoayyed mmoayyed merged commit 651c0a7 into apereo:master Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants