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

Document the role of CredentialsContainer #15319

Closed
marcusdacoregio opened this issue Jun 28, 2024 · 2 comments
Closed

Document the role of CredentialsContainer #15319

marcusdacoregio opened this issue Jun 28, 2024 · 2 comments
Assignees
Labels
in: docs An issue in Documentation or samples type: enhancement A general enhancement
Milestone

Comments

@marcusdacoregio
Copy link
Contributor

The CredentialsContainer interface is used internally by the framework to clear the user's credentials after a successful authentication. However, there is no mention in the reference docs about this interface and whether users should implement it or not.

@marcusdacoregio marcusdacoregio added in: docs An issue in Documentation or samples type: enhancement A general enhancement labels Jun 28, 2024
@marcusdacoregio marcusdacoregio self-assigned this Jun 28, 2024
@marcusdacoregio marcusdacoregio added this to the 5.8.14 milestone Jun 28, 2024
@PhilHYChen
Copy link

If I may, I would like to offer my two cents:

  1. Add a "Note" box highlighting CredentialsContainer to "Sevlet Applications -> Authentication -> Authentication Architecture" under the second last paragraph of #ProviderManager.
    https://docs.spring.io/spring-security/reference/servlet/authentication/architecture.html#servlet-authentication-providermanager

  2. Add a new "Password Erasure" item, which provides the best practice and risk assessment regarding credential erasure, next to "Password Storage" in "Servlet Applications -> Authentication -> Username/Password" in the reference doc.
    https://docs.spring.io/spring-security/reference/servlet/authentication/passwords/storage.html

If the reference docs could provide a general guideline about credential erasure alongside its storage, developers could make their custom implementations with peace of mind.

  1. Add CredentialsContainer under the previous section in the reference doc.

  2. In the UserDetails reference & API doc, recommend developers to implement CredentialsContainer if they do not utilize caching.
    https://docs.spring.io/spring-security/reference/servlet/authentication/passwords/user-details.html
    https://docs.spring.io/spring-security/site/docs/current/api/org/springframework/security/core/userdetails/UserDetails.html

There is also another thing I'd like to mention. Though this is somewhat beyond the CredentialsContainer interface, when it comes to username/password authentication, the intended interface responsible for credential erasure (that is, if still desirable) in Spring Security seems vague. 

We can also look further beyond the interfaces and directly into some out-of-the-box implementations.

I apologize if I misunderstood the docs and the codes. I am genuinely confused here. Should either or neither my custom implementation of AuthenticationProvider or AuthenticationManager erase credentials? If I follow or deviate away from the contracts and mimic the official out-of-the-box implementations, do I risk breaking things?

Maybe it's just all a legacy thing, though if resources allow, a clean-up would probably help for better consistency.

@marcusdacoregio
Copy link
Contributor Author

Thanks for the suggestions @PhilHYChen. Would you like to provide a PR for items 1, 2, 3 and 4?

Instead, it relies on ProviderManager (Spring Security's default implementation of AuthenticationManager) to erase the credentials and other sensitive data. However, the AuthenticationManager API doc states that the implementation should return "a fully authenticated object including credentials" with the authenticate method.

I think that the information in the API doc is outdated. The AuthenticationProvider class is there since the beginning and a lot of things changed in the meantime, however the documentation wasn't updated accordingly. I believe it should just state that the method should return "a fully authenticated object".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: docs An issue in Documentation or samples type: enhancement A general enhancement
Projects
Status: No status
Development

No branches or pull requests

2 participants