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

feat: Associate role with appkeys #1198

Merged
merged 1 commit into from
Jun 8, 2023
Merged

feat: Associate role with appkeys #1198

merged 1 commit into from
Jun 8, 2023

Conversation

JigarJoshi
Copy link
Contributor

@JigarJoshi JigarJoshi commented May 16, 2023

Describe your changes

  • Associate role with app keys.
  • Do not allow user to create appkey/user invitation with role higher than their own.

Note: This should not be merged prior to updating interfaces (cloud console, CLI) that accepts the role for app key creation.

How best to test these changes

Added test.

Issue ticket number and link

@reviewpad reviewpad bot added the large label May 16, 2023
@reviewpad reviewpad bot added the medium label May 16, 2023
@JigarJoshi JigarJoshi force-pushed the jmj/add_role_to_apps branch 2 times, most recently from 9512f3e to ee9fd3d Compare May 16, 2023 19:43
@JigarJoshi JigarJoshi force-pushed the jmj/add_role_to_apps branch 2 times, most recently from b834c8c to 8118b35 Compare May 24, 2023 21:41
@JigarJoshi JigarJoshi marked this pull request as ready for review May 24, 2023 22:14
efirs
efirs previously approved these changes May 25, 2023
@@ -101,6 +101,13 @@ type AuthConfig struct {
EnableErrorLog bool `mapstructure:"enable_error_log" yaml:"enable_error_log" json:"enable_error_log"`
}

type ApiKeysConfig struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not related to this change but manual merge has moved it around.

// role names.
readOnlyRoleName = "ro"
editorRoleName = "e"
ownerRoleName = "o"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved it to auth package to avoid circular dependencies.

@@ -450,6 +447,19 @@ func authorize(ctx context.Context) (err error) {
return nil
}

/*
func isAuthorizedProject(reqMetadata *request.Metadata, accessToken *types.AccessToken) bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

keeping it here to enable back the project mismatch authz.

@@ -115,8 +122,15 @@ type GetUserResp struct {
AppMetaData *UserAppData `json:"app_metadata" db:"app_metadata"`
}

// returns currentSub, creationTime, error.
func _createAppKey(ctx context.Context, clientId string, clientSecret string, g *gotrue, keyName string, keyDescription string, project string, keyType string) (string, int64, error) {
type keyInfo struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

internal type to avoid long number of args.

Comment on lines +181 to +187
func _getEmail(clientId string, appKeyType string, g *gotrue) string {
suffix := g.AuthConfig.Gotrue.UsernameSuffix
if appKeyType == AppKeyTypeApiKey {
suffix = g.AuthConfig.ApiKeys.EmailSuffix
}
return fmt.Sprintf("%s%s", clientId, suffix)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

manual merge moved this method around.

@@ -193,48 +193,127 @@ func TestGoTrueAuthProvider(t *testing.T) {
Boolean()
require.True(t, deletedResponse.Raw())
}
func TestApiKeyCrud(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

manual merge moved it around.

@JigarJoshi JigarJoshi force-pushed the jmj/add_role_to_apps branch 3 times, most recently from bae077c to f1f3f4f Compare June 7, 2023 23:53
feat: Associate role with appkeys

fix: Do not allow higher role app key created

fix: Fixed test to follow project level authz

fix: rebase

fix: rebase

fix: rebase

fix: Fix the merge

fix: User cannot invite user with higher role
@JigarJoshi JigarJoshi merged commit b1a26b1 into main Jun 8, 2023
3 checks passed
@JigarJoshi JigarJoshi deleted the jmj/add_role_to_apps branch June 8, 2023 03:46
@tigrisdata-argocd-bot
Copy link
Collaborator

🎉 This PR is included in version 1.0.0-beta.117 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants