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 REST-Authentication endpoints to send warnings for the AuthenticationResult #3686

Merged
merged 9 commits into from
Dec 30, 2018

Conversation

larsgrefer
Copy link
Contributor

@larsgrefer larsgrefer commented Dec 6, 2018

With this PR, RestAuthenticationHandler reads two custom HTTP headers which can contain warnings for the created AuthenticationHandlerExecutionResult:

  • X-Cas-PasswordExpirationDate will generate a PasswordExpiringWarningMessageDescriptor.
  • X-Cas-Warning will generate a DefaultMessageDescriptor (can be used multiple times)

@mmoayyed mmoayyed modified the milestones: 6.0.0, 6.1.0-RC1 Dec 6, 2018
@codecov
Copy link

codecov bot commented Dec 9, 2018

Codecov Report

Merging #3686 into master will decrease coverage by 0.79%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##             master   #3686     +/-   ##
==========================================
- Coverage        61%   60.2%   -0.8%     
+ Complexity     6133    6063     -70     
==========================================
  Files          1451    1451             
  Lines         30900   30911     +11     
  Branches       2867    2869      +2     
==========================================
- Hits          18849   18610    -239     
- Misses        10246   10511    +265     
+ Partials       1805    1790     -15
Impacted Files Coverage Δ Complexity Δ
...o/cas/adaptors/rest/RestAuthenticationHandler.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...services/CouchbaseRegisteredServiceSavedEvent.java 0% <0%> (-100%) 0% <0%> (-1%)
...rvices/CouchbaseRegisteredServiceDeletedEvent.java 0% <0%> (-100%) 0% <0%> (-1%)
...g/apereo/cas/audit/CouchbaseAuditTrailManager.java 0% <0%> (-100%) 0% <0%> (-5%)
...persondir/support/CouchbasePersonAttributeDao.java 0% <0%> (-75.87%) 0% <0%> (-10%)
.../apereo/cas/services/CouchbaseServiceRegistry.java 0% <0%> (-75.41%) 0% <0%> (-12%)
...o/cas/ticket/registry/CouchbaseTicketRegistry.java 0% <0%> (-71.77%) 0% <0%> (-16%)
...reo/cas/couchbase/core/CouchbaseClientFactory.java 0% <0%> (-71.06%) 0% <0%> (-15%)
...authentication/CouchbaseAuthenticationHandler.java 0% <0%> (-48.15%) 0% <0%> (-6%)
...DefaultPersonDirectoryAttributeRepositoryPlan.java 50% <0%> (-37.5%) 3% <0%> (-1%)
... and 3 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 8a9ae0d...b3d5edc. Read the comment docs.

@codecov
Copy link

codecov bot commented Dec 9, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@5617542). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3686   +/-   ##
=========================================
  Coverage          ?   60.87%           
  Complexity        ?     6127           
=========================================
  Files             ?     1452           
  Lines             ?    30957           
  Branches          ?     2881           
=========================================
  Hits              ?    18846           
  Misses            ?    10297           
  Partials          ?     1814
Impacted Files Coverage Δ Complexity Δ
...o/cas/adaptors/rest/RestAuthenticationHandler.java 0% <0%> (ø) 0 <0> (?)

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 5617542...09654b0. Read the comment docs.

Copy link
Member

@mmoayyed mmoayyed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is excellent. A few comments here and there, mostly on docs.

### `X-Cas-Warning`

For each `X-Cas-Warning` header present in the response, a corresponding `DefaultMessageDescriptor` is added
to the `AuthenticationHandlerExecutionResult`. The header value is used as `code` and `defaultMessage`.
Copy link
Member

@mmoayyed mmoayyed Dec 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather that describing what happens internally API-wise (which doesn't really belong here and would serve better as javadocs, etc), it would be much better if you actually describe the what and not the how. What does CAS do when you pass along the warnings? where are they processed? where do they show up? etc etc. Avoid using class names and try to use plain language as much as possible.

@larsgrefer
Copy link
Contributor Author

@mmoayyed Please review again

@mmoayyed
Copy link
Member

Looks great. Will merge at the right time. Thank you.

@stale
Copy link

stale bot commented Dec 19, 2018

This patch has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added Pending and removed Pending labels Dec 19, 2018
@stale
Copy link

stale bot commented Dec 24, 2018

This patch has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added Pending and removed Pending labels Dec 24, 2018
@stale
Copy link

stale bot commented Dec 29, 2018

This patch has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added Pending and removed Pending labels Dec 29, 2018
@mmoayyed mmoayyed merged commit 95de115 into apereo:master Dec 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants