Added set commands for base64url, expiry and ip matching #12

Closed
wants to merge 8 commits into
from

Projects

None yet

2 participants

@mevdschee

I added a command to do base64url encoding and decoding. I also added an expired and ip_matches command that will compare with the current timestamp and ip address. These are building blocks for an alternative secure link module.

@agentzh
OpenResty member

@mevdschee Thank you for your contribution!

Several comments though:

  1. I think we should still make this module compile with earlier versions of NGINX like 1.5.9. I suggest adding macro tests against nginx_version to enable the new functionality conditionally.
  2. Please add guard macros to your header filters for consistency.
  3. Please remove redundant blank lines in some places (like the one before local variable declarations and the one at ngx_http_set_misc_module.c:90).

Will you fix these?

@mevdschee

I'll try to adjust the code to fix these issues. Thank you.

@mevdschee

I made the adjustments you requested in fd8b988. Does it look good to you?

@agentzh
OpenResty member

@mevdschee Thank you for the quick fixes! Some more suggestions:

  1. Please put C variable declarations to the beginning of the code block according to the NGINX coding style (and to please some old C compilers).
  2. Please avoid using things like ndk_palloc_re because I'd like to limit our usage here to the set_var NDK submodule only.
@agentzh
OpenResty member

@mevdschee Also, will you mind adding some documentation for the new directives to README? Thanks!

@mevdschee

Do you want me to update src/ngx_http_set_base64.c and src/ngx_http_set_base64.h as well?

@agentzh
OpenResty member

@mevdschee Preferably in a separate branch and separate pull request :) Thanks!

@mevdschee

How do I update the documentation? What did you make those readme's with? I see a reference to "wiki2markdown.pl" in a comment. What is that and where can I get it? Is there an online editor for that ".wiki" format? Does it adhere to any standard?

@agentzh
OpenResty member

@mevdschee You're recommended to include your changes in the .wiki file in your patch. You can use the wiki.nginx.org site to test your changes.

@mevdschee

Okay.. I updated the code in b51046b. I hope it is good now.

@mevdschee

Also, I have found "wiki2markdown.pl" in https://github.com/agentzh/nginx-devel-utils

@mevdschee

Okay.. Now how do you do update the plain text README? The wiki and markdown are updated in 29b18d2.

@mevdschee

I have found wiki2pod.pl and the pod2text Linux tool. The README is updated in 02e12a3.

@agentzh
OpenResty member

@mevdschee Thanks for the updates! I'll try to merge your patch soon :)

Maurits van ... added some commits Mar 13, 2014
@mevdschee

NB: I have added IPv6 test cases for ip_matches.

@agentzh
OpenResty member

@mevdschee Hmm, seems that you've already put 3 unrelated features in this single pull request. Alas.

Will you split these unrelated features into 3 different branches and create 3 different pull requests separately? Mixing things together makes the review and merge process on my side much harder. I prefer small atomic changes in every pull request. Thank you for your cooperation!

@mevdschee mevdschee closed this Mar 13, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment