Skip to content

Manual extension check to prevent permission issues #3496

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

Open
dkottis opened this issue Mar 12, 2025 · 5 comments
Open

Manual extension check to prevent permission issues #3496

dkottis opened this issue Mar 12, 2025 · 5 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@dkottis
Copy link

dkottis commented Mar 12, 2025

Currently, extensions are always checked or installed using CREATE EXTENSION IF NOT EXISTS. However, due to a recent change by Microsoft with PostgreSQL Flexible Servers, only the superuser has permission to create an extension. See here.

In our scenario, the application automatically migrates its database. This process triggers the CREATE EXTENSION IF NOT EXISTS command, which, even if the extension is already installed, fails due to insufficient rights.

Would it be possible to perform a manual check to determine if an extension is already installed at this location:

//...
        builder
            .Append("CREATE EXTENSION IF NOT EXISTS ")
            .Append(DelimitIdentifier(extension.Name));
//...

similar to how the schema is checked here:

//...
        // PostgreSQL has CREATE SCHEMA IF NOT EXISTS, but that requires CREATE privileges on the database even if the schema already
        // exists. This blocks multi-tenant scenarios where the user has no database privileges.
        // So we procedurally check if the schema exists instead, and create it if not.
        var schemaName = operation.Name.Replace("'", "''");

        // If we're generating an idempotent migration, we're already in a PL/PGSQL DO block; otherwise we need to start one.
        if (!Options.HasFlag(MigrationsSqlGenerationOptions.Idempotent))
        {
            builder
                .AppendLine(@"DO $EF$")
                .AppendLine("BEGIN");
        }
//...

This would also benefit other scenarios where the application might not have sufficient rights to create extensions.

@roji
Copy link
Member

roji commented Mar 12, 2025

@Astasian it might be possible, but then what's the point of having that CREATE EXTENSION at all, given that it would either be skipped (because the extension is already there) or fail (because the extension isn't there and there's no permissions to add it)? In other words, if you're not managing extensions via your migrations (because of this permission issue), wouldn't it be better to just remove the create extension operations from your migrations altogether?

@dkottis
Copy link
Author

dkottis commented Mar 13, 2025

@roji You are correct if the problem is the migration itself; however, the issue arises from the initialization code for the EF Core tables and the related extensions.

If you call MigrateAsync even without a pending migration, we end up here

//...
    public override async Task MigrateAsync(string? targetMigration, CancellationToken cancellationToken = default)
    {
        var appliedMigrations = await _historyRepository.GetAppliedMigrationsAsync(cancellationToken).ConfigureAwait(false);

  ->      await base.MigrateAsync(targetMigration, cancellationToken).ConfigureAwait(false);
//..

which will call the internal Migrator from EF here

                    try
                    {
      ->                  return await migrator._historyRepository.CreateIfNotExistsAsync(ct).ConfigureAwait(false);
                    }

which will call the IHistoryRepository here

   async Task<bool> IHistoryRepository.CreateIfNotExistsAsync(CancellationToken cancellationToken)
    {
        // In PG, doing CREATE TABLE IF NOT EXISTS isn't concurrency-safe, and can result a "duplicate table" error or in a unique
        // constraint violation (duplicate key value violates unique constraint "pg_type_typname_nsp_index").
        // We catch this and report that the table wasn't created.
        try
        {
            return (await Dependencies.MigrationCommandExecutor.ExecuteNonQueryAsync(
                    GetCreateIfNotExistsCommands(), Dependencies.Connection, new MigrationExecutionState(), commitTransaction: true,
                    cancellationToken: cancellationToken).ConfigureAwait(false))
                != 0;
        }
        catch (PostgresException e) when (e.SqlState is PostgresErrorCodes.UniqueViolation
                                              or PostgresErrorCodes.DuplicateTable
                                              or PostgresErrorCodes.DuplicateObject)
        {
            return false;
        }
    }

    private IReadOnlyList<MigrationCommand> GetCreateIfNotExistsCommands()
        => Dependencies.MigrationsSqlGenerator.Generate([new SqlOperation
        {
            Sql = GetCreateIfNotExistsScript()
        }]);

and GetCreateIfNotExistsCommands to build the necessary commands to build the migration history table. But the resulting command will also include a statement to create the extension(s), even though we have no intend of creating it (because this is not a migration). So the query will always look like this (with postgis in this case)

CREATE EXTENSION IF NOT EXISTS postgis;
CREATE TABLE IF NOT EXISTS __ef_migrations_history (
  migration_id character varying(150) NOT NULL,
  product_version character varying(32) NOT NULL,
  CONSTRAINT pk__ef_migrations_history PRIMARY KEY (migration_id)
);

As far as I could the debug the reason for this is a AlterDatabaseOperation, but I don't know why.

@roji
Copy link
Member

roji commented Mar 13, 2025

Thanks, that makes sense... I hope to take a general look at the initial history repository creation - there are various other issues with it (#3407), I'll take a look at all of these together to get a holistic view.

In the meantime, the issue you're describing only at the initial creation only (one-time thing), so I suggest simply working around that by e.g. generating a SQL script and manually removing the CREATE EXTENSION - it's not great, just a workaround for now. And for the other cases (actual migrations), I think the reasonable thing is to just edit the migration and remove the extension creation by hand - that's also a one-time-per-extension thing, so shouldn't be too problematic.

@roji roji added the bug Something isn't working label Mar 13, 2025
@roji roji self-assigned this Mar 13, 2025
@roji roji added this to the 10.0.0 milestone Mar 13, 2025
@dkottis
Copy link
Author

dkottis commented Mar 13, 2025

Thank you very much for your help.

In my case this code was always run when calling MigrateAsync(); on the DatebaseContext, even without any pending migrations and with a fully initialized __ef_migrations_history table.

@roji
Copy link
Member

roji commented Mar 13, 2025

Oh, then that does sound a bit more serious... I'm unfortunately very much swamped at work at the moment, it may be a while before I can take a look...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants