Added denylist options for the connections

[apamildner]

As described in https://auth0.com/docs/security/denylist-user-attributes there exists an option to add a DenyList on all connections such that some user data fields are never stored in auth0. This PR adds the ability to include this in the request to update connections.

Proposed Changes

• Adds extra field of NonPersistentAttrs *[]string to all Connection structs

Acceptance Test Output

$ go test ./... -v -run TestConnection

Community Note

• Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
• Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

[yvovandoorn]

Hi @apamildner thanks for your continued contributions! Can you please run go generate ./... to generate the missing accessor for the new field and gofmt -w management/. to fix any spacing issues?

Thanks!

[apamildner]

Hi! Thanks for the response. I've run the commands you specified now 👍

[yvovandoorn]

One thing I noticed is that you're using []string instead of string. Since this operates exactly the opposite of SetUserAttribute, why not implement the same way as that?

What benefit is addressed by utilizing []string?

[apamildner]

It needs to be a []stringbecause it accepts an arbitrary lists of attributes that I want to have on the denylist. I don't think it is an opposite of SetUserAttributes as that option supports a limited set of strings such as on_each_login. But please enlighten me if I have misunderstood something here 👍

[yvovandoorn]

I don't think it is an opposite of SetUserAttributes as that option supports a limited set of strings such as on_each_login.

You're right @apamildner. It is more like scopes where it is an arbitrary list that changes per provider (e.g. different set of scopes for Google than for LinkedIn and again for a custom Oauth2 provider. However, it is unlike scopes in the sense that they are stored like (specifically for the OIDC and Oauth2 connection strategies) this on Auth0's side:

"scope": "openid profile email read:users write:users read:group write:group",

Whereas non_persistent_attrs, the management API returns an actual array of items:

    "non_persistent_attrs": [
      "ethnicity",
      "gender",
      "first_name",
      "family_name"
    ]

It needs to be a []stringbecause it accepts an arbitrary lists of attributes that I want to have on the denylist.

Ok, I've self-researched enough to know that []string is fine based on the above.

But in return, I do think the testing should be expanded beyond just adding it to L107 in https://github.com/go-auth0/auth0/blob/master/management/connection_test.go.

• I suggest utilizing expect.Expect on a few connection strategies (auth0 database, OIDC, AAD, Google are probably good ones to start with, all available if you have the time) to ensure the attributes are present. Reason: This at least validates your action in L107 that the statement is true.
• More testing around adding/removing attributes to the list (e.g. what the SDK does with Oauth2 scopes -> L192-L198. Reason: This would match a real-world scenario of adding/removing attributes for a specific connection provider.

But please enlighten me if I have misunderstood something here 👍

No, you were right about SetUserAttributes. I used a bad example!

[apamildner]

Thanks for the feedback. I added some tests now and rewrote some of the existing ones to actually check against the returned objects from the api. I ran into a problem though. The build/PR pipeline is not working even though I have done go generate ./... and I can't seem to figure out why.

[yvovandoorn]

The build/PR pipeline is not working even though I have done go generate ./... and I can't seem to figure out why.

Looks like your branch didn't bring in the PR (and subsequent PR that fixed the generate) I approved while you were adding tests.

I brought in the changes for management.gen.go to get this building, but you will need to rebase this PR with master in order to pass testing.

[yvovandoorn]

When I bring your branch current on my machine and try to test, it is failing (unfortunately)

go test ./... -v -run TestConnectionOptions                                     
testing: warning: no tests to run
PASS
ok  	gopkg.in/auth0.v5	(cached) [no tests to run]
testing: warning: no tests to run
PASS
ok  	gopkg.in/auth0.v5/internal/client	(cached) [no tests to run]
testing: warning: no tests to run
PASS
ok  	gopkg.in/auth0.v5/internal/tag	(cached) [no tests to run]
testing: warning: no tests to run
PASS
ok  	gopkg.in/auth0.v5/internal/testing/expect	(cached) [no tests to run]
=== RUN   TestConnectionOptions
=== RUN   TestConnectionOptions/GoogleOAuth2
2021/03/24 13:43:16 400 Bad Request: Payload validation error: 'Additional properties not allowed: strategy,name,id'.
FAIL	gopkg.in/auth0.v5/management	0.825s
FAIL

[apamildner]

It was indeed good that we extended the tests. Seems like not all connections support this feature which is what I assumed from the docs, but to be honest the documentation is quite lacking about what connections support this: https://auth0.com/docs/security/denylist-user-attributes. If you have time, it would be great if I could get this information and then I could remove the option for the unsupported connections.

[yvovandoorn]

Hey @apamildner,

I took a look at the source code around this and every connection object supports this. However where I led you astray and how your tests were written, is that denying attributes only works with the PATCH method.

The documentation does say this (To add attributes to the DenyList, make a PATCH call to the Update Connection endpoint of the Management API.).

So the tests will need to do something like this:

• Create Connection, verify created
• Add deny list, verify added
• Modify deny list, verify changed
• Delete Connection, verify gone

[yvovandoorn]

Any progress @apamildner on updating the tests to only work on PATCH vs at initial connection creation?

[apamildner]

Sorry for the delay, I finally had time to work on this! I added the tests as specified for two of the connections.

[yvovandoorn]

LGTM. Merging.