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

SQL-User-Name should be removed after rlm_sql finishes processing a request #640

Closed
philmayers opened this issue May 15, 2014 · 7 comments
Closed
Labels
usability category: relates to how easy (or difficult) a feature is to use v3.x.x meta: relates to the v3.0.x branch

Comments

@philmayers
Copy link

If you do this:

authorize {
  update request {
    Tmp-String-0 := "%{sql:select '%{SQL-User-Name}'}"
  }
}

...it doesn't work because the %{SQL-User-Name} seems to have been expanded by the xlat machinery before calling sql_xlat. This code:

https://github.com/FreeRADIUS/freeradius-server/blob/master/src/modules/rlm_sql/rlm_sql.c#L152

....is therefore not doing what it should be.

This works:

authorize {
  sql
  update request {
    Tmp-String-0 := "%{sql:select '%{SQL-User-Name}'}"
  }
}

...because the SQL-User-Name is left lying around on the request from the sql.authorize call for the xlat to find.

@arr2036
Copy link
Member

arr2036 commented May 15, 2014

There's no good fix for this. The best we can do is guarantee consistent behaviour by removing Sql-User-Name from the request before returning from any of rlm_sql's mod_* functions.

The reason why SQL-User-Name is needed, is to allow for querying with alternative usernames to allow the 'profile' functionality to work. There's no reason to use it in an xlat string, User-Name will still be expanded and escaped correctly.

@philmayers
Copy link
Author

I don't mind if it's left lying around (though IIRC the old code did indeed remove it before returning). What I noticed was it no longer working in xlats, unless you'd already called one which is weirdly inconsistent.

If it shouldn't be used in an XLAT then a) that's a change in behaviour and b) it should definitely be removed before returning to avoid people doing that and seeing the inconsistent behaviour above.

FWIW the reason we use it is to avoid cluttering up the SQL xlat queries with %{%{%{%{%{%{%{%{% into infinity when doing complex fallback of various values. It's also faster to do this once before entering the query. Obviously I can (and now will) just define a local attribute and set it with a policy at the top of all of our virtual servers. Yay DRY oh wait no.

@arr2036
Copy link
Member

arr2036 commented May 15, 2014

Or if you really want to confuse people:

# outside server
sql_username = "%{%{Stripped-User-Name}:-%{User-Name}}"

"%{sql:SELECT * FROM BLAH WHERE user='${sql_username}'}"

@arr2036
Copy link
Member

arr2036 commented May 15, 2014

Expansion of ${} has gotten better to the point that they're almost like macros when used in value string and conditions.

@philmayers
Copy link
Author

Hey neat. That works for me.

@arr2036 arr2036 changed the title SQL-User-Name doesn't work in an SQL xlat SQL-User-Name should be removed after rlm_sql finishes processing a request May 16, 2014
@alandekok
Copy link
Member

Can we close this?

also, if SQL-User-Name is confusing, we should look at replacing it with something better.

@arr2036
Copy link
Member

arr2036 commented May 16, 2014

no, we need to remove SQL-User-name before returning from module functions

alandekok added a commit that referenced this issue Jun 13, 2014
It's really just a macro around pairdelete.  But it's clearer
to have sql_set_user() / sql_unset_user, than to use pairdelete
alandekok added a commit that referenced this issue Jun 13, 2014
It's really just a macro around pairdelete.  But it's clearer
to have sql_set_user() / sql_unset_user, than to use pairdelete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
usability category: relates to how easy (or difficult) a feature is to use v3.x.x meta: relates to the v3.0.x branch
Projects
None yet
Development

No branches or pull requests

3 participants