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

Decouple access management from appeal #246

Closed
rahmatrhd opened this issue Aug 4, 2022 · 11 comments · Fixed by #249, #277 or #272
Closed

Decouple access management from appeal #246

rahmatrhd opened this issue Aug 4, 2022 · 11 comments · Fixed by #249, #277 or #272
Assignees
Labels

Comments

@rahmatrhd
Copy link
Member

rahmatrhd commented Aug 4, 2022

Summary

Currently, appeal acts as the information holder for approval flow and user access. We can decouple those responsibilities by creating a new entity called access that manages user access and its lifecycle.

Changes:

1. Process

  1. Once appeal is approved, guardian will create an access entry representing the current user access to the resource
  2. Access revocation will be done to access instead of appeal
  3. If a new appeal to the same resource created by the same user with the same role is approved, guardian will revoke the existing active access (if exists) and create a new one.

2. Entity

Appeal:

  type Appeal struct {
  	ID            string                 
  	ResourceID    string                 
  	PolicyID      string                 
  	PolicyVersion uint                   
  	Status        string                 
  	AccountID     string                 
  	AccountType   string                 
  	CreatedBy     string                 
  	Creator       interface{}            
  	Role          string                 
  	Permissions   []string               
  	Options       *AppealOptions         
  	Details       map[string]interface{} 
  	Labels        map[string]string      

- 	RevokedBy    string    
- 	RevokedAt    time.Time 
- 	RevokeReason string    

  	Policy    *Policy     
  	Resource  *Resource   
  	Approvals []*Approval 

  	CreatedAt time.Time 
  	UpdatedAt time.Time 
  }

Access:

+  type Access struct {
+ 	ID             string
+ 	Status         string // active | inactive
+ 	AccountID      string
+ 	AccountType    string
+ 	ResourceID     string
+ 	Permissions    []string
+ 	ExpirationDate *time.Time
+ 	AppealID       string
+ 	RevokedBy      string
+ 	RevokedAt      *time.Time
+ 	RevokeReason   string
+       CreatedBy      string
+ 	CreatedAt      time.Time
+ 	UpdatedAt      time.Time
+  }

3. Lifecycle

Appeal:

  1. pending
    2. canceled
-   2. active
+   2. approved
-     3. terminated
    2. rejected

Access:

+ 1. active
+   2. inactive
@mabdh
Copy link
Member

mabdh commented Aug 14, 2022

@rahmatrhd does the mean of *time.Time is a nullable time? So is it possible to have an access with null ExpirationDate ?

@rahmatrhd
Copy link
Member Author

@mabdh yes, that's the case for permanent access

@Chief-Rishab
Copy link
Member

Chief-Rishab commented Aug 15, 2022

@bsushmith and I were having some discussions on invite based providers, suppose a user requesting for a resource didn’t accept the invite within the given the time(24 hours), should we make him create the appeal on guardian again? In that case the managers/resource owners will have to approve the appeal again for the same user.

We were thinking if there’s a possibility to ensure that if the approvers have already approved the user's appeal, and the invite has expired, can we keep a boolean flag (verified) in Access Struct to check on that. Using this we can make another API call to resend the invite without going into the entire approval process again? This would simplify the invite based flow, instead of making the users and approvers do the same thing twice.
@rahmatrhd @AkarshSatija @ravisuhag @mabdh

@rahmatrhd
Copy link
Member Author

@bsushmith @Chief-Rishab I was thinking that guardian can have a job that periodically checks the invitation status to the provider and set the access status to active or canceled accordingly when the access created with initial status pending

@rahmatrhd
Copy link
Member Author

rahmatrhd commented Aug 23, 2022

in order to rollout this access entity while still maintaining the backward compatibility for the client (frontend), we need to have two separate releases/versions:

Release 1

  • introduce access entity
  • introduce GET /accesses, GET /me/accesses, GET /accesses/:id, PUT /accesses/:id/revoke, and PUT /accesses/revoke (bulk revoke accesses) APIs
  • deprecate PUT /appeals/:id/revoke, and POST /appeals/revoke (bulk revoke appeals) APIs
  • maintain the status of provider access in both appeal and access (see the table below for the mapping)
appeal status access status business logic changes db migration
pending N/A - -
canceled N/A - -
rejected N/A - -
active active create access entry when the appeal is activated active appeals will be migrated to active access
terminated inactive revoking active appeal or access will change the status of both entities, appeal -> terminated, access -> inactive terminated appeal records will be migrated to inactive access records along with the revoked_at, revoked_by, and revoke_reason information

Release 2

  • delete PUT /appeals/:id/revoke, and POST /appeals/revoke (bulk revoke appeals) APIs
  • only maintain the status of provider access in access (see the table below for the mapping)
appeal status access status business logic changes db migration
pending N/A - -
canceled N/A - -
rejected N/A - -
approved active change appeal status to approved instead of active when all approvals got approved update appeals with status = active to approved
approved inactive remove appeal revoke logic from accessService.Revoke and accessService.BulkRevoke update appeals with status = terminated to approved and remove revoked_at, revoked_by, and revoke_reason columns from appeals

In between those two releases we can update the client (frontend) to use access instead of appeal for listing list of user accesses

@mabdh @Chief-Rishab @ravisuhag @bsushmith @singhvikash11 please have a look and let me know if there's something I missed

@mabdh
Copy link
Member

mabdh commented Aug 24, 2022

In between those two releases we can update the client (frontend) to use access instead of appeal for listing list of user accesses

Why can't this be part of Release 1 ?

@rahmatrhd
Copy link
Member Author

@mabdh the releases I mentioned are specific for guardian (backend), updating the client going to be a separate task. But still, release1, update client, and release2 had to be done sequentially as each depends on the prior task

@mabdh
Copy link
Member

mabdh commented Aug 24, 2022

@rahmatrhd based on what you mention, updated client should be part of release 2 then?

@rahmatrhd
Copy link
Member Author

@mabdh the client (frontend) now is managed in different project/repo anyway, so the requirement for guardian is only to have two separate releases/versions with requirements I mentioned above. This strategy is basically to give the client chance to update to the new version without breaking/having downtime during the backend is using release1, and release2 is basically to remove the deprecated/unused features.

@mabdh
Copy link
Member

mabdh commented Aug 24, 2022

Oh I see got it, by client, I thought it is guardian cli.

@rahmatrhd rahmatrhd linked a pull request Aug 24, 2022 that will close this issue
10 tasks
@Chief-Rishab
Copy link
Member

Chief-Rishab commented Aug 25, 2022

(As discussed with @rahmatrhd ) Guardian CLI commands will also have to be updated in release 2

@rahmatrhd rahmatrhd reopened this Aug 28, 2022
@rahmatrhd rahmatrhd added this to the v0.4 milestone Aug 28, 2022
@rahmatrhd rahmatrhd removed this from the v0.4.0 milestone Sep 5, 2022
@rahmatrhd rahmatrhd linked a pull request Sep 6, 2022 that will close this issue
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment