feat: implement Service Account / Local Users #3215

alexmt · 2020-03-10T01:29:10Z

Closes #3185
Closes #2108
Closes #3221

PR introduces local users support in Argo CD.

CLI commands

List existing accounts:

argocd account list                                                                                                                                                      NAME   ENABLED  CAPABILITIES
admin  true     apiKey, login
test   true     apiKey

get account details:

Name:               admin
Enabled:            true
Capabilities:       apiKey, login

Tokens:
ID                                    ISSUED AT                  EXPIRING AT
2b8174fb-5e67-4f34-80d8-511f91422a6a  2020-03-09T14:31:25-07:00  never
c4f53804-0182-4021-b502-09a947f0488f  2020-03-09T14:26:34-07:00  2020-03-09T14:26:34-07:00 (expired)

generate token for an account:

argocd account generate-token --account test
xxxxxxxxxxxxxxxxxxxxx

delete existing account token:

argocd account delete-token 2b8174fb-5e67-4f34-80d8-511f91422a6a

change local user password:

 argocd account update-password --account <name> --current-password <current-user-password> --new-password <new-password-for-localuser>

UI

util/settings/settings_test.go

util/settings/settings.go

util/settings/accounts_test.go

cmd/argocd/commands/account.go

server/account/account.go

codecov · 2020-03-10T02:51:26Z

Codecov Report

Merging #3215 into master will increase coverage by 0.17%.
The diff coverage is 41.39%.

@@            Coverage Diff             @@
##           master    #3215      +/-   ##
==========================================
+ Coverage   38.81%   38.99%   +0.17%     
==========================================
  Files         169      172       +3     
  Lines       18321    18764     +443     
  Branches      272      237      -35     
==========================================
+ Hits         7112     7317     +205     
- Misses      10333    10540     +207     
- Partials      876      907      +31

Impacted Files	Coverage Δ
cmd/argocd/commands/account.go	`0.00% <0.00%> (ø)`
ui/src/app/shared/models.ts	`100.00% <ø> (ø)`
util/settings/settings.go	`38.72% <11.11%> (+0.81%)`	⬆️
util/session/sessionmanager.go	`55.94% <55.55%> (-2.46%)`	⬇️
ui/src/app/shared/services/accounts-service.ts	`58.33% <58.33%> (ø)`
server/account/account.go	`63.02% <69.69%> (+19.27%)`	⬆️
util/settings/accounts.go	`73.33% <73.33%> (ø)`
server/project/project.go	`59.43% <100.00%> (ø)`
server/rbacpolicy/rbacpolicy.go	`80.30% <100.00%> (+0.61%)`	⬆️
server/server.go	`57.59% <100.00%> (ø)`
... and 9 more

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 42d5723...0addefc. Read the comment docs.

server/account/account_test.go

server/account/account.go

jannfis

Wow, awesome work. Looks good to me so far, I have some minor comments see below.

jannfis · 2020-03-16T21:28:12Z

cmd/argocd/commands/account.go

+		Use:     "get",
+		Short:   "Get account details",
+		Example: "argocd account get",


IMHO should provide information on the argument the command optional takes - i.e. the account name. Also should be made clear in the example that with no parameters, current account will be get, and if parameter given, information for named account will be retrieved.

Agree. Updated example of all commands that defaults to the current account.

jannfis · 2020-03-16T21:31:39Z

cmd/argocd/commands/account.go

+		Use:     "generate-token",
+		Short:   "Generate account token",
+		Example: "argocd account generate-token",


Same as above, information about the arguments should be given.

jannfis · 2020-03-16T21:32:14Z

cmd/argocd/commands/account.go

+		Use:     "delete-token",
+		Short:   "Deletes account token",
+		Example: "argocd account delete-token ID",


Here as well - more information about the parameters, please.

jannfis · 2020-03-16T21:32:56Z

common/common.go

@@ -38,8 +38,6 @@ const (

 // Default paths on the pod's file system
 const (
-	// The default base path where application config is located
-	DefaultPathAppConfig = "/app/config"


Hm, is this not used anywhere?

Yes, look like we forgot to delete it after some refactoring.

jannfis · 2020-03-17T06:27:53Z

server/account/account.go

@@ -28,39 +30,49 @@ type Server struct {
 // NewServer returns a new instance of the Session service
 func NewServer(sessionMgr *session.SessionManager, settingsMgr *settings.SettingsManager, enf *rbac.Enforcer) *Server {
 	return &Server{sessionMgr, settingsMgr, enf}
-
 }

 // UpdatePassword updates the password of the local admin superuser.


This comment should be adapted to reflect reality. Probably there are more

jannfis · 2020-03-17T13:40:22Z

server/account/account.proto

+	rpc GetAccount(GetAccountRequest) returns (Account) {
+    	option (google.api.http).get = "/api/v1/account/{name}";
+    }


Indentation

jannfis · 2020-03-17T18:21:49Z

util/session/sessionmanager.go

 	return &s
 }

 // Create creates a new token for a given subject (user) and returns it as a string.
 // Passing a value of `0` for secondsBeforeExpiry creates a token that never expires.
-func (mgr *SessionManager) Create(subject string, secondsBeforeExpiry int64) (string, error) {
+func (mgr *SessionManager) Create(subject string, secondsBeforeExpiry int64, id string) (string, error) {


We should clarify what id is used for here.

done. Updated comment

jannfis · 2020-03-17T18:29:07Z

util/settings/accounts.go

@@ -0,0 +1,307 @@
+package settings


General thought on this package - I think all Public methods and types should be rather well commented.

Agree. Added missing comments

jannfis · 2020-03-17T18:31:23Z

util/settings/accounts.go

+}
+
+func parseAdminAccount(secret *v1.Secret, cm *v1.ConfigMap) (*Account, error) {
+	adminAccount := &Account{Enabled: true, Capabilities: []AccountCapability{AccountCapabilityApiKey, AccountCapabilityLogin}}


Hm, do we really want to allow admin account access to API by default? Or is that for backwards compatibility? If so, we should make a deprecation notice, that admin access to API will be disabled from v2 onwards or something similar.

Nice catch. Admin account should not be able to generate API keys by default. That functionality does not exist before 1.5 so we are not breaking backward compatibility. Updated the default capabilities.

jannfis · 2020-03-17T18:34:21Z

util/settings/accounts_test.go

@@ -0,0 +1,67 @@
+package settings


General thought on this test package. I think we should also test for bad/malformed input data, that would also come up with more code coverage.

Done. Added missing tests

alexmt · 2020-03-17T20:53:11Z

Thank you for review @jannfis ! PTAL

jannfis

LGTM

alex1989hu · 2020-03-19T06:57:09Z

@alexmt: thanks for the documentation part

omerlh · 2020-03-25T10:25:44Z

Something unclear to me - look like you can define a token in the config map (by looking on the PR), but how can you use this token? What defined in the PR is claims, how can I sign it?

alexmt · 2020-03-26T21:09:00Z

hello @omerlh ,

User copy generated token and use it same as token generated during normal login process. Only token id, issuedAt and expiringAt time fields are stored in secret. This allows user to know when the token is going to expire and revoke it if necessary

omerlh · 2020-04-02T05:02:55Z

So there is no declarative way to create tokens? only using the CLI?

daurnimator · 2020-05-26T12:57:04Z

util/settings/accounts.go

+	for i := range a.Capabilities {
+		items = append(items, string(a.Capabilities[i]))
+	}
+	return strings.Join(items, ",")


This results in "apiKey, login" getting turned into "apiKey,login"

alexmt requested a review from jessesuen March 10, 2020 01:29

golangcibot reviewed Mar 10, 2020

View reviewed changes

alexmt force-pushed the 3185-local-users branch 2 times, most recently from 5b1e3db to 148e925 Compare March 10, 2020 02:51

alexmt force-pushed the 3185-local-users branch 2 times, most recently from 13878e6 to cc73f4f Compare March 10, 2020 04:03

alexmt mentioned this pull request Mar 10, 2020

JWT invalid => Password for superuser has changed since token issued #2108

Closed

alexmt force-pushed the 3185-local-users branch from cc73f4f to feb513b Compare March 10, 2020 18:05

golangcibot reviewed Mar 10, 2020

View reviewed changes

server/account/account_test.go Outdated Show resolved Hide resolved

alexmt force-pushed the 3185-local-users branch 3 times, most recently from cccec64 to 1f400db Compare March 10, 2020 20:25

golangcibot reviewed Mar 10, 2020

View reviewed changes

server/account/account.go Outdated Show resolved Hide resolved

alexmt force-pushed the 3185-local-users branch from 1f400db to 58e0259 Compare March 10, 2020 20:52

alexmt marked this pull request as ready for review March 10, 2020 21:22

alexmt force-pushed the 3185-local-users branch from 3cbd93f to ee02a6e Compare March 11, 2020 19:16

alexmt requested a review from jannfis March 17, 2020 00:31

jessesuen approved these changes Mar 17, 2020

View reviewed changes

jannfis requested changes Mar 17, 2020

View reviewed changes

alexmt force-pushed the 3185-local-users branch from ee02a6e to 214a6fa Compare March 17, 2020 20:49

alexmt force-pushed the 3185-local-users branch from 214a6fa to 0da95f0 Compare March 17, 2020 21:09

alexmt added 3 commits March 17, 2020 14:21

feat: implement Service Account / Local Users

c59c048

add local users documentation

4c9eb27

implement accounts management UI

e47257c

alexmt force-pushed the 3185-local-users branch from 0da95f0 to 536e323 Compare March 17, 2020 21:21

address code review notes

0addefc

alexmt force-pushed the 3185-local-users branch from 536e323 to 0addefc Compare March 17, 2020 21:37

jannfis approved these changes Mar 17, 2020

View reviewed changes

alexmt merged commit 3c2be61 into argoproj:master Mar 17, 2020

alexmt deleted the 3185-local-users branch March 17, 2020 22:31

alex1989hu mentioned this pull request Mar 19, 2020

feat: Disable Admin Login (fixes #3019) #3179

Merged

4 tasks

daurnimator reviewed May 26, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement Service Account / Local Users #3215

feat: implement Service Account / Local Users #3215

alexmt commented Mar 10, 2020 •

edited

Loading

codecov bot commented Mar 10, 2020 •

edited

Loading

jannfis left a comment

jannfis Mar 16, 2020

alexmt Mar 17, 2020

jannfis Mar 16, 2020

jannfis Mar 16, 2020

alexmt Mar 17, 2020

jannfis Mar 16, 2020

alexmt Mar 17, 2020

jannfis Mar 17, 2020

alexmt Mar 17, 2020

jannfis Mar 17, 2020

alexmt Mar 17, 2020

jannfis Mar 17, 2020

alexmt Mar 17, 2020

jannfis Mar 17, 2020

alexmt Mar 17, 2020

jannfis Mar 17, 2020

alexmt Mar 17, 2020

jannfis Mar 17, 2020

alexmt Mar 17, 2020

alexmt commented Mar 17, 2020

jannfis left a comment

alex1989hu commented Mar 19, 2020

omerlh commented Mar 25, 2020

alexmt commented Mar 26, 2020

omerlh commented Apr 2, 2020

daurnimator May 26, 2020

feat: implement Service Account / Local Users #3215

feat: implement Service Account / Local Users #3215

Conversation

alexmt commented Mar 10, 2020 • edited Loading

CLI commands

UI

codecov bot commented Mar 10, 2020 • edited Loading

Codecov Report

jannfis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexmt commented Mar 17, 2020

jannfis left a comment

Choose a reason for hiding this comment

alex1989hu commented Mar 19, 2020

omerlh commented Mar 25, 2020

alexmt commented Mar 26, 2020

omerlh commented Apr 2, 2020

Choose a reason for hiding this comment

alexmt commented Mar 10, 2020 •

edited

Loading

codecov bot commented Mar 10, 2020 •

edited

Loading