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

fix: apply scopes from argocd-rbac-cm to project jwt group searches #3508

Merged
merged 7 commits into from
May 25, 2020

Conversation

Erog38
Copy link
Contributor

@Erog38 Erog38 commented Apr 29, 2020

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Optional. My organization is added to USERS.md.
  • I've signed the CLA and my build is green (troubleshooting builds).

@CLAassistant
Copy link

CLAassistant commented Apr 29, 2020

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Apr 29, 2020

Codecov Report

Merging #3508 into master will increase coverage by 0.28%.
The diff coverage is 70.58%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3508      +/-   ##
==========================================
+ Coverage   40.99%   41.28%   +0.28%     
==========================================
  Files         114      114              
  Lines       16403    16447      +44     
==========================================
+ Hits         6725     6790      +65     
+ Misses       8797     8776      -21     
  Partials      881      881              
Impacted Files Coverage Δ
server/rbacpolicy/rbacpolicy.go 78.87% <0.00%> (-2.29%) ⬇️
server/session/session.go 0.00% <0.00%> (ø)
server/project/project.go 61.40% <100.00%> (ø)
server/server.go 55.47% <100.00%> (ø)
util/jwt/jwt.go 58.06% <100.00%> (ø)
util/session/sessionmanager.go 68.11% <100.00%> (ø)
server/application/application.go 27.97% <0.00%> (-0.35%) ⬇️
pkg/apis/application/v1alpha1/types.go 62.03% <0.00%> (+2.89%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 991ee9b...65092c4. Read the comment docs.

@Erog38
Copy link
Contributor Author

Erog38 commented Apr 29, 2020

I definitely signed the CLA and updated my git email for the commit, so I'm not sure what to do for it to recognize it was signed.

@jannfis
Copy link
Member

jannfis commented Apr 29, 2020

Hi @Erog38, thanks a lot for your contribution!

The changes look good from a technical perspective to me, I just have a small request: Is there an existing issue describing the problem this PR is solving? If yes, can you please link it. If not, can you please elaborate a little on the use case of this PR?

Thank you!

@Erog38
Copy link
Contributor Author

Erog38 commented Apr 29, 2020

Thanks @jannfis, this fixes the TODO here:

// TODO: groups is hard-wired but we should really be honoring the 'scopes' section in argocd-rbac-cm.

@dirtycajunrice
Copy link
Contributor

@jannfis For a little more perspective, we use amazon cognito which does not allow using a groups scope whatsoever and cognito:groups is only in the JWT. This will allow us to specify any jwt claim as what holds the groups scope. (This also unblocks us from completely adopting ArgoCD)

@Erog38
Copy link
Contributor Author

Erog38 commented May 8, 2020

@jannfis Is there any more information I can help provide for you?

@jannfis jannfis self-requested a review May 15, 2020 15:26
@jannfis
Copy link
Member

jannfis commented May 15, 2020

@Erog38 Sorry for the delay, I will start reviewing this PR now :)

Copy link
Member

@jannfis jannfis left a comment

Choose a reason for hiding this comment

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

Thanks for your submission and sorry for the delay in review.

I have reviewed your change and manually tested for regression, it looks good so far to me. I have some minor comments (see below), I feel you should run make lint over your code, maybe I have missed some white space issue ;)

@jannfis
Copy link
Member

jannfis commented May 15, 2020

@jessesuen Maybe you want to take a look at this change because it's OAuth related, and I think you're the one knowing it inside-out.

@jannfis jannfis requested a review from jessesuen May 15, 2020 16:20
@Erog38
Copy link
Contributor Author

Erog38 commented May 15, 2020

I adjusted the whitespace issues , please let me know if I can adjust anything else, I want to get this out to our integration environment soon so we can start going production ready 😃

@Erog38 Erog38 force-pushed the feature/rbac-apply-scopes-projects branch from 41f8dbd to a116d24 Compare May 15, 2020 21:39
@jannfis
Copy link
Member

jannfis commented May 17, 2020

Hi @Erog38, as the gitops-engine PRs (#3066 and #3599) were just merged, I think you need to resync your PR with master branch again and merge any required changes manually.

This change was very intrusive, and probably requires manual rework of most pending PRs in the queue. Sorry for that, but we couldn't wait any longer with that change.

@Erog38
Copy link
Contributor Author

Erog38 commented May 19, 2020

@jannfis no worries, I know how these things are. I've brought the branch up to date. it looks like the workflow is failing on an unauthorized error from Sonarcloud. Do you know anything about this?

Copy link
Member

@jannfis jannfis left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for going the extra mile!

@jannfis
Copy link
Member

jannfis commented May 19, 2020

it looks like the workflow is failing on an unauthorized error from Sonarcloud. Do you know anything about this?

We're still unsure why this step works for some users while it won't work for others. I think it's some bug (or feature?) with CircleCI.

@Erog38
Copy link
Contributor Author

Erog38 commented May 20, 2020

Sorry @jannfis but is there another step I can get here for this to be merged as well?

@jannfis
Copy link
Member

jannfis commented May 20, 2020

Hey @Erog38, all is fine from my side. I was just thinking that @jessesuen might want to take a look too before merging this.

@jannfis jannfis merged commit 8aadc31 into argoproj:master May 25, 2020
@ragarcia26 ragarcia26 mentioned this pull request Jun 3, 2020
5 tasks
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.

4 participants