Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions lib/src/authentication/token_store.dart
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ import 'credential.dart';

/// Stores and manages authentication credentials.
class TokenStore {
TokenStore(this.cacheRootDir);
TokenStore(this.configDir);

/// Cache directory.
final String cacheRootDir;
final String configDir;

/// List of saved authentication tokens.
///
Expand Down Expand Up @@ -92,6 +92,7 @@ class TokenStore {

/// Writes [credentials] into "tokens.json".
void _saveCredentials(List<Credential> credentials) {
ensureDir(path.dirname(_tokensFile));
writeTextFile(
_tokensFile,
jsonEncode(<String, dynamic>{
Expand Down Expand Up @@ -165,5 +166,5 @@ class TokenStore {
}

/// Full path to the "tokens.json" file.
String get _tokensFile => path.join(cacheRootDir, 'tokens.json');
String get _tokensFile => path.join(configDir, 'tokens.json');
}
33 changes: 32 additions & 1 deletion lib/src/io.dart
Original file line number Diff line number Diff line change
Expand Up @@ -509,7 +509,18 @@ void createPackageSymlink(String name, String target, String symlink,
///
/// The "_PUB_TESTING" variable is automatically set for all the test code's
/// invocations of pub.
final bool runningFromTest = Platform.environment.containsKey('_PUB_TESTING');
final bool runningFromTest =
Platform.environment.containsKey('_PUB_TESTING') && _assertionsEnabled;

final bool _assertionsEnabled = () {
try {
assert(false);
// ignore: avoid_catching_errors
} on AssertionError {
return true;
}
return false;
}();

final bool runningFromFlutter =
Platform.environment.containsKey('PUB_ENVIRONMENT') &&
Expand Down Expand Up @@ -984,3 +995,23 @@ class PubProcessResult {

bool get success => exitCode == exit_codes.SUCCESS;
}

/// The location for dart-specific configuration.
final String dartConfigDir = () {
if (runningFromTest) {
return Platform.environment['_PUB_TEST_CONFIG_DIR'];
}
String configDir;
if (Platform.isLinux) {
configDir = Platform.environment['XDG_CONFIG_HOME'] ??
path.join(Platform.environment['HOME'], '.config');
} else if (Platform.isWindows) {
configDir = Platform.environment['APPDATA'];
} else if (Platform.isMacOS) {
configDir = path.join(
Platform.environment['HOME'], 'Library', 'Application Support');
} else {
configDir = path.join(Platform.environment['HOME'], '.config');
}
return path.join(configDir, 'dart');
}();
22 changes: 20 additions & 2 deletions lib/src/oauth2.dart
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,12 @@ void logout(SystemCache cache) {
log.message('Logging out of pub.dartlang.org.');
log.message('Deleting $credentialsFile');
_clearCredentials(cache);
// Test if we also have a legacy credentials file.
final legacyCredentialsFile = _legacyCredentialsFile(cache);
if (entryExists(legacyCredentialsFile)) {
log.message('Also deleting legacy credentials at $legacyCredentialsFile');
deleteEntry(legacyCredentialsFile);
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it make more sense if we move legacy credentials file into new path instead of deleting? It would be backwards-compatible with CI jobs that uses credentials.json for authentication & publishing packages.

Copy link
Contributor Author

@sigurdm sigurdm Sep 3, 2021

Choose a reason for hiding this comment

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

Here we are in the dart pub logout code - it should delete any credentials, so I don't think it makes sense to keep the legacy ones.

In the code for _credentialsFile below we use the credentials file from the legacy location if it exists there. We could consider moving it, but that would make it hard(er) to switch between older and newer versions of pub.

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sorry, 😅 haven't noticed it's for logout command.

}
} else {
log.message(
'No existing credentials file $credentialsFile. Cannot log out.');
Expand Down Expand Up @@ -180,8 +186,20 @@ void _saveCredentials(SystemCache cache, Credentials credentials) {
}

/// The path to the file in which the user's OAuth2 credentials are stored.
String _credentialsFile(SystemCache cache) =>
path.join(cache.rootDir, 'credentials.json');
///
/// This used to be PUB_CACHE/credentials.json. But the pub cache is not the
/// best place for storing secrets, as it might be shared.
///
/// To provide backwards compatibility we use the legacy file if only it exists.
String _credentialsFile(SystemCache cache) {
final newCredentialsFile = path.join(dartConfigDir, 'pub_credentials.json');
return [newCredentialsFile, _legacyCredentialsFile(cache)]
.firstWhere(fileExists, orElse: () => newCredentialsFile);
}

String _legacyCredentialsFile(SystemCache cache) {
return path.join(cache.rootDir, 'credentials.json');
}

/// Gets the user to authorize pub as a client of pub.dartlang.org via oauth2.
///
Expand Down
2 changes: 1 addition & 1 deletion lib/src/system_cache.dart
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ class SystemCache {
/// Defaults to `false`.
SystemCache({String rootDir, bool isOffline = false})
: rootDir = rootDir ?? SystemCache.defaultDir,
tokenStore = TokenStore(rootDir ?? SystemCache.defaultDir) {
tokenStore = TokenStore(dartConfigDir) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's also rename tokens.json to pub-tokens.json.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes!
Done!

for (var source in sources.all) {
if (source is HostedSource) {
_boundSources[source] = source.bind(this, isOffline: isOffline);
Expand Down
64 changes: 50 additions & 14 deletions test/descriptor.dart
Original file line number Diff line number Diff line change
Expand Up @@ -186,30 +186,66 @@ Descriptor hostedCache(Iterable<Descriptor> contents, {int port}) {
]);
}

/// Describes the file in the system cache that contains the client's OAuth2
/// Describes the file that contains the client's OAuth2
/// credentials. The URL "/token" on [server] will be used as the token
/// endpoint for refreshing the access token.
Descriptor credentialsFile(PackageServer server, String accessToken,
{String refreshToken, DateTime expiration}) {
return dir(cachePath, [
file(
return dir(
configPath,
[
file(
'pub_credentials.json',
_credentialsFileContent(
server,
accessToken,
refreshToken: refreshToken,
expiration: expiration,
),
),
],
);
}

Descriptor legacyCredentialsFile(PackageServer server, String accessToken,
{String refreshToken, DateTime expiration}) {
return dir(
cachePath,
[
file(
'credentials.json',
oauth2.Credentials(accessToken,
refreshToken: refreshToken,
tokenEndpoint: Uri.parse(server.url).resolve('/token'),
scopes: [
'openid',
'https://www.googleapis.com/auth/userinfo.email',
],
expiration: expiration)
.toJson())
]);
_credentialsFileContent(
server,
accessToken,
refreshToken: refreshToken,
expiration: expiration,
),
),
],
);
}

String _credentialsFileContent(
PackageServer server,
String accessToken, {
String refreshToken,
DateTime expiration,
}) =>
oauth2.Credentials(
accessToken,
refreshToken: refreshToken,
tokenEndpoint: Uri.parse(server.url).resolve('/token'),
scopes: [
'openid',
'https://www.googleapis.com/auth/userinfo.email',
],
expiration: expiration,
).toJson();

/// Describes the file in the system cache that contains credentials for
/// third party hosted pub servers.
Descriptor tokensFile([Map<String, dynamic> contents = const {}]) {
return dir(cachePath,
return dir(configPath,
[file('tokens.json', contents != null ? jsonEncode(contents) : null)]);
}

Expand Down
42 changes: 39 additions & 3 deletions test/oauth2/logout_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import '../descriptor.dart' as d;
import '../test_pub.dart';

void main() {
test('with an existing credentials.json, deletes it.', () async {
test('with an existing credentials file, deletes it.', () async {
await servePackages();
await d
.credentialsFile(globalPackageServer, 'access token',
Expand All @@ -21,13 +21,49 @@ void main() {
await runPub(
args: ['logout'], output: contains('Logging out of pub.dartlang.org.'));

await d.dir(configPath, [d.nothing('pub_credentials.json')]).validate();
});

test(
'with an existing credentials file stored in the legacy location, deletes both.',
() async {
await servePackages();
await d
.credentialsFile(
globalPackageServer,
'access token',
refreshToken: 'refresh token',
expiration: DateTime.now().add(Duration(hours: 1)),
)
.create();

await d
.legacyCredentialsFile(
globalPackageServer,
'access token',
refreshToken: 'refresh token',
expiration: DateTime.now().add(Duration(hours: 1)),
)
.create();

await runPub(
args: ['logout'],
output: allOf(
[
contains('Logging out of pub.dartlang.org.'),
contains('Also deleting legacy credentials at ')
],
),
);

await d.dir(cachePath, [d.nothing('credentials.json')]).validate();
await d.dir(configPath, [d.nothing('pub_credentials.json')]).validate();
});
test('with no existing credentials.json, notifies.', () async {
await d.dir(cachePath, [d.nothing('credentials.json')]).create();
await d.dir(configPath, [d.nothing('pub_credentials.json')]).create();
await runPub(
args: ['logout'], output: contains('No existing credentials file'));

await d.dir(cachePath, [d.nothing('credentials.json')]).validate();
await d.dir(configPath, [d.nothing('pub_credentials.json')]).validate();
});
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ void main() {
await d.validPackage.create();

await servePackages();
await d.dir(cachePath, [d.file('credentials.json', '{bad json')]).create();
await d.dir(
configPath, [d.file('pub_credentials.json', '{bad json')]).create();

var pub = await startPublish(globalPackageServer);
await confirmPublish(pub);
Expand Down
26 changes: 16 additions & 10 deletions test/test_pub.dart
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ String yaml(value) => jsonEncode(value);
/// sandbox directory.
const String cachePath = 'cache';

/// The path of the config directory used for tests, relative to the
/// sandbox directory.
const String configPath = '.config';

/// The path of the mock app directory used for tests, relative to the sandbox
/// directory.
const String appPath = 'myapp';
Expand Down Expand Up @@ -326,16 +330,17 @@ void symlinkInSandbox(String target, String symlink) {
///
/// If [environment] is given, any keys in it will override the environment
/// variables passed to the spawned process.
Future<void> runPub(
{List<String> args,
output,
error,
outputJson,
silent,
int exitCode,
String workingDirectory,
Map<String, String> environment,
List<String> input}) async {
Future<void> runPub({
List<String> args,
output,
error,
outputJson,
silent,
int exitCode,
String workingDirectory,
Map<String, String> environment,
List<String> input,
}) async {
exitCode ??= exit_codes.SUCCESS;
// Cannot pass both output and outputJson.
assert(output == null || outputJson == null);
Expand Down Expand Up @@ -415,6 +420,7 @@ Map<String, String> getPubTestEnvironment([String tokenEndpoint]) {
var environment = {
'CI': 'false', // unless explicitly given tests don't run pub in CI mode
'_PUB_TESTING': 'true',
'_PUB_TEST_CONFIG_DIR': _pathInSandbox(configPath),
'PUB_CACHE': _pathInSandbox(cachePath),
'PUB_ENVIRONMENT': 'test-environment',

Expand Down
8 changes: 4 additions & 4 deletions test/token/add_token_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -75,24 +75,24 @@ void main() {
});

test('with invalid server url returns error', () async {
await d.dir(cachePath).create();
await d.dir(configPath).create();
await runPub(
args: ['token', 'add', 'http:;://invalid-url,.com'],
error: contains('Invalid [hosted-url]'),
exitCode: exit_codes.USAGE,
);

await d.dir(cachePath, [d.nothing('tokens.json')]).validate();
await d.dir(configPath, [d.nothing('tokens.json')]).validate();
});

test('with non-secure server url returns error', () async {
await d.dir(cachePath).create();
await d.dir(configPath).create();
await runPub(
args: ['token', 'add', 'http://mypub.com'],
error: contains('Unsecure package repository could not be added.'),
exitCode: exit_codes.DATA,
);

await d.dir(cachePath, [d.nothing('tokens.json')]).validate();
await d.dir(configPath, [d.nothing('tokens.json')]).validate();
});
}
2 changes: 1 addition & 1 deletion test/token/remove_token_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,6 @@ void main() {

await runPub(args: ['token', 'remove', '--all']);

await d.dir(cachePath, [d.nothing('tokens.json')]).validate();
await d.dir(configPath, [d.nothing('tokens.json')]).validate();
});
}