new connector for Atlassian Crowd #1515
Conversation
There was a problem hiding this comment.
Took an initial pass, please bear with me! Thanks for the contribution, this is nice.
- Could you please add a comment referring to some kind of API docs for this?
- Let's please add tests -- it's difficult to have any integration-test-like stuff for this, but you can glance at the tests for github/gitlab -- there's still some meaningful unit tests possible, I believe.
| } | ||
|
|
||
| // Open returns a strategy for logging in through Atlassian Crowd. | ||
| func (c *Config) Open(id string, logger log.Logger) (connector.Connector, error) { |
There was a problem hiding this comment.
[nit] if id isn't used, let's do this:
| func (c *Config) Open(id string, logger log.Logger) (connector.Connector, error) { | |
| func (c *Config) Open(_ string, logger log.Logger) (connector.Connector, error) { |
| } | ||
|
|
||
| type connectorData struct { | ||
| // GitLab's OAuth2 tokens never expire. We don't need a refresh token. |
There was a problem hiding this comment.
Sorry, connectorData is not used in the Crowd connector. I removed it.
| return connector.Identity{}, false, nil | ||
| } | ||
|
|
||
| c.logger.Infof("crowd Login scopes: %+v", s) |
There was a problem hiding this comment.
Can we remove this or change it to debug? I'm not sure I'd want to see this for every login using this connector 😄
| IdleConnTimeout: 90 * time.Second, | ||
| TLSHandshakeTimeout: 10 * time.Second, | ||
| ExpectContinueTimeout: 1 * time.Second, | ||
| }, |
There was a problem hiding this comment.
❓ Why do we need this? Does it differ from http.DefaultTransport?
There was a problem hiding this comment.
There was a problem hiding this comment.
There is a difference with DefaultTransport in one field - ForceAttemptHTTP2.
| func (c *crowdConnector) authenticateWithPassword(ctx context.Context, client *http.Client, username string, password string) (invalidPass bool, err error) { | ||
| req, err := c.crowdUserManagementRequest(ctx, | ||
| "POST", | ||
| fmt.Sprintf("/session"), |
There was a problem hiding this comment.
| fmt.Sprintf("/session"), | |
| "/session", |
|
|
||
| var authError crowdAuthenticationError | ||
|
|
||
| c.logger.Infof("body: %s", string(body)) |
There was a problem hiding this comment.
This can also go away I think.
| // read operations of the /api/v4/user endpoint | ||
| scopeUser = "read_user" | ||
| // used to retrieve groups from /oauth/userinfo | ||
| // https://docs.gitlab.com/ee/integration/openid_connect_provider.html |
There was a problem hiding this comment.
This plugin is based on gitlab plugin. Definitely code cleaning is required.
| scopeOpenID = "openid" | ||
| ) | ||
|
|
||
| // Config holds configuration options for Atlassian Crowd conector. |
There was a problem hiding this comment.
[typo]
| // Config holds configuration options for Atlassian Crowd conector. | |
| // Config holds configuration options for Atlassian Crowd connector. |
nabokihms
force-pushed
the
atlassian-crowd-connector
branch
3 times, most recently
from
95a30cd to
fbe878a
Compare
|
@srenatus can you take a look at the PR again? We run the Atlassian Crowd connector in production for a couple of months and everything seems fine!
If Thanks in advance! |
nabokihms
force-pushed
the
atlassian-crowd-connector
branch
from
fbe878a to
7ef1179
Compare
|
Is there anything that still needs to be done for this apart from someone giving it a positive review? |
|
@srenatus @bonifaido @sagikazarmark - I just setup dex with this connector and it works beautifully! I don't suppose any of you can give it a review so it can be included in the official dex builds? :-) |
|
Yay! Thanks guys! 😄 |
New connector for Atlassian Crowd identity management server. This initial implementation uses PasswordConnector ans should support groups and refresh. But there is no tests yet and sso feature is not tested with Jira or Confluence. Is this looks interesting?