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

Access Token APIs #5797

Merged
merged 18 commits into from Feb 1, 2019
Merged

Access Token APIs #5797

merged 18 commits into from Feb 1, 2019

Conversation

@GaneshSPatil
Copy link
Contributor

@GaneshSPatil GaneshSPatil commented Jan 31, 2019

issue link: #5354

Add API endpoints to -

  1. generate a token
  2. get a token by name(without a token value)
  3. list all tokens
  4. Revoke a token for the user by token name

Note: Deletion of the token is not required as the token usage information is useful for audit purpose

@bdpiprava bdpiprava force-pushed the auth-token-store-in-db branch from 1d05bf7 to 79ad70d Feb 1, 2019
@bdpiprava bdpiprava added this to the Release 19.2.0 milestone Feb 1, 2019
@bdpiprava bdpiprava merged commit 236d4ba into gocd:master Feb 1, 2019
3 of 5 checks passed
@bdpiprava bdpiprava deleted the auth-token-store-in-db branch Feb 1, 2019
"description" : token.getDescription(),
"auth_config_id": token.authConfigId,
"_meta" : [
"is_revoked" : token.isRevoked(),
Copy link
Member

@ketan ketan Feb 1, 2019

Choose a reason for hiding this comment

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

Is there a need for is_ prefix?

"is_revoked" : token.isRevoked(),
"revoked_at" : null,
"created_at" : jsonDate(token.getCreatedAt()),
"last_used_at": null
Copy link
Member

@ketan ketan Feb 1, 2019

Choose a reason for hiding this comment

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

Do we really want this? Every API call will potentially write to the DB.

Copy link
Contributor Author

@GaneshSPatil GaneshSPatil Feb 1, 2019

Choose a reason for hiding this comment

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

@ketan -- You're right. This might cause performance issues, as every request will require a DB call.

We're thinking of throttling it somehow or storing it in memory.

private String value;
private String originalValue;
private String description;
private Boolean isRevoked = false;
Copy link
Member

@ketan ketan Feb 1, 2019

Choose a reason for hiding this comment

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

Is this intentionally set to Boolean instead of boolean. There's always a possibility of null with Boolean objects. If the field is not-nullable, would boolean is a be better option?

public AccessToken(String name, String value, String description, Boolean isRevoked, Date createdAt, Date lastUsed) {
this(name, value, description, isRevoked);
this.createdAt = new Timestamp(createdAt.getTime());
setLastUsed(lastUsed);
Copy link
Member

@ketan ketan Feb 1, 2019

Choose a reason for hiding this comment

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

this.lastUsed = lastUsed?

Copy link
Contributor Author

@GaneshSPatil GaneshSPatil Feb 1, 2019

Choose a reason for hiding this comment

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

@ketan -- the data type of lastUsed is the timestamp, whereas what we send in, is Date.
So, we need to convert Date into Timestamp, which is done by the setter.

Didn't want to duplicate the following logic at multiple places, hence kept it this way.
this.lastUsed = lastUsed != null ? new Timestamp(lastUsed.getTime()) : null;

import java.util.Date;
import java.util.Objects;

public class AccessToken extends PersistentObject {
Copy link
Member

@ketan ketan Feb 1, 2019

Choose a reason for hiding this comment

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

Do we really need so many constructors on this class?

saltValue VARCHAR(255) UNIQUE NOT NULL,
description VARCHAR(1024),
isRevoked BOOLEAN,
revokedAt TIMESTAMP,
Copy link
Member

@ketan ketan Feb 1, 2019

Choose a reason for hiding this comment

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

Should we consider recording the reason why it was revoked?

Copy link
Contributor Author

@GaneshSPatil GaneshSPatil Feb 1, 2019

Choose a reason for hiding this comment

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

Yeah. Good point. We can add an optional revoked_description.

get("", mimeType, this::getAllAccessTokens);
post("", mimeType, this::createAccessToken);
patch(String.format("%s%s/revoke", Routes.AccessToken.USERNAME, Routes.AccessToken.TOKEN_NAME), mimeType, this::revokeAccessToken);
get(Routes.AccessToken.TOKEN_NAME, mimeType, this::getAccessToken);
Copy link
Member

@ketan ketan Feb 1, 2019

Choose a reason for hiding this comment

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

Seems to imply that token names are unique, which they are not. Perhaps just use the database ID?

Copy link
Member

@ketan ketan Feb 1, 2019

Choose a reason for hiding this comment

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

To extend this argument further, should the revoke url also use the ID?

return null;
}

if (description != null && description.length() > 1024) {
Copy link
Member

@ketan ketan Feb 1, 2019

Choose a reason for hiding this comment

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

Don't see why we need this restriction? Just make it a clob in the DB. Won't be an overhead.


String digestToken(String originalToken, String salt) throws Exception {
SecretKeyFactory factory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA256");
SecretKey key = factory.generateSecret(new PBEKeySpec(originalToken.toCharArray(), salt.getBytes(), DEFAULT_ITERATIONS, DESIRED_KEY_LENGTH));
Copy link
Member

@ketan ketan Feb 1, 2019

Choose a reason for hiding this comment

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

We had discussed about storing these parameters (iteration count, key length, probably with the algorithm, in this case — PBKDF2WithHmacSHA256) in the DB, so admins can choose to tweak these parameters at a later point without breaking existing tokens.

Copy link
Contributor

@bdpiprava bdpiprava Feb 11, 2019

Choose a reason for hiding this comment

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

We will run migration later if needed.

}

private static String generateSalt() throws Exception {
return Base64.encodeBase64String(SecureRandom.getInstance("SHA1PRNG").generateSeed(SALT_LENGTH));
Copy link
Member

@ketan ketan Feb 1, 2019

Choose a reason for hiding this comment

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

Store this in the DB too, in case we need to change the algorithm later?

Copy link
Contributor

@bdpiprava bdpiprava Feb 11, 2019

Choose a reason for hiding this comment

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

same as above

@maheshp maheshp added this to Done in 19.2.0 Feb 5, 2019
kierarad added a commit to kierarad/gocd that referenced this issue Feb 6, 2019
* Define a AuthTokenSqlMapDao to store and retrieve token information from DB

* Introduce Auth Token API to manage auth tokens

* Store the hashed token instead of storing the raw value

* Some tests

* Store information of the user who has created the auth token

* Use SHA-256 instead of MD5 for hashing token value

* Change username data type from VARCHAR to VARCHAR_IGNORECASE

* Introduce AuthTokenMother to create authToken in tests

* Fix auth token controller specs

* Remove unused class variable from test

* Introduce auth token index endpoint to fetch all the tokens belonging to me

* Add extra attributes to auth token
* Salt Id
* Salt Value
* Auth Config Id
* Revoked At

* Add support for revoking access tokens
Add a method to validate and fetch access token object provided access token

* Rename controller package and module

* rename service and dao

* Rename auth token package

* Rename rules under url rewrite

* Renamed few more occurences of auth token to acccess tokens
@rajiesh rajiesh moved this from Done to QA Done in 19.2.0 Feb 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
19.2.0
QA Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants