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

Refs #28215 -- Marked credentials as a sensitive variables. #12640

Merged
merged 1 commit into from Oct 28, 2020

Conversation

hramezani
Copy link
Member

@hramezani hramezani commented Mar 29, 2020

Continue #10921
Addressed reviewer comments.
I've created setUp function in test and defined password there to don't use a password like 'secret' + 'test'.
Also, I think there is no need for third test, So, I removed it.

@collinanderson
Copy link
Contributor

collinanderson commented Apr 2, 2020

@hramezani Thank you for picking this up!

@hramezani
Copy link
Member Author

@hramezani Thank you for picking this up!

@collinanderson You are welcome.
I just completed it. The main effort was yours.
Thank you for the initial patch.

Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Hi @hramezani (hi @collinanderson 👋 ) This looks great, thanks!

Two questions:

  • Can we call this Fixed ... — there's not much else we could do here right?
  • Should ModelBackend.authenticate() be marked with sensitive_variables as part of this? (I'm not really seeing the benefit of marking the test TypeErrorBackend otherwise...

Thanks.

Co-authored-by: Collin Anderson <collin@onetencommunications.com>
@hramezani
Copy link
Member Author

Hi @carltongibson

Can we call this Fixed ... — there's not much else we could do here right?

I think this patch addresses just the case that we have an error during the login.

Should ModelBackend.authenticate() be marked with sensitive_variables as part of this?

I don't think so. Why should we mark ModelBackend.authenticate() as well?

I'm not really seeing the benefit of marking the test TypeErrorBackend otherwise...

We are going to test sensitive variables when we have an error in login. So, I think the TypeErrorBackend does it for us and we can use it.

I rebased the patch and added @collinanderson as Co-authored because he was the initiator of the patch and I just completed it.

@carltongibson
Copy link
Member

OK, super. Thanks both.

@carltongibson carltongibson merged commit 4eb7567 into django:master Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants