FirebirdSQL / firebird

Firebird server, client and tools
https://firebirdsql.org
1.26k stars 217 forks source link

Incorrect handling of non-ASCII object names in CREATE MAPPING statement #8253

Closed hvlad closed 2 months ago

hvlad commented 2 months ago

Steps to reproduce:

  1. check current console code page

    firebird>chcp
    Active code page: 866
  2. connect with charset corresponding to the console code page

    firebird>isql employee -ch dos866
    Database: employee, User: SYSDBA
  3. create mapping using non-ASCII group name

    SQL> create mapping m1 using any plugin from group группа to role role1;
    Statement failed, SQLSTATE = 42000
    Dynamic SQL Error
    -SQL error code = -104
    -Token unknown - line 1, column 47
    -г

    this is expected as literal группа is not quoted

  4. check with single and double quotes, local and global mappings

    SQL> create mapping m1 using any plugin from group 'группа' to role role1;
    SQL> create mapping m2 using any plugin from group "группа" to role role1;
    SQL> create global mapping m3 using any plugin from group 'группа' to role role1;
    SQL> create global mapping m4 using any plugin from group "группа" to role role1;

    no errors, as expected

  5. check created mappings

    
    SQL> show mapping;
    M1 USING ANY PLUGIN FROM GROUP группа TO ROLE ROLE1
    M2 USING ANY PLUGIN FROM GROUP ╨│╤А╤Г╨┐╨┐╨░ TO ROLE ROLE1

Global mapping M3 USING ANY PLUGIN FROM GROUP ???????????? TO ROLE ROLE1 M4 USING ANY PLUGIN FROM GROUP ?????????????????????????????????? TO ROLE ROLE1


I see two issues here:
- when using double quotes, literal is not encoded correctly by parser before put into database
- when creating global mapping, strings is not not encoded correctly when passed into security.db connection

Also, it is not clear why double quotes is allowed at all. I don't think that name of the mapped object (for ex. OS user or group name) should follow rules of SQL-identifier.
hvlad commented 2 months ago

BTW, SHOW MAPPING command should use single quotes around 'from object name', i.e.

M1 USING ANY PLUGIN FROM GROUP группа TO ROLE ROLE1  -- current, wrong
M1 USING ANY PLUGIN FROM GROUP 'группа' TO ROLE ROLE1  -- quoted, correct
mrotteveel commented 2 months ago

I would consider it an identifier, and as such it should follow the rules for identifiers (regular identifier unquoted, otherwise quoted with double quotes). Being able to either be unquoted, or quoted as a string literal feels very odd to me.

hvlad commented 2 months ago

Why Windows (Linux) group name should be SQL identifier ? What if one day some auth plugin will work with FQDN's (full qualified domain names) ?

aafemt commented 2 months ago

I would consider it an identifier, and as such it should follow the rules for identifiers

But group name is an external identifier which is not bound by SQL rules. IIRC Windows group names are case-insensitive but can contain spaces. Because of this it has sense to require them to be a string expression. Interpretation of the value of this expression is up to the auth plugin.

mrotteveel commented 2 months ago

Why Windows (Linux) group name should be SQL identifier ? What if one day some auth plugin will work with FQDN's (full qualified domain names) ?

That would still fall in the rules of quoted identifiers. And anyway, otherwise that would be an argument that it should always be a string literal, and never be allowed unquoted.

asfernandes commented 2 months ago

And anyway, otherwise that would be an argument that it should always be a string literal, and never be allowed unquoted.

Agreed. Like EXTERNAL NAME utf_string, MODULE_NAME utf_string, ENTRY_POINT utf_string.

hvlad commented 2 months ago

Why Windows (Linux) group name should be SQL identifier ? What if one day some auth plugin will work with FQDN's (full qualified domain names) ?

That would still fall in the rules of quoted identifiers.

Rules for SQL identifiers is not about quoting only. Its also limits length allowed.

And anyway, otherwise that would be an argument that it should always be a string literal, and never be allowed unquoted.

This is my point too.

AlexPeshkoff commented 2 months ago

I resend email cause original delivery somewhy failed... May be due to attachment in it?

On 9/16/24 14:55, Vlad Khorsun wrote:

Why Windows (Linux) group name should be SQL identifier ? What if one day some auth plugin will work with FQDN's (full qualified domain names) ?

That would still fall in the rules of quoted identifiers.

Rules for SQL identifiers is not about quoting only. Its also limits length allowed.

And anyway, otherwise that would be an argument that it should always be a string literal, and never be allowed unquoted.

This is my point too.

Some logic in forcing from_name in mapping to be a string literal is present. But when enabling use of any form (unquoted, single or double quotes) to specify it I've followed very simple goal - make user's life in typical case as simple as possible. In /etc/group on my box (removed attachment due to failed delivery) - only one group does not follow SQL rules. I.e. I agree that such approach is not typical for SQL but it works in 99% of cases. (Certainly charset should be fixed, it's plain bug.) What about unquoted from_name - I see absolutely no use breaking existing scripts. And changing a lot of examples in our own documentation. Like this:

   CREATE MAPPING DEF_SYSDBA USING PLUGIN SRP IN "security.db" FROM USER SYSDBA TO USER;

Can someone explain why typing 'SYSDBA' here is better than SYSDBA?

mrotteveel commented 2 months ago

Can someone explain why typing 'SYSDBA' here is better than SYSDBA?

In that case it isn't, because there it is a SQL identifier.

AlexPeshkoff commented 2 months ago

On 9/16/24 20:33, Mark Rotteveel wrote:

Can someone explain why typing 'SYSDBA' here is better than SYSDBA?

In that case it isn't, because there it is a SQL identifier.

Mark, the only problem here is that user's login name (like sysdba) and group name (OS object) appears in exactly same place in SQL statement - what means name of object from which mapping is done depends upon context of mapping, it may be SQL identifier, may be some OS object name, not necessary group. Therefore I've enabled entering it in different notations.