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

Add the mysql.procs_privs table #2099

Merged
merged 14 commits into from
Oct 25, 2023
Merged

Add the mysql.procs_privs table #2099

merged 14 commits into from
Oct 25, 2023

Conversation

macneale4
Copy link
Contributor

In order to support procedure and function permissions, the mysql.procs_priv needs to be supported. This change add support, but gates the ability to create these grants because they are not being used for actual permission checks yet. That will come next.

I have engine tests on another branch due to the gate. I'll add them when the feature is complete. There are also bats tests in flight in the dolt repo which can be seen here:
dolthub/dolt@4d8fe2a...macneale4/privs_tests_wip

@macneale4 macneale4 marked this pull request as ready for review October 24, 2023 20:15
@macneale4 macneale4 requested a review from max-hoffman October 24, 2023 20:15
Copy link
Contributor

@max-hoffman max-hoffman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly LGTM, I think we want enginetests in UserPrivTests that mirror the bats. If you wanted to wait until this is run in prod to before adding enginetests tests that's fine with me

@@ -22,11 +22,17 @@ table PrivilegeSetTable {
privs:[int];
columns:[PrivilegeSetColumn];
}
table PrivilegeSetRoutine {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: space

@@ -97,6 +97,24 @@ func serializeTables(b *flatbuffers.Builder, tables []PrivilegeSetTable) flatbuf
return serializeVectorOffsets(b, serial.PrivilegeSetDatabaseStartTablesVector, offsets)
}

func serializeRoutines(b *flatbuffers.Builder, routines []PrivilegeSetRoutine) flatbuffers.UOffsetT {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: inconsistent spacing in some of these functions, some skip a line, many don't

}
}
} else {
if n.ObjectType != plan.ObjectType_Any {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stylistically, i might have continued after the first if to flatten the nesting and pull this else block out, and then maybe switch on these object types to make the decision tree a bit more straightforward

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

honestly, I hate this code. very hard to follow. I'll massage on the next round.

if user == nil {
return nil, sql.ErrGrantUserDoesNotExist.New()

if routineGrantsEnabled() && (n.ObjectType == plan.ObjectType_Procedure || n.ObjectType == plan.ObjectType_Function) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if procedure and function behave the same do we need the distinction? a helper method on the type of these symbols would also be a way to do something like n.ObjectType.IsProcGrantable()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They behave the same, but the are unique in the namespace. You can grant access to one or the other, but not both, with one command.

Obviously having a function and a procedure with the same seems like a terrible idea, but it is what it is.

rows = append(rows, sql.Row{privStr})
}

// TODO: display column privileges
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we usually default to erroring for in-progress but currently unsupported behavior

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there is no way to create a grant without the magic env var flag, I think this is fine. After the next change this will be supported. Also, this is needed for the tests I have written in bats.

@@ -132,6 +132,30 @@ func generatePrivStrings(db, tbl, user string, privs []sql.PrivilegeType) string
return fmt.Sprintf("GRANT %s ON %s.%s TO %s%s", privStr, db, tbl, user, withGrantOption)
}

// generateRoutinePrivStrings creates a formatted GRANT <PRILEDGE_LIST> on <ROUTINE_TYPE> <ROUTINE> to <user@host> string
func generateRoutinePrivStrings(db, routine, routine_type, user string, privs []sql.PrivilegeType) string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be useful as a Routine method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can refactor in the future if needed - this seems pretty show specific to me.

@macneale4 macneale4 merged commit 6406fc9 into main Oct 25, 2023
@Hydrocharged Hydrocharged deleted the macneale4/procs_priv_table branch February 7, 2024 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants