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

Added flag to make wrapping username with domain optional (to support basic auth). #1290

Merged
merged 2 commits into from
Jun 12, 2023

Conversation

OrangeAndGreen
Copy link
Contributor

Technical Summary

Added a dedicated BasicAuth class that the Android code can use to perform basic HTTP auth without any additional wrapping. The Android code does a type check on the AuthInfo to determine what credentials to send up, and other sub-classes involve additional steps that don't work for basic auth.

Safety Assurance

Safety story

Simply adding a class here for commcare-android to use.

Automated test coverage

No new tests.

Special deploy instructions

  • This PR can be deployed after merge with no further considerations.

Rollback instructions

  • This PR can be reverted after deploy with no further considerations.

Review

  • The set of people pinged as reviewers is appropriate for the level of risk of the change.

Duplicate PR

Automatically duplicate this PR as defined in contributing.md.

@shubham1g5
Copy link
Contributor

@OrangeAndGreen Where can I see the corresponding Android code for this PR ? Basically want to see what changes in Android code necessitate the need of basic auth class as it looks exactly same as ProvidedAuth .

@OrangeAndGreen
Copy link
Contributor Author

Of course, that makes sense! The relevant change is in the dv/connect_id branch of commcare-android. The usage is in HttpUtils.getCredential, where the case for ProvidedAuth wraps the username using buildDomainUser(). I added a case for BasicAuth that passes the username straight through.

@shubham1g5
Copy link
Contributor

That makes sense, one other way to do that would be to just add a wrapDomain flag to the current auth class defaulting it to true. I don't have a strong preference between the two but if we want to add another class which is potentially just for connect, I think we should have it in a connect specific package and probably rename it to ConnectAuth or something like that which makes it obvious this should not be used for normal CC pathways.

@OrangeAndGreen
Copy link
Contributor Author

Ah, I think I like your wrapDomain approach better! It solves the problem, keeps it more generic, and also prevents the need for a new class. I'll make that change.

… domain (instead of additional BasicAuth class).
@OrangeAndGreen
Copy link
Contributor Author

Ok, change made and ready for review.

Copy link
Contributor

@shubham1g5 shubham1g5 left a comment

Choose a reason for hiding this comment

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

Thanks Dave this looks good. Couple things around core PRs -

  1. It's a good practice to PR the corresponding Android/Formplayer repo change along with the Core changes and cross-request them to each other. That way we have a history of why a change was made in core.

  2. We need to duplicate all the core PRs across formplayer and master. The process for that is outlined here.

@OrangeAndGreen
Copy link
Contributor Author

cross-request: dimagi/commcare-android#2660

@OrangeAndGreen OrangeAndGreen changed the title Added BasicAuth subclass of AuthInfo, to support basic HTTP auth. Added flag to make wrapping username with domain optional (to support basic auth). Jun 9, 2023
@OrangeAndGreen
Copy link
Contributor Author

Duplicate this PR

@OrangeAndGreen OrangeAndGreen merged commit 943a4d7 into master Jun 12, 2023
@OrangeAndGreen OrangeAndGreen deleted the dv/basic_auth branch June 12, 2023 19:50
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.

2 participants