OrchardCore icon indicating copy to clipboard operation
OrchardCore copied to clipboard

Validate database prefix before allowing an admin to be added

Open MikeAlhayek opened this issue 2 years ago • 15 comments

  1. Fix #11803
  2. Fix #11863
  3. Fix #11876

MikeAlhayek avatar Jun 07 '22 14:06 MikeAlhayek

Thanks again Mike, seems still you have a Git issue ;)

hishamco avatar Jun 07 '22 15:06 hishamco

I may share simpler way to fix the issue, let me try it tonight

hishamco avatar Jun 07 '22 15:06 hishamco

Thanks again Mike, seems still you have a Git issue ;)

Yes I see that. Please check #11825

I may share simpler way to fix the issue, let me try it tonight

Can't wait to see a simpler way.

Thank you

MikeAlhayek avatar Jun 07 '22 15:06 MikeAlhayek

If the pr #11713 merged, the code in ConnectionValidator should get updated by injecting ITableNameConvention in the constructor and using it instead of manually creating an instance during validation

MikeAlhayek avatar Jun 09 '22 23:06 MikeAlhayek

Please HOLD ON, I'm working on similar PR to make it easier to achieve this goal, also we need more unit tests to make sure nothing broken

If my attempt failed probably I could add some modifiaction into this PR

hishamco avatar Jun 10 '22 07:06 hishamco

FYI I labled this PR as dontmerge because I mentioned at the beginning before this PR started that I have a simpler way to achieve the goal

After exchanging messages and voice notes with @CrestApps I realize there are two issues:

  1. Avoid adding duplicate table prefix in the same database
  2. Checking table existance (or check if Documents table is there) to avoid invali serial number issue

Based on that I created the PR #11846 which is fixes the issue without hitting the database. Backing to this PR I see the connection validations is good for certain scenarios such as checking Documents table, database is LIVE. So, if we add the validation during tenant creation that we be a behavior breaking-change because in the past the user can create a tenant without the database exists.

I suggest to :

  • Open another issue explaining point 2, so this PR can be modified little bit and fine to fix the issue
  • The PR I created can target the old issue because as described above it should avoid adding a duplicate table prefix on the same database.
  • Add connection validation if the provider has connectionString (If we agreed that we can accept the behavior breaking changes)

/cc @sebastienros @jtkech @Skrypt @agriffard yours feedback is very important here

hishamco avatar Jun 11 '22 14:06 hishamco

The build is broken :(

hishamco avatar Jun 11 '22 18:06 hishamco

The build is broken :(

Very strange the build failed to pull the required packages which is why it failed. I faked a commit to force it to build again

MikeAlhayek avatar Jun 12 '22 00:06 MikeAlhayek

I will make a quick review, but can your PR target the issue no. 2 to make it clear what each PR is target

hishamco avatar Jun 12 '22 03:06 hishamco

I will make a quick review, but can your PR target the issue no. 2 to make it clear what each PR is target

@hishamco I created another issue to describe the other scenarios

MikeAlhayek avatar Jun 13 '22 21:06 MikeAlhayek

@CrestApps can this target #11863 and related to the prefix issue, coz invalid serial could be happen if we didn't specify a table prefix

hishamco avatar Jun 13 '22 21:06 hishamco

@sebastienros hope we can accept this PR to resolve the invalid serial issue, but again I see table prefix issue can be easily resolved using settings

hishamco avatar Jun 16 '22 17:06 hishamco

Fix conflicts by merging latest main branch.

Skrypt avatar Jul 09 '22 16:07 Skrypt

@Skrypt done.

MikeAlhayek avatar Jul 09 '22 20:07 MikeAlhayek

This one for next triage

Skrypt avatar Aug 08 '22 20:08 Skrypt