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

http: refactor http auth into static functions #12272

Closed
wants to merge 1 commit into from

Conversation

Gottox
Copy link
Contributor

@Gottox Gottox commented Nov 5, 2023

Based on #12262 I factored the auth functions out into its own functions. While doing that I noticed that the auth code could be turned into a abstract interface with multiple implementors. Are you peps interested in this?

The changes I currently did are purely cosmetic (given, that the compiler is smart enough to do proper inlining/dead code elimination), but if I'm implementing an abstract interface, this would introduce an indirection between the http code and the http-auth code.

This PR is opened as a draft for now to discuss if my approach makes sense.

@github-actions github-actions bot added the HTTP label Nov 5, 2023
@Gottox Gottox force-pushed the improve/generic-http-auth branch 3 times, most recently from e726a5a to fdbf3d5 Compare November 5, 2023 16:17
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

  1. Needs a rebase
  2. Seems to have broken test 1204

authinfo->data->state.authproblem = TRUE;
}
}
return true;
Copy link
Member

Choose a reason for hiding this comment

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

We use TRUE and FALSE (uppercase).

Comment on lines +1196 to +1200
if(handle_spnego(&authinfo)) {}
else if(handle_ntlm(&authinfo)) {}
else if(handle_digest(&authinfo)) {}
else if(handle_basic(&authinfo)) {}
else if(handle_bearer(&authinfo)) {}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if(handle_spnego(&authinfo)) {}
else if(handle_ntlm(&authinfo)) {}
else if(handle_digest(&authinfo)) {}
else if(handle_basic(&authinfo)) {}
else if(handle_bearer(&authinfo)) {}
if(handle_spnego(&authinfo) ||
handle_ntlm(&authinfo) ||
handle_digest(&authinfo) ||
handle_basic(&authinfo) ||
handle_bearer(&authinfo))
{}

Saves having to use {} so many times.

Copy link
Contributor Author

@Gottox Gottox Nov 5, 2023

Choose a reason for hiding this comment

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

in that case, even the if is not needed:

handle_spnego(&authinfo) || 
       handle_ntlm(&authinfo) ||
       handle_digest(&authinfo) ||
       handle_basic(&authinfo) ||
       handle_bearer(&authinfo);

@Gottox
Copy link
Contributor Author

Gottox commented Nov 5, 2023

@bagder Generally speaking would you support factoring out all the authentication specific code into its own interface?

For start, I'm thinking of a simple interface like this: (pseudocode)

struct authimpl {
  const char *name;
  bool (*handle)(struct authdata *);
};

// ...

static bool handle_auth_basic(struct authdata *) {
  // ...
}

static struct authimpl authimpls[] {
  { "Basic", 5, handle_auth_basic },
  { "Bearer", 5, handle_auth_bearer },
  // ...
}

// ...

struct authdata ad = { ... };
for(int i = 0; i < LENGTH(authimpls); i++) {
  if (checkauth(authimpls[i].name, auth)) {
    authimpls[i].handle(&ad);
    break;
  }
}

In the second iteration I'd like to move the auth methods to their own files and let them build conditionally. This would reduce #ifdef clutter even more at the cost compile time inlining.

free(authinfo->data->req.newurl);
authinfo->data->req.newurl = strdup(authinfo->data->state.url);
if(!authinfo->data->req.newurl)
return CURLE_OUT_OF_MEMORY;
Copy link
Member

Choose a reason for hiding this comment

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

This returns an error code, not a boolean! You probably need to propagate the error code.

@bagder
Copy link
Member

bagder commented Nov 5, 2023

@bagder Generally speaking would you support factoring out all the authentication specific code into its own interface?

I would. I think that could clean things up nicely!

@bagder
Copy link
Member

bagder commented Dec 2, 2023

Stalled

@bagder bagder closed this Dec 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants