-
-
Notifications
You must be signed in to change notification settings - Fork 359
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
Implement roles.add
RPC endpoint
#3769
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some issues that keep this from working. See line comments for details.
Additionally, please write a couple of SQL tests to make sure the functionality is working (these would have caught one of the issues), and a couple of mocked call-of-the-RPC-function tests for the roles.add
and roles.list_
methods (these would have caught the other breaking problem).
db/sql/00_msar.sql
Outdated
CASE WHEN login_ THEN | ||
PERFORM create_basic_mathesar_user(rolename, password_); | ||
ELSE | ||
PERFORM __msar.exec_ddl('CREATE ROLE %I', rolename); | ||
PERFORM __msar.exec_ddl( | ||
'GRANT CREATE, TEMP ON DATABASE %I TO %I', | ||
current_database()::text, | ||
rolename |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user/role should not have the CREATE
privilege on the database while creating the role, which both create_basic_mathesar_user
and the code here grants.
A tangential question:
I was under the impression that we would allow mathesar_schemas
to be used by PUBLIC
which will not require us to have to explicitly grant access to each role. Is that not the case? What about roles created outside of Mathesar and configured here? Do those roles not have access to our schemas? (Also a question to @mathemancer)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be the right thing to do. We'd have to add default privileges to them.
@pavish @mathemancer I found out that creating a new role automatically grants You could also notice this in the sql test in the PR |
@Anish9901 |
Huh. Well, I looked into this in the docs and found this:
@pavish What are the UI implications (if any) of this? I think we should avoid altering those default privileges unless we really need to (even on internal DBs) and we should definitely not do that on non-internal DBs. @Anish9901 The reason your |
The UI would not be affected by this. We currently do not support altering default privileges for objects for RC. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, LGTM.
CASE WHEN login_ THEN | ||
EXECUTE format('CREATE USER %I WITH PASSWORD %L', rolename, password_); | ||
ELSE | ||
EXECUTE format('CREATE ROLE %I', rolename); | ||
END CASE; | ||
RETURN msar.get_role(rolename); | ||
END; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
Fixes #3670
Checklist
Update index.md
).develop
branch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin