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

Changes to how user mapping is handled #729

Merged
merged 5 commits into from
Dec 17, 2020
Merged

Changes to how user mapping is handled #729

merged 5 commits into from
Dec 17, 2020

Conversation

treydock
Copy link
Contributor

@treydock treydock commented Nov 5, 2020

Moves regex user mapping to Lua.

Keeps OOD_USER_MAP_CMD but makes the default OOD_USER_MAP_MATCH to do regex in pure Lua.

New ood-portal-generator options

  • user_mapping - defaults to match but can also be command
  • user_map_match - defaults to .*

Makes it so ood_auth_map is run using ondemand SCL to ensure the we can remove the need to make Apache aware of SCL Ruby and not rely on system Ruby to launch the mapping script.

Still need unit tests for Lua and maybe a unit test to validate user_mapping when the value is command.

Fixes #728

@ericfranz
Copy link
Contributor

After reviewing the full set of changes, it seems like user_mapping may be redundant.

  1. The presence of user_map_match or user_map_cmd in the ood-portal-generator conf can indicate which the user wants to use
  2. If both are present in the config, could just user_map_cmd.
  3. If none are present, default to user_map_match as specified.
-@user_mapping     = opts.fetch(:user_mapping, 'match')
-@user_map_match   = opts.fetch(:user_map_match, ".*")
-@user_map_cmd     = opts.fetch(:user_map_cmd, nil)
+@user_map_cmd     = opts.fetch(:user_map_cmd, nil)
+@user_map_match = @user_map_cmd ? nil : opts.fetch(:user_map_match, ".*")

And then instead of

  <%- if @user_mapping =~ %r{match|regex} -%>
  SetEnv OOD_USER_MAP_MATCH "<%= @user_map_match %>"
  <%- end -%>
  <%- if @user_mapping =~ %r{command|cmd} && @user_map_cmd -%>
  SetEnv OOD_USER_MAP_CMD "<%= @user_map_cmd %>"
  <%- end -%>

we do

  <%- if @user_map_cmd -%>
  SetEnv OOD_USER_MAP_CMD "<%= @user_map_cmd %>"
  <%- elsif @user_map_match -%>
  SetEnv OOD_USER_MAP_MATCH "<%= @user_map_match %>"
  <%- end -%>

@ericfranz
Copy link
Contributor

The benefit of getting rid of the new user_mapping setting is that existing ood portal configs will be backwards compatible since they all set user_map_cmd right now - though we can of course recommend sites to switch to user_map_match

@treydock
Copy link
Contributor Author

My only objection is this will be for OnDemand 2.0 so we are already at a breaking change release and we should move people to the more efficient method of mapping regexes. I don't think we should make a more efficient method and still support ood_auth_map regexes, offering people 2 ways to do the exact same thing when one is far better than another does not make sense to me. We should completely remove ood_auth_map.regex and just stick with one solution for doing regex mapping.

@ericfranz
Copy link
Contributor

We can drop the old Ruby ood_auth_map.regex and wait and see if we get any feedback that the new approach doesn't work for some edge case. If that happens, we can provide directions to use the old mapping in that Discourse thread or even the documentation if it comes to that.

Regardless, the suggestion above provides a better admin experience for configuration:

-@user_mapping     = opts.fetch(:user_mapping, 'match')
-@user_map_match   = opts.fetch(:user_map_match, ".*")
-@user_map_cmd     = opts.fetch(:user_map_cmd, nil)
+@user_map_cmd     = opts.fetch(:user_map_cmd, nil)
+@user_map_match = @user_map_cmd ? nil : opts.fetch(:user_map_match, ".*")

Since you only have to do user_map_match: or user_map_cmd: and you don't have to provide the redundant user_mapping: match. Because if you are specifying a user_map_match: we know you want to use the match strategy and if you are specifying a user_map_cmd: we know you want to use the cmd strategy.

@treydock
Copy link
Contributor Author

treydock commented Dec 8, 2020

Made those necessary adjustments.

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.

Drop ood_auth_map in favor of lua code
3 participants