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 realm to be set by server for basic auth. #124

Merged
merged 2 commits into from Sep 24, 2015

Conversation

Projects
None yet
2 participants
@vonrosen
Contributor

vonrosen commented Jul 30, 2015

No description provided.

Show outdated Hide outdated ...in/java/org/archive/modules/credential/HttpAuthenticationCredential.java
//if no realm specified, use the one returned by the server
if (c.getRealm() == null || c.getRealm().isEmpty()) {
c.setRealm(realm);

This comment has been minimized.

@nlevitt

nlevitt Aug 13, 2015

Member

Is it necessary to call c.setRealm()? (Heritrix's http auth code isn't the easiest to follow.) It seems ugly for this method to have the side effect of modifying the credential. In the event that the crawl encounters another 401 with a different realm, then this credential won't be usable for that. If it is necessary for the realm to be set, maybe make a copy of the credential here, call setRealm(), and return that one instead.

About the comment:
//if no realm specified, use the one returned by the server
I think this might be clearer to explain why it's doing what it's doing right here:
// empty realm field means the credential can be used for any realm specified by server

@nlevitt

nlevitt Aug 13, 2015

Member

Is it necessary to call c.setRealm()? (Heritrix's http auth code isn't the easiest to follow.) It seems ugly for this method to have the side effect of modifying the credential. In the event that the crawl encounters another 401 with a different realm, then this credential won't be usable for that. If it is necessary for the realm to be set, maybe make a copy of the credential here, call setRealm(), and return that one instead.

About the comment:
//if no realm specified, use the one returned by the server
I think this might be clearer to explain why it's doing what it's doing right here:
// empty realm field means the credential can be used for any realm specified by server

nlevitt added a commit that referenced this pull request Sep 24, 2015

Merge pull request #124 from vonrosen/ari-4371
Allow realm to be set by server for basic auth.

@nlevitt nlevitt merged commit 88c7f3c into internetarchive:master Sep 24, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment