Merge setup script changes from v7.12.0.7 into main - #8388
Open
melton-jason wants to merge 4 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe setup script derives separate MariaDB hosts for migrator and app accounts. Attachment bulk-import files receive formatting-only updates with no behavior or API changes. ChangesMariaDB account host targeting
Attachment bulk-import formatting
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Warning One or more dependencies are approaching or past End-of-Life. |
g1rly-c0d3r
approved these changes
Jul 31, 2026
g1rly-c0d3r
left a comment
g1rly-c0d3r
left a comment
Collaborator
There was a problem hiding this comment.
Testing instructions
- Ensure the MIGRATOR_NAME and APP_USER_NAME environment variables are different from each other and the MASTER_NAME environment variable
- Ensure that there are no pre-existing users in the database which have the names of the MIGRATOR_NAME and APP_USER_NAME (with any host)
- Start Specify7 and ensure the database setup script completes successfully (or at least gets to checking for and/or running migrations)
- Ensure that the created users have a Host which matches that of the MASTER_NAME user
- Set the MIGRATOR_NAME and MIGRATOR_PASSWORD environment variables to the same as the MASTER_NAME and MASTER_PASSWORD environment variables
- Ensure that there are no pre-existing users in the database which have the names of the MASTER_NAME and do not have a generic host from the first step (
%) - Start Specify7 and ensure the database setup script completes successfully
- Ensure that a new user was not created in MariaDB for the MIGRATOR_NAME
- With the MIGRATOR variables still matching the MASTER variables, set the MIGRATOR_USER_HOST environment variable to a valid, but more specific value
- This can be the host returned by the USER() function executed earlier in these testing instructions
- Start Specify7 and ensure the database setup script completes successfully
- Ensure that a new user was not created in MariaDB for the MIGRATOR_NAME
- Set the APP_USER_NAME and APP_USER_PASSWORD to the same as the MASTER_NAME and MASTER_PASSWORD environment variables
- Ensure that there are no pre-existing users in the database which have the names of the MASTER_NAME and do not have a generic host from the first step (
%) - Start Specify7 and ensure the database setup script completes successfully
- Ensure that a new user was not created in MariaDB for the APP_USER_NAME
- With the APP_USER variables still matching the MASTER variables, set the APP_USER_HOST environment variable to any value that is not the generic host from the first step
- Start Specify7 and ensure the database setup script completes successfully
- Ensure that a new user was not created in MariaDB for the APP_USER_NAME
- Set the APP_USER_NAME and APP_USER_PASSWORD to the same as the MIGRATOR_NAME and MIGRATOR_PASSWORD environment variables
- Ensure that there are no pre-existing users in the database which have the names of the MIGRATOR_NAME and do not have a generic host from the first step (
%) - Start Specify7 and ensure the database setup script completes successfully
- Ensure that a new user was not created in MariaDB for the APP_USER_NAME
- With the APP_USER variables still matching the MIGRATOR variables, set the APP_USER_HOST environment variable to something different from the MIGRATOR_USER_HOST
- Start Specify7 and ensure the database setup script completes successfully
- Ensure that a new user was not created in MariaDB for the APP_USER_NAME
- Set the MIGRATOR_NAME to be different from the MASTER_NAME
- Set the MIGRATOR_USER_HOST environment variable to a valid, but more specific value than the host of the MASTER user
- This can be the host returned by the USER() function executed earlier in these testing instructions
- Start Specify7 and ensure the database setup script completes successfully
- Ensure that a new user was created with the MIGRATOR_NAME and MIGRATOR_USER_HOST
- Set the APP_USER_NAME to be different from the MASTER_NAME and MIGRATOR_NAME
- Set the APP_USER_HOST environment variable to a valid, but more specific value than the host of the MASTER user
- This can be the host returned by the USER() function executed earlier in these testing instructions
- Start Specify7 and ensure the database setup script completes successfully
- Ensure that a new user was created with the APP_USER_NAME and APP_USER_HOST
Looks good, all users were created correctly when I specified no host, and when I did, they were not duplicated. When I initially specified a host, they were created with that name.
I do think that it could be confusing that the MIGRATOR_USER_HOST has a different naming scheme than both MIGRATOR_NAME and MIGRATOR_PASSWORD, but @melton-jason explained that this was more accurate. I think the best solution is to potentially re-name MIGRATOR_NAME and MIGRATOR_PASSWORD, but I understand that that would cause any self-hosted user to have to edit these variables, potentially causing confusion.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sp7_db_setup_check.sh`:
- Around line 175-186: After resolving MIGRATOR_USER_HOST and APP_USER_HOST,
create separate SQL-escaped host variables with sql_string_literal(), preserving
the raw variables for diagnostics. Update every MariaDB SELECT, CREATE USER,
GRANT, and SHOW GRANTS statement, including the referenced later statement
blocks, to interpolate only the escaped variables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e719fa1-b1a9-4152-b4e3-11d1013daa05
📒 Files selected for processing (1)
sp7_db_setup_check.sh
Comment on lines
+175
to
+186
| if [[ "$SAME_MASTER_AND_MIGRATOR" == true ]]; then | ||
| MIGRATOR_USER_HOST="${CLIENT_HOST:-}" | ||
| else | ||
| MIGRATOR_USER_HOST="${MIGRATOR_USER_HOST:-${CLIENT_HOST:-}}" | ||
| fi | ||
|
|
||
| if [[ "$SAME_MASTER_AND_APP" == true ]]; then | ||
| APP_USER_HOST="${CLIENT_HOST:-}" | ||
| elif [[ "$SAME_MIGRATOR_AND_APP" == true ]]; then | ||
| APP_USER_HOST="${MIGRATOR_USER_HOST:-}" | ||
| else | ||
| echo "Warning: Could not detect client host via USER(); will only create users for explicit *_HOST values." | ||
| APP_USER_HOST="${APP_USER_HOST:-${CLIENT_HOST:-}}" |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Escape account hosts before use in SQL.
MIGRATOR_USER_HOST and APP_USER_HOST can now come from environment variables. The script interpolates them into SQL literals without sql_string_literal(). An override containing a single quote can inject SQL that runs with MASTER_USER_NAME privileges.
Keep the raw values for diagnostics. Create escaped SQL-specific variables after host resolution. Use those variables in every SELECT, CREATE USER, GRANT, and SHOW GRANTS statement.
Proposed fix
if [[ "$SAME_MASTER_AND_APP" == true ]]; then
APP_USER_HOST="${CLIENT_HOST:-}"
elif [[ "$SAME_MIGRATOR_AND_APP" == true ]]; then
APP_USER_HOST="${MIGRATOR_USER_HOST:-}"
else
APP_USER_HOST="${APP_USER_HOST:-${CLIENT_HOST:-}}"
fi
+
+SQL_MIGRATOR_USER_HOST="$(sql_string_literal "$MIGRATOR_USER_HOST")"
+SQL_APP_USER_HOST="$(sql_string_literal "$APP_USER_HOST")"
- "SELECT COUNT(*) FROM mysql.user WHERE user = '$SQL_MIGRATOR_NAME' AND host = '$MIGRATOR_USER_HOST';")
+ "SELECT COUNT(*) FROM mysql.user WHERE user = '$SQL_MIGRATOR_NAME' AND host = '$SQL_MIGRATOR_USER_HOST';")Apply the same replacement to the other MariaDB statements.
Also applies to: 214-220, 236-249, 277-287, 317-327
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sp7_db_setup_check.sh` around lines 175 - 186, After resolving
MIGRATOR_USER_HOST and APP_USER_HOST, create separate SQL-escaped host variables
with sql_string_literal(), preserving the raw variables for diagnostics. Update
every MariaDB SELECT, CREATE USER, GRANT, and SHOW GRANTS statement, including
the referenced later statement blocks, to interpolate only the escaped
variables.
github-project-automation
Bot
moved this from 📋Back Log
to Dev Attention Needed
in General Tester Board
Triggered by edd7b09 on branch refs/heads/setup_script_hotfix
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes an Issue with the Setup Script where:
Specifically, the setup script would always try and use the IP address of the client as seen from the server when dealing with authentication connection.
For example, if there was a
master@%user on the database and the IP address of the client from the perspective of the database was an IP address like192.168.0.2, then Specify would check for and create amaster@'192.168.0.2'MIGRATOR/APP user if the name portion of the connection was the same as the MASTER user.This led to a problem where:
a. For example, consider the above case where the user is defined as
master@'%'a. In the example case, the query would indicate the user does not exist as there's not a user for
master@'192.168.0.2'a. e.g.,
master@'192.168.0.2'a. This includes the queries that give the new user permissions, which the new user will always not be able to do
This PR does two things:
By default, inherits the host of the MASTER_USER when creating/connecting with the MIGRATOR or APP user
a. This results in Specify only working with the most specific host name combination that already exists on the master user
b. e.g., if the master user connection is defined as master@%, the migrator and app user will inherit the host of %
c. If there are two connections for master: master@% and master@ip , then Specify will use ip when the connection is authenticated with that connection (i.e., when connecting to the DB from that IP)
d. This should work out-of-the-box and be sufficient for nearly all connections. This directly resolves the instigating Issue where Specify was creating a user with a more-specific IP
Allows the MIGRATOR and APP user hosts from the MariaDB side to be manually overwritten through the MIGRATOR_USER_HOST and APP_USER_HOST environment variables
a. The use case for these is for more fine-grained hosts for the created users. I would say these are for more advanced setups and most people will not need these options
b. These variables do not have a default value and only have meaning when explicitly set (they will "default" to inheriting the host of the master user)
c. If the user itself would be inherited (e.g., in the case where the migrator user is the same as the master user, or the app user is the same as the master user or migrator user) then these variables are ignored/inherited in order of precedence
d. This has some pros/cons. It leads to a little less flexibility from a user perspective, but does prevent the leading issue from happening. If someone is familiar with database administration, then it is better to not have this feature: otherwise I think it's safest to have a safety guard at the cost of flexibility
e. The commit for this is 9ebe0c9
Checklist
self-explanatory (or properly documented)
specify7/specifyweb/specify/management/commands/run_key_migration_functions.py
Line 50 in ea04665
Testing instructions
These testing instructions will only be performable with a local Specify instance where you have access to the database
%You can run the following SQL in the MariaDB database to view all users and their respective hosts:
You can use CREATE USER to create new MariaDB users.
e.g.:
You can use DROP USER to delete MariaDB users:
ALL PRIVILEGES ON *.*and make sure they haveGRANT OPTIONView GRANT for a refresher on how to give users different permissions:
localhostor a private IP like (172.18.x.x), while the latter should be the connection set up and/or verified in the previous stepYou can run the following command from the "root" directory of Specify 7 to get the IP address of the Specify7 container as seen by MariaDB (USER()), as well as the full name of the user used to authenticate the connection (CURRENT_USER()).
%)%)%)Summary by CodeRabbit
Bug Fixes
Maintenance